On Sun, 2018-08-12 at 18:25 +0200, Hans de Goede wrote:
Thanks for the patch, few comments below.
> GpioInt ACPI event handlers may see there IRQ triggered immediately
> after requesting the IRQ (esp. level triggered ones). This means that
> they
> may run before any (builtin) other drivers have had a chance to
(builtin) other -> other (builtin)
> register
> their OpRegion handlers, leading to errors like this:
>
> [ 1.133274] ACPI Error: No handler for Region [PMOP]
> ((____ptrval____)) [UserDefinedRegion] (20180531/evregion-132)
> [ 1.133286] ACPI Error: Region UserDefinedRegion (ID=141) has no
> handler (20180531/exfldio-265)
> [ 1.133297] ACPI Error: Method parse/execution failed
> \_SB.GPO2._L01, AE_NOT_EXIST (20180531/psparse-516)
>
> We already defer the manual initial trigger of edge triggered
> interrupts
> by running it from a late_initcall handler, this commit replaces this
> with
> deferring the entire acpi_gpiochip_request_interrupts() call till
> then,
> fixing the problem of some OpRegions not being registered yet.
>
> Note that this removes the need to have a list of edge triggered
> handlers
> which need to run, since the entire acpi_gpiochip_request_interrupts()
> call
> is now delayed, acpi_gpiochip_request_interrupt() can call these
> directly
> now.
Should it have Fixes tag?
> + struct list_head deferred_req_irqs_list_entry;
Can we drop 'req' part from the name? It's way too long already. I think
we have enough context to understand to which list it will be chained
to.
> +static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
> +static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
> +static bool acpi_gpio_deferred_req_irqs_done;
Ditto.
> + mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> + defer = !acpi_gpio_deferred_req_irqs_done;
> + if (defer)
> + list_add(&acpi_gpio->deferred_req_irqs_list_entry,
> + &acpi_gpio_deferred_req_irqs_list);
> + mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +
> + if (defer)
> + return;
You might need no temporary variable if you rearrange the above like
lock()
if (...) {
list_add();
unlock();
return;
}
unlock();
(Personally I like this slightly more, but have not been insisting.)
> + mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> + if (!list_empty(&acpi_gpio->deferred_req_irqs_list_entry))
> + list_del_init(&acpi_gpio->deferred_req_irqs_list_entry);
Side note. This trick I start seeing more often, perhaps in the future
something like generic macro will be available.
> + mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
> +/* Run deferred acpi_gpiochip_request_interrupts() */
> +static int acpi_gpio_handle_deferred_request_interrupts(void)
> {
> + struct acpi_gpio_chip *acpi_gpio, *tmp;
> +
> + mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
> + list_for_each_entry_safe(acpi_gpio, tmp,
> + &acpi_gpio_deferred_req_irqs_list,
> + deferred_req_irqs_list_entry) {
> + acpi_handle handle;
> + handle = ACPI_HANDLE(acpi_gpio->chip->parent);
> + if (handle)
Since you are not checking return code the below is NULL aware for
handle parameter.
> + acpi_walk_resources(handle, "_AEI",
> + acpi_gpiochip_request_interr
> upt,
> + acpi_gpio);
> +
> + list_del_init(&acpi_gpio->deferred_req_irqs_list_entry);
> }
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
