Re: [PATCH] gpiolib-acpi: make sure we trigger edge events at least once on boot

[Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>]

On Thu, Jul 12, 2018 at 11:43 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 12-07-18 11:36, Andy Shevchenko wrote:
> > On Wed, 2018-07-11 at 22:23 +0200, Hans de Goede wrote:
> >
> >> Note that the running of the event-handler is done 3 seconds after the
> >> GPIO driver loads, this is done because the event-handler AML code may
> >> rely on OperationRegions registered by other drivers and the GPIO
> >> driver
> >> is initialized very early on, where as the total init of all drivers
> >> can
> >> take up to 2.5 seconds. This delay avoid errors like these:
> >>
> >> [    0.133026] ACPI Error: No handler for Region [XSCG]
> >> ((____ptrval____)) [GenericSerialBus] (20180531/evregion-132)
> >> [    0.133036] ACPI Error: Region GenericSerialBus (ID=9) has no
> >> handler (20180531/exfldio-265)
> >> [    0.133046] ACPI Error: Method parse/execution failed
> >> \_SB.GPO2._E12, AE_NOT_EXIST (20180531/psparse-516)
> >
> >
> >> +/* Initialization of all builtin drivers may take up to 2.5 seconds
> >> */
> >
> > Ouch, this sounds quite fragile. On your case it's 2.5s, on someone's
> > else it might take 5 or more?
> >
> > Maybe we could go other way around, i.e. if the driver in question needs
> > to be loaded and requires this W/A, it can notify somehow GPIO ACPI to
> > sync the state?
>
> I agree this is fragile, thinking more about this I think it would be better
> to put all events which need an initial sync on an initial sync
> list and go over that list from a late_initcall() handler, that way we
> know for certain that all bultin drivers will be probed when we run the
> event handlers for the initial state sync.
>
> I think that will be a better solution, do you agree?

This would definitely be better from my point of view.

However, I wonder if this won't solve the race with user space. In the
initial writing of this patch, the issue was that the ACPI LID state
was reported to be closed if the GPIO was not read once. And this made
systemd to put the system to sleep ASAP.
If we delay the reading of the GPIO, there is a chance the first boot
would trigger the suspend, though eventually the suspend loop will
end.

Note that this won't apply to the surface 3 anymore (which explains
why I 'forgot' to send this patch) as I used an other way to report
the LID status thanks to an other custom driver.

I do wonder if the DSDT doesn't hint you that the USB controller
depends on the GPIO irq chip, and in that case if we can't detect the
dependency to fire the GPIOs when we can.

I think this would be better, though more complex (fragile then). So
maybe this could be an enhancement to think of in the future. I'd say
use the late_initcall() for now, and think a little bit more if the
DSDT doesn't help us detecting the situation where we need to wait for
the USB chip before triggering the GPIO.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html