| Message ID | 20180711202339.16950-2-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib-acpi: make sure we trigger edge events at least once on boot | expand |
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?
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? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 11:43 AM Hans de Goede <hdegoede@redhat.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 12-07-18 11:59, Benjamin Tissoires wrote: > On Thu, Jul 12, 2018 at 11:43 AM Hans de Goede <hdegoede@redhat.com> 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. The late_initcall will run before /sbin/init gets executed, so it will still run in time for systemd to get the correct initial LID state. Regards, Hans > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index e2232cbcec8b..75556b20d98e 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -20,15 +20,20 @@ #include <linux/interrupt.h> #include <linux/mutex.h> #include <linux/pinctrl/pinctrl.h> +#include <linux/workqueue.h> #include "gpiolib.h" +/* Initialization of all builtin drivers may take up to 2.5 seconds */ +#define ACPI_IRQ_INITIAL_SYNC_DELAY msecs_to_jiffies(3000) + struct acpi_gpio_event { struct list_head node; acpi_handle handle; unsigned int pin; unsigned int irq; struct gpio_desc *desc; + struct delayed_work initial_sync_work; }; struct acpi_gpio_connection { @@ -89,6 +94,7 @@ static irqreturn_t acpi_gpio_irq_handler(int irq, void *data) { struct acpi_gpio_event *event = data; + cancel_delayed_work_sync(&event->initial_sync_work); acpi_evaluate_object(event->handle, NULL, NULL, NULL); return IRQ_HANDLED; @@ -125,6 +131,15 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares, } EXPORT_SYMBOL_GPL(acpi_gpio_get_irq_resource); +static void acpi_irq_initial_sync(struct work_struct *work) +{ + struct acpi_gpio_event *event = + container_of(work, struct acpi_gpio_event, + initial_sync_work.work); + + acpi_evaluate_object(event->handle, NULL, NULL, NULL); +} + static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, void *context) { @@ -136,7 +151,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, irq_handler_t handler = NULL; struct gpio_desc *desc; unsigned long irqflags; - int ret, pin, irq; + int ret, pin, irq, value; if (!acpi_gpio_get_irq_resource(ares, &agpio)) return AE_OK; @@ -167,6 +182,8 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, gpiod_direction_input(desc); + value = gpiod_get_value(desc); + ret = gpiochip_lock_as_irq(chip, pin); if (ret) { dev_err(chip->parent, "Failed to lock GPIO as interrupt\n"); @@ -208,6 +225,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, event->irq = irq; event->pin = pin; event->desc = desc; + INIT_DELAYED_WORK(&event->initial_sync_work, acpi_irq_initial_sync); ret = request_threaded_irq(event->irq, NULL, handler, irqflags, "ACPI:Event", event); @@ -222,6 +240,19 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares, enable_irq_wake(irq); list_add_tail(&event->node, &acpi_gpio->events); + + /* + * Make sure we trigger the initial state of the IRQ when using RISING + * or FALLING. Note this uses a delayed work-queue as the AML code + * may refer to OperationRegions from other (builtin) drivers which + * may be probed after us. + */ + if (handler == acpi_gpio_irq_handler && + (((irqflags & IRQF_TRIGGER_RISING) && value == 1) || + ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))) + schedule_delayed_work(&event->initial_sync_work, + ACPI_IRQ_INITIAL_SYNC_DELAY); + return AE_OK; fail_free_event: @@ -298,6 +329,7 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) disable_irq_wake(event->irq); free_irq(event->irq, event); + cancel_delayed_work_sync(&event->initial_sync_work); desc = event->desc; if (WARN_ON(IS_ERR(desc))) continue;