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

[Samuel Thibault]

Damien Zammit, le jeu. 01 avril 2021 00:23:28 +1100, a ecrit:
> +void
> +acpi_ds_dump_method_stack(acpi_status status, ...)
> +//                     struct acpi_walk_state *walk_state,
> +//                     union acpi_parse_object *op)
> +{
> +     return;
> +}

I guess you can use backtrace() from glibc.

> +void
> +acpi_os_vprintf(const char *fmt, va_list args)
> +{
> +     static char buffer[512];
> +
> +     vsnprintf(buffer, 512, fmt, args);
> +     printf(buffer);

Why using svnprintf+printf rather than vprintf?

> +acpi_cpu_flags
> +acpi_os_acquire_lock(acpi_spinlock lockp)
> +{
> +     pthread_mutex_lock(&lockp);

That cannot be right. In C, parameters are always passed as a copy, so
here you have a copy of the lock, so you'll not actually be locking the
lock behind. What you want is making the acpi_spinlock pthread_mutex_t*,
so that it's a copy of the pointer to the mutex that you get, which you
can pass over to pthread_mutex_lock.

Also, better return the result.

> +void
> +acpi_os_delete_lock(acpi_spinlock handle)
> +{
> +}

Use pthread_mutex_destroy, you'll be able to check its return value, to
check for EBUSY.

> +/* FIXME: Need proper semaphores */

Yes, you should be able to just use sem_init/wait/post/destroy

I don't know what the semantic of max_units is. Posix most probably
doesn't provide it. At least you can check that it is below
SEM_VALUE_MAX (but sem_init will fail in that case already anyway).

> +acpi_status
> +acpi_os_wait_semaphore(acpi_handle handle, u32 units, u16 timeout)
> +{
> +     return AE_OK;
> +}

Wait means to acquire the semaphore, so sem_wait.

> +/* Supposed to be 64 bit free-running
> + * monotonic timer with 100ns granularity

100ns? How odd :)

> + */
> +u64
> +acpi_os_get_timer(void)
> +{
> +     struct timespec now;
> +
> +     clock_gettime(CLOCK_MONOTONIC, &now);
> +     return (u64)(now.tv_nsec / 100);
> +}

I guess it also wants the tv_sec part.

> +     virt_addr = mmap(NULL, size + into_page, PROT_READ | PROT_WRITE,

Rather write it into_page+size, that will better match what the reader
should understand.

> +                      MAP_SHARED | MAP_FIXED, fd_mem, nearest_page);

MAP_FIXED? I don't think you want that.

> +acpi_status
> +acpi_os_read_port(acpi_io_address port, u32 * value, u32 width)
> +{
> +     u32 dummy;
> +
> +     if (!value)
> +             value = &dummy;
> +
> +     *value = 0;
> +     if (width <= 8) {

I'd say reject width that is not 8, 16, or 32?

> +acpi_status
> +acpi_os_physical_table_override(struct acpi_table_header *existing_table,
> +                             acpi_physical_address *address,
> +                             u32 *table_length)
> +{
> +     *table_length = 0;
> +     *address = 0;

What is this for?

> +void
> +acpi_os_unmap_memory(void *virt, acpi_size size)
> +{
> +     void *freeme = (void *)trunc_page(virt);
> +     if (!freeme) {
> +             acpi_os_printf("Nothing to unmap\n");
> +             return;
> +     }
> +     munmap(freeme, size + (ptrdiff_t)(virt - trunc_page(virt)));
> +}

Similarly, better use +size

> +static acpi_status
> +acpi_os_read_iomem(void *virt_addr, u64 *value, u32 width)
> +{
> +

*value=0 is not needed here?

> +     switch (width) {
> +     case 8:
> +             *(u8 *) value = *((volatile u8 *)virt_addr);
> +             break;


> +void
> +acpi_os_sleep(u64 ms)
> +{
> +     usleep(1000 * ms);
> +}
> +
> +void
> +acpi_os_stall(u32 us)
> +{
> +     usleep(us);
> +}

What do they say about the difference between sleep and stall?

> +acpi_status
> +acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> +                               void *context)
> +{
> +     return AE_OK;
> +}

I'd ray rather print some TODO and return an error?

> + * Missing symbols to implement:
> + *
> + * x isdigit
> + * x isprint
> + * x isspace
> + * x isxdigit
> + * x tolower
> + * x toupper

Those should already be coming from libc

Samuel