[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/5] acpica: Add acpi_init
From: |
Samuel Thibault |
Subject: |
Re: [PATCH 3/5] acpica: Add acpi_init |
Date: |
Thu, 1 Apr 2021 00:11:46 +0200 |
User-agent: |
NeoMutt/20170609 (1.8.3) |
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
- [hurd - ACPICA]: glue code for libacpica, Damien Zammit, 2021/03/31
- [PATCH 1/5] acpica: Add makefile, Damien Zammit, 2021/03/31
- [PATCH 2/5] acpica: Add config for GNU, Damien Zammit, 2021/03/31
- [PATCH 3/5] acpica: Add acpi_init, Damien Zammit, 2021/03/31
- Re: [PATCH 3/5] acpica: Add acpi_init,
Samuel Thibault <=
- [PATCH 4/5] acpi_init: Add enter_sleep method based on ACPI server, Damien Zammit, 2021/03/31
- [PATCH 5/5] acpi: Add ACPICA as a library to this server, Damien Zammit, 2021/03/31