Re: [PATCH 3/5] libacpica: Add acpi_init

[Damien Zammit]

On 3/4/21 11:49 pm, Samuel Thibault wrote:
> Damien Zammit, le sam. 03 avril 2021 23:16:33 +1100, a ecrit:
>> +acpi_status
>> +acpi_os_create_lock(acpi_spinlock *lockp)
>> +{
>> +    acpi_semaphore sema = NULL;
>> +    if (acpi_os_create_semaphore(1, 0, &sema) == AE_OK) {
>> +            *lockp = sema;
> 
> No, directly pass lockp to acpi_os_create_semaphore. You cannot assume
> that copying the content of a lock can produce a working lock. It
> happens to be ok for the sem_t implementation of GNU/Hurd, but that's
> only a coincidence.

OK

> 
>> +acpi_status
>> +acpi_os_wait_semaphore(acpi_semaphore handle, u32 units, u16 timeout)
>> +{
>> +    int i;
>> +
>> +    if (!timeout)
>> +            timeout = 1;
>> +
>> +    for (i = 0; i < timeout; i++) {
>> +            if (!sem_trywait(handle)) {
>> +                    return AE_OK;
> 
> Is timeout really expected to be the number of *times* to try to take
> the semaphore? Isn't it rather a time value that you can rather pass to
> sem_timedwait?

It was a cheap way to wait, yes, I cheated.  Because sem_timedwait
expects an epoch and I didn't have time to implement it properly yet.

>> +    if ((fd_mem = open("/dev/mem", O_RDWR)) < 0) {
>> +            acpi_os_printf("Can't open /dev/mem\n");
>> +            return (void *)-1;
>> +    }
>> +
>> +    /* MAP_FIXED so we can access particular physical regions */
> 
> ??? MAP_FIXED has nothing to do with the offset within the fd_mem. It is
> about the address you pass as first argument, here NULL, so MAP_FIXED is
> meaningless here.

OK

>> +acpi_status
>> +acpi_os_predefined_override(const struct acpi_predefined_names *init_val,
>> +                        acpi_string *new_val)
>> +{
>> +    if (!init_val || !new_val)
>> +            return AE_BAD_PARAMETER;
>> +
>> +    *new_val = init_val->val;
>> +
>> +    return AE_OK;
>> +}
> 
> What is the rationale for this function? It looks odd that it'd to be
> implemented so trivially.

This is to override the predefined strings, but we don't need to do anything 
special here.
I think *new_val = 0; would probably work too, but leaving it out completely 
made it fail.

Damien