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