Hi,
On 14-08-18 13:02, Andy Shevchenko wrote:
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)
Fixed for v2.
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?
I cannot come up with a specific commit this fixes, this is more of
a generic pre-existing issue then anything introduced by a specific
commit, so no.
+ 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.
We are not deferring the IRQ, we are deferring the _requesting_
of the IRQ, and this fits in 80 chars in most places fine, and
there where it does not dropping the _req is not going to help,
so I would prefer to keep this as is.
+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.)
Sorry, but no / nack, I really dislike constructions where lock and
unlock are not balanced 1:1. I personally really dislike unlock before
return code like you suggest and always suggest people to use a goto
to the actual normal exit path unlock, setting retval before the goto.
+ 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.
Ah, ok I will drop the check for v2.
+ acpi_walk_resources(handle, "_AEI",
+ acpi_gpiochip_request_interr
upt,
+ acpi_gpio);
+
+ list_del_init(&acpi_gpio->deferred_req_irqs_list_entry);
}
Regards,
Hans
