Re: [PATCH] gpiolib-acpi: Register GpioInt ACPI event handlers from a late_initcall

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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