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

[Samuel Thibault]

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.

> +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?

> +     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.

> +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.

Samuel