| Message ID | 20201106192304.49179-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: acpi: pin configuration fixes | expand |
Hi, On 11/6/20 8:22 PM, Andy Shevchenko wrote: > We didn't take into account the debounce settings supplied by ACPI. > This change is targeting the mentioned gap. > > Reported-by: Coiby Xu <coiby.xu@gmail.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> I added an older version of this (which only modified acpi_dev_gpio_irq_get()) for testing and when booting a kernel with that version applied to it, on a Cherry Trail device I noticed that a whole bunch of devices where no longer seen by the kernel because of acpi_dev_gpio_irq_get() returning errors now (-ENOTSUPP). Quoting from the gpiod_set_debounce docs: /** * gpiod_set_debounce - sets @debounce time for a GPIO * @desc: descriptor of the GPIO for which to set debounce time * @debounce: debounce time in microseconds * * Returns: * 0 on success, %-ENOTSUPP if the controller doesn't support setting the * debounce time. */ This is expected on GPIO chips where setting the debounce time is not supported. So the error handling should be modified to ignore -ENOTSUPP errors here. This certainly MUST NOT be merged as is because it breaks a lot of things as is. Regards, Hans > --- > drivers/gpio/gpiolib-acpi.c | 18 ++++++++++++++++++ > drivers/gpio/gpiolib-acpi.h | 2 ++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index c127b410a7a2..b4a0decfeac2 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -299,6 +299,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares, > return AE_OK; > } > > + ret = gpiod_set_debounce(desc, agpio->debounce_timeout); > + if (ret) > + goto fail_free_desc; > + > ret = gpiochip_lock_as_irq(chip, pin); > if (ret) { > dev_err(chip->parent, > @@ -664,6 +668,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data) > lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr, > agpio->pin_table[pin_index]); > lookup->info.pin_config = agpio->pin_config; > + lookup->info.debounce = agpio->debounce_timeout; > lookup->info.gpioint = gpioint; > > /* > @@ -961,6 +966,10 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) > if (ret < 0) > return ret; > > + ret = gpiod_set_debounce(desc, info.debounce); > + if (ret) > + return ret; > + > irq_flags = acpi_dev_get_irq_type(info.triggering, > info.polarity); > > @@ -1048,6 +1057,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, > if (!found) { > enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio); > const char *label = "ACPI:OpRegion"; > + int ret; > > desc = gpiochip_request_own_desc(chip, pin, label, > GPIO_ACTIVE_HIGH, > @@ -1058,6 +1068,14 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, > goto out; > } > > + ret = gpiod_set_debounce(desc, agpio->debounce_timeout); > + if (ret) { > + status = AE_ERROR; > + gpiochip_free_own_desc(desc); > + mutex_unlock(&achip->conn_lock); > + goto out; > + } > + > conn = kzalloc(sizeof(*conn), GFP_KERNEL); > if (!conn) { > status = AE_NO_MEMORY; > diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h > index 1c6d65cf0629..e2edb632b2cc 100644 > --- a/drivers/gpio/gpiolib-acpi.h > +++ b/drivers/gpio/gpiolib-acpi.h > @@ -18,6 +18,7 @@ struct acpi_device; > * @pin_config: pin bias as provided by ACPI > * @polarity: interrupt polarity as provided by ACPI > * @triggering: triggering type as provided by ACPI > + * @debounce: debounce timeout as provided by ACPI > * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping > */ > struct acpi_gpio_info { > @@ -27,6 +28,7 @@ struct acpi_gpio_info { > int pin_config; > int polarity; > int triggering; > + unsigned int debounce; > unsigned int quirks; > }; > >
On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/6/20 8:22 PM, Andy Shevchenko wrote: ... > I added an older version of this (which only modified acpi_dev_gpio_irq_get()) > for testing and when booting a kernel with that version applied to it, > on a Cherry Trail device I noticed that a whole bunch of devices where no > longer seen by the kernel because of acpi_dev_gpio_irq_get() returning > errors now (-ENOTSUPP). > > Quoting from the gpiod_set_debounce docs: > > /** > * gpiod_set_debounce - sets @debounce time for a GPIO > * @desc: descriptor of the GPIO for which to set debounce time > * @debounce: debounce time in microseconds > * > * Returns: > * 0 on success, %-ENOTSUPP if the controller doesn't support setting the > * debounce time. > */ > > This is expected on GPIO chips where setting the debounce time > is not supported. So the error handling should be modified to > ignore -ENOTSUPP errors here. > > This certainly MUST NOT be merged as is because it breaks a lot > of things as is. Thank you very much for the testing! I remember that I fixed debounce for BayTrail, but it seems I have yet to fix Cherry Trail pin control as a prerequisite to this patch. And like I said this series is definitely not for backporting.
Hi, On 11/7/20 4:26 PM, Andy Shevchenko wrote: > On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 11/6/20 8:22 PM, Andy Shevchenko wrote: > > ... > >> I added an older version of this (which only modified acpi_dev_gpio_irq_get()) >> for testing and when booting a kernel with that version applied to it, >> on a Cherry Trail device I noticed that a whole bunch of devices where no >> longer seen by the kernel because of acpi_dev_gpio_irq_get() returning >> errors now (-ENOTSUPP). >> >> Quoting from the gpiod_set_debounce docs: >> >> /** >> * gpiod_set_debounce - sets @debounce time for a GPIO >> * @desc: descriptor of the GPIO for which to set debounce time >> * @debounce: debounce time in microseconds >> * >> * Returns: >> * 0 on success, %-ENOTSUPP if the controller doesn't support setting the >> * debounce time. >> */ >> >> This is expected on GPIO chips where setting the debounce time >> is not supported. So the error handling should be modified to >> ignore -ENOTSUPP errors here. >> >> This certainly MUST NOT be merged as is because it breaks a lot >> of things as is. > > Thank you very much for the testing! I remember that I fixed debounce > for BayTrail, but it seems I have yet to fix Cherry Trail pin control > as a prerequisite to this patch. > > And like I said this series is definitely not for backporting. Independent of fixing the CherryTrail pinctrl driver to support this, I strongly believe that -ENOTSUPP should be ignored (treated as success) by this patch. Remember ACPI is not only used on x86 but also on ARM now a days. We simply cannot guarantee that all pinctrls will support (let alone implement) debounce settings. E.g. I'm pretty sure that the pinctrl on the popular Allwinner A64 does not support debouncing and there are builts using a combination of uboot + EDK2 to boot! The documentation for gpiod_set_debounce even explicitly mentioned that -ENOTSUPP is an error which one may expect (and thus treat specially). The same goes for the bias stuff too. Regards, Hans
Hi, On 11/7/20 4:26 PM, Andy Shevchenko wrote: > On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 11/6/20 8:22 PM, Andy Shevchenko wrote: > > ... > >> I added an older version of this (which only modified acpi_dev_gpio_irq_get()) >> for testing and when booting a kernel with that version applied to it, >> on a Cherry Trail device I noticed that a whole bunch of devices where no >> longer seen by the kernel because of acpi_dev_gpio_irq_get() returning >> errors now (-ENOTSUPP). >> >> Quoting from the gpiod_set_debounce docs: >> >> /** >> * gpiod_set_debounce - sets @debounce time for a GPIO >> * @desc: descriptor of the GPIO for which to set debounce time >> * @debounce: debounce time in microseconds >> * >> * Returns: >> * 0 on success, %-ENOTSUPP if the controller doesn't support setting the >> * debounce time. >> */ >> >> This is expected on GPIO chips where setting the debounce time >> is not supported. So the error handling should be modified to >> ignore -ENOTSUPP errors here. >> >> This certainly MUST NOT be merged as is because it breaks a lot >> of things as is. > > Thank you very much for the testing! I remember that I fixed debounce > for BayTrail, but it seems I have yet to fix Cherry Trail pin control > as a prerequisite to this patch. > > And like I said this series is definitely not for backporting. Independent of fixing the CherryTrail pinctrl driver to support this, I strongly believe that -ENOTSUPP should be ignored (treated as success) by this patch. Remember ACPI is not only used on x86 but also on ARM now a days. We simply cannot guarantee that all pinctrls will support (let alone implement) debounce settings. E.g. I'm pretty sure that the pinctrl on the popular Allwinner A64 does not support debouncing and there are builts using a combination of uboot + EDK2 to boot! The documentation for gpiod_set_debounce even explicitly mentioned that -ENOTSUPP is an error which one may expect (and thus treat specially). The same goes for the bias stuff too. Regards, Hans
On Sun, Nov 08, 2020 at 10:31:32AM +0100, Hans de Goede wrote: > On 11/7/20 4:26 PM, Andy Shevchenko wrote: > > On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 11/6/20 8:22 PM, Andy Shevchenko wrote: ... > > Thank you very much for the testing! I remember that I fixed debounce > > for BayTrail, but it seems I have yet to fix Cherry Trail pin control > > as a prerequisite to this patch. > > > > And like I said this series is definitely not for backporting. > > Independent of fixing the CherryTrail pinctrl driver to support this, > I strongly believe that -ENOTSUPP should be ignored (treated as success) > by this patch. Remember ACPI is not only used on x86 but also on ARM > now a days. We simply cannot guarantee that all pinctrls will support > (let alone implement) debounce settings. E.g. I'm pretty sure that > the pinctrl on the popular Allwinner A64 does not support debouncing > and there are builts using a combination of uboot + EDK2 to boot! > > The documentation for gpiod_set_debounce even explicitly mentioned that > -ENOTSUPP is an error which one may expect (and thus treat specially). > > The same goes for the bias stuff too. While for debounce I absolutely agree with you I don't think it applies to bias. ACPI table is coupled with a platform and setting bias == !PullNone implies that bias is supported. If we break something with this it means: - ACPI table is broken and we need a quirk - GPIO library is broken on architectural level and needs not to return ENOTSUPP for the flags configuration. I will update this with taking debounce optional support into account.
Hi, On 11/9/20 12:45 PM, Andy Shevchenko wrote: > On Sun, Nov 08, 2020 at 10:31:32AM +0100, Hans de Goede wrote: >> On 11/7/20 4:26 PM, Andy Shevchenko wrote: >>> On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> On 11/6/20 8:22 PM, Andy Shevchenko wrote: > > ... > >>> Thank you very much for the testing! I remember that I fixed debounce >>> for BayTrail, but it seems I have yet to fix Cherry Trail pin control >>> as a prerequisite to this patch. >>> >>> And like I said this series is definitely not for backporting. >> >> Independent of fixing the CherryTrail pinctrl driver to support this, >> I strongly believe that -ENOTSUPP should be ignored (treated as success) >> by this patch. Remember ACPI is not only used on x86 but also on ARM >> now a days. We simply cannot guarantee that all pinctrls will support >> (let alone implement) debounce settings. E.g. I'm pretty sure that >> the pinctrl on the popular Allwinner A64 does not support debouncing >> and there are builts using a combination of uboot + EDK2 to boot! >> >> The documentation for gpiod_set_debounce even explicitly mentioned that >> -ENOTSUPP is an error which one may expect (and thus treat specially). >> >> The same goes for the bias stuff too. > > While for debounce I absolutely agree with you I don't think it applies to > bias. ACPI table is coupled with a platform and setting bias == !PullNone > implies that bias is supported. What about PullDefault ? I can easily see DSDT writers using PullDefault on platforms where bias setting is not supported. > If we break something with this it means: > - ACPI table is broken and we need a quirk Broken ACPI tables are as common as rain in the Netherlands, where ever possible we want to deal with these / workaround the brokenness without requiring per device quirks. Requiring a per device quirk for every broken ACPI table out there does not scale. > - GPIO library is broken on architectural level and needs not to return > ENOTSUPP for the flags configuration. Usually we handle features not being implemented gracefully. E.g chances are good that whatever bias is required was already setup by the firmware (or bootloader in the uboot + EDK2 case). Making bias set failures a critical error will likely regress working platforms in various cases. Keep in mind we have an existing userbase where things is working fine without taking the bias settings into account now. So if we hit the -ENOTSUPP case on any platform out there, then that is a regression plain and simple and as you know regressions are a big red flag. Since it is really easy to avoid the regression here we really should avoid it IMHO. What about just printing a warning on ENOTSUPP when setting the bias instead ? Regards, Hans
+Cc: my work address back (by some reason Gmail(?) dropped it) On Mon, Nov 9, 2020 at 1:53 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/9/20 12:45 PM, Andy Shevchenko wrote: > > On Sun, Nov 08, 2020 at 10:31:32AM +0100, Hans de Goede wrote: > >> On 11/7/20 4:26 PM, Andy Shevchenko wrote: > >>> On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@redhat.com> wrote: > >>>> On 11/6/20 8:22 PM, Andy Shevchenko wrote: > > > > ... > > > >>> Thank you very much for the testing! I remember that I fixed debounce > >>> for BayTrail, but it seems I have yet to fix Cherry Trail pin control > >>> as a prerequisite to this patch. > >>> > >>> And like I said this series is definitely not for backporting. > >> > >> Independent of fixing the CherryTrail pinctrl driver to support this, > >> I strongly believe that -ENOTSUPP should be ignored (treated as success) > >> by this patch. Remember ACPI is not only used on x86 but also on ARM > >> now a days. We simply cannot guarantee that all pinctrls will support > >> (let alone implement) debounce settings. E.g. I'm pretty sure that > >> the pinctrl on the popular Allwinner A64 does not support debouncing > >> and there are builts using a combination of uboot + EDK2 to boot! > >> > >> The documentation for gpiod_set_debounce even explicitly mentioned that > >> -ENOTSUPP is an error which one may expect (and thus treat specially). > >> > >> The same goes for the bias stuff too. > > > > While for debounce I absolutely agree with you I don't think it applies to > > bias. ACPI table is coupled with a platform and setting bias == !PullNone s/None/Default/ > > implies that bias is supported. > > What about PullDefault ? I can easily see DSDT writers using PullDefault > on platforms where bias setting is not supported. PullDefault is ASIS in terms of BIAS, it's always supported. I mistakenly took None in the upper paragraph, but I got your point. > > If we break something with this it means: > > - ACPI table is broken and we need a quirk > > Broken ACPI tables are as common as rain in the Netherlands, where ever > possible we want to deal with these / workaround the brokenness > without requiring per device quirks. Requiring a per device quirk for > every broken ACPI table out there does not scale. Okay. > > - GPIO library is broken on architectural level and needs not to return > > ENOTSUPP for the flags configuration. > > Usually we handle features not being implemented gracefully. E.g chances > are good that whatever bias is required was already setup by the firmware > (or bootloader in the uboot + EDK2 case). Making bias set failures a > critical error will likely regress working platforms in various cases. > > Keep in mind we have an existing userbase where things is working fine > without taking the bias settings into account now. So if we hit the > -ENOTSUPP case on any platform out there, then that is a regression > plain and simple and as you know regressions are a big red flag. Yeah, probably the best is to have these things optional and thus return only real error cases. > Since it is really easy to avoid the regression here we really > should avoid it IMHO. What about just printing a warning on ENOTSUPP > when setting the bias instead ? So, looking into this deeper I have got the following: FUNC Relation to ENOTSUPP gpiod_set_config() returns if not supported gpiod_set_debounce() as gpiod_set_config() above gpio_set_debounce() legacy wrapper on top of gpiod_set_debounce() gpiod_set_transitory() skips it (returns okay) with debug message gpio_set_config() returns if not supported gpio_set_bias() skips it (returns okay) Now, what the heck are the last ones? Why do we have this difference? (I meant APIs) So, I will try to unify it. Not sure that we need to issue a message in case something is not supported. Debug one? Might be, but don't think we need it right now.
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index c127b410a7a2..b4a0decfeac2 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -299,6 +299,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares, return AE_OK; } + ret = gpiod_set_debounce(desc, agpio->debounce_timeout); + if (ret) + goto fail_free_desc; + ret = gpiochip_lock_as_irq(chip, pin); if (ret) { dev_err(chip->parent, @@ -664,6 +668,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data) lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr, agpio->pin_table[pin_index]); lookup->info.pin_config = agpio->pin_config; + lookup->info.debounce = agpio->debounce_timeout; lookup->info.gpioint = gpioint; /* @@ -961,6 +966,10 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) if (ret < 0) return ret; + ret = gpiod_set_debounce(desc, info.debounce); + if (ret) + return ret; + irq_flags = acpi_dev_get_irq_type(info.triggering, info.polarity); @@ -1048,6 +1057,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, if (!found) { enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio); const char *label = "ACPI:OpRegion"; + int ret; desc = gpiochip_request_own_desc(chip, pin, label, GPIO_ACTIVE_HIGH, @@ -1058,6 +1068,14 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, goto out; } + ret = gpiod_set_debounce(desc, agpio->debounce_timeout); + if (ret) { + status = AE_ERROR; + gpiochip_free_own_desc(desc); + mutex_unlock(&achip->conn_lock); + goto out; + } + conn = kzalloc(sizeof(*conn), GFP_KERNEL); if (!conn) { status = AE_NO_MEMORY; diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h index 1c6d65cf0629..e2edb632b2cc 100644 --- a/drivers/gpio/gpiolib-acpi.h +++ b/drivers/gpio/gpiolib-acpi.h @@ -18,6 +18,7 @@ struct acpi_device; * @pin_config: pin bias as provided by ACPI * @polarity: interrupt polarity as provided by ACPI * @triggering: triggering type as provided by ACPI + * @debounce: debounce timeout as provided by ACPI * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping */ struct acpi_gpio_info { @@ -27,6 +28,7 @@ struct acpi_gpio_info { int pin_config; int polarity; int triggering; + unsigned int debounce; unsigned int quirks; };