| Message ID | 20211122220416.11241-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | gpiolib: acpi: Do not set the IRQ type if the IRQ is already in use | expand |
On Mon, Nov 22, 2021 at 11:04:16PM +0100, Hans de Goede wrote: > If the IRQ is already in use, then acpi_dev_gpio_irq_get_by() really > should not change the type underneath the current owner. > > I specifically hit an issue with this an a Chuwi Hi8 Super (CWI509) Bay > Trail tablet, when the Boot OS selection in the BIOS is set to Android. > In this case _STA for a MAX17047 ACPI I2C device wrongly returns 0xf and > the _CRS resources for this device include a GpioInt pointing to a GPIO > already in use by an _AEI handler, with a different type then specified > in the _CRS for the MAX17047 device. Leading to the acpi_dev_gpio_irq_get() > call done by the i2c-core-acpi.c code changing the type breaking the > _AEI handler. > > Now this clearly is a bug in the DSDT of this tablet (in Android mode), > but in general calling irq_set_irq_type() on an IRQ which already is > in use seems like a bad idea. I'm fine with the change, one comment below, though. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpio/gpiolib-acpi.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index 985e8589c58b..24cd86bf2c4c 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -1056,8 +1056,12 @@ int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int ind > irq_flags = acpi_dev_get_irq_type(info.triggering, > info.polarity); > > - /* Set type if specified and different than the current one */ > - if (irq_flags != IRQ_TYPE_NONE && > + /* > + * If the IRQ is not already in use then set type > + * if specified and different than the current one. > + */ > + if (can_request_irq(irq, irq_flags) && > + irq_flags != IRQ_TYPE_NONE && > irq_flags != irq_get_trigger_type(irq)) > irq_set_irq_type(irq, irq_flags); What about issuing a debug (?) message if (can_request_irq(irq, irq_flags) { if (irq_flags != IRQ_TYPE_NONE && irq_flags != irq_get_trigger_type(irq)) irq_set_irq_type(irq, irq_flags); } else { dev_dbg(..., FW_BUG "IRQ %d already in use\n", irq); } ?
Hi, On 11/23/21 10:36, Andy Shevchenko wrote: > On Mon, Nov 22, 2021 at 11:04:16PM +0100, Hans de Goede wrote: >> If the IRQ is already in use, then acpi_dev_gpio_irq_get_by() really >> should not change the type underneath the current owner. >> >> I specifically hit an issue with this an a Chuwi Hi8 Super (CWI509) Bay >> Trail tablet, when the Boot OS selection in the BIOS is set to Android. >> In this case _STA for a MAX17047 ACPI I2C device wrongly returns 0xf and >> the _CRS resources for this device include a GpioInt pointing to a GPIO >> already in use by an _AEI handler, with a different type then specified >> in the _CRS for the MAX17047 device. Leading to the acpi_dev_gpio_irq_get() >> call done by the i2c-core-acpi.c code changing the type breaking the >> _AEI handler. >> >> Now this clearly is a bug in the DSDT of this tablet (in Android mode), >> but in general calling irq_set_irq_type() on an IRQ which already is >> in use seems like a bad idea. > > I'm fine with the change, one comment below, though. > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpio/gpiolib-acpi.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c >> index 985e8589c58b..24cd86bf2c4c 100644 >> --- a/drivers/gpio/gpiolib-acpi.c >> +++ b/drivers/gpio/gpiolib-acpi.c >> @@ -1056,8 +1056,12 @@ int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int ind >> irq_flags = acpi_dev_get_irq_type(info.triggering, >> info.polarity); >> >> - /* Set type if specified and different than the current one */ >> - if (irq_flags != IRQ_TYPE_NONE && >> + /* >> + * If the IRQ is not already in use then set type >> + * if specified and different than the current one. >> + */ >> + if (can_request_irq(irq, irq_flags) && >> + irq_flags != IRQ_TYPE_NONE && >> irq_flags != irq_get_trigger_type(irq)) >> irq_set_irq_type(irq, irq_flags); > > What about issuing a debug (?) message > > if (can_request_irq(irq, irq_flags) { > if (irq_flags != IRQ_TYPE_NONE && > irq_flags != irq_get_trigger_type(irq)) > irq_set_irq_type(irq, irq_flags); > } else { > dev_dbg(..., FW_BUG "IRQ %d already in use\n", irq); > } > > ? That is a good idea, I would even be fine with making it a dev_warn, because it really is a FW_BUG if we get here. If we turn out to hit this too much we can always lower the log level later. Shall I submit a v2 with your suggestion, but then using a dev_warn ? Regards, Hans
On Tue, Nov 23, 2021 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/23/21 10:36, Andy Shevchenko wrote: > > On Mon, Nov 22, 2021 at 11:04:16PM +0100, Hans de Goede wrote: > >> If the IRQ is already in use, then acpi_dev_gpio_irq_get_by() really > >> should not change the type underneath the current owner. > >> > >> I specifically hit an issue with this an a Chuwi Hi8 Super (CWI509) Bay > >> Trail tablet, when the Boot OS selection in the BIOS is set to Android. > >> In this case _STA for a MAX17047 ACPI I2C device wrongly returns 0xf and > >> the _CRS resources for this device include a GpioInt pointing to a GPIO > >> already in use by an _AEI handler, with a different type then specified > >> in the _CRS for the MAX17047 device. Leading to the acpi_dev_gpio_irq_get() > >> call done by the i2c-core-acpi.c code changing the type breaking the > >> _AEI handler. > >> > >> Now this clearly is a bug in the DSDT of this tablet (in Android mode), > >> but in general calling irq_set_irq_type() on an IRQ which already is > >> in use seems like a bad idea. > > > > I'm fine with the change, one comment below, though. > > > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/gpio/gpiolib-acpi.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > >> index 985e8589c58b..24cd86bf2c4c 100644 > >> --- a/drivers/gpio/gpiolib-acpi.c > >> +++ b/drivers/gpio/gpiolib-acpi.c > >> @@ -1056,8 +1056,12 @@ int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int ind > >> irq_flags = acpi_dev_get_irq_type(info.triggering, > >> info.polarity); > >> > >> - /* Set type if specified and different than the current one */ > >> - if (irq_flags != IRQ_TYPE_NONE && > >> + /* > >> + * If the IRQ is not already in use then set type > >> + * if specified and different than the current one. > >> + */ > >> + if (can_request_irq(irq, irq_flags) && > >> + irq_flags != IRQ_TYPE_NONE && > >> irq_flags != irq_get_trigger_type(irq)) > >> irq_set_irq_type(irq, irq_flags); > > > > What about issuing a debug (?) message > > > > if (can_request_irq(irq, irq_flags) { > > if (irq_flags != IRQ_TYPE_NONE && > > irq_flags != irq_get_trigger_type(irq)) > > irq_set_irq_type(irq, irq_flags); > > } else { > > dev_dbg(..., FW_BUG "IRQ %d already in use\n", irq); > > } > > > > ? > > That is a good idea, I would even be fine with making it a dev_warn, because it > really is a FW_BUG if we get here. If we turn out to hit this too much we > can always lower the log level later. > > Shall I submit a v2 with your suggestion, but then using a dev_warn ? Please do, it will be slightly easier for me.
Hi, On 11/23/21 12:01, Andy Shevchenko wrote: > On Tue, Nov 23, 2021 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 11/23/21 10:36, Andy Shevchenko wrote: >>> On Mon, Nov 22, 2021 at 11:04:16PM +0100, Hans de Goede wrote: >>>> If the IRQ is already in use, then acpi_dev_gpio_irq_get_by() really >>>> should not change the type underneath the current owner. >>>> >>>> I specifically hit an issue with this an a Chuwi Hi8 Super (CWI509) Bay >>>> Trail tablet, when the Boot OS selection in the BIOS is set to Android. >>>> In this case _STA for a MAX17047 ACPI I2C device wrongly returns 0xf and >>>> the _CRS resources for this device include a GpioInt pointing to a GPIO >>>> already in use by an _AEI handler, with a different type then specified >>>> in the _CRS for the MAX17047 device. Leading to the acpi_dev_gpio_irq_get() >>>> call done by the i2c-core-acpi.c code changing the type breaking the >>>> _AEI handler. >>>> >>>> Now this clearly is a bug in the DSDT of this tablet (in Android mode), >>>> but in general calling irq_set_irq_type() on an IRQ which already is >>>> in use seems like a bad idea. >>> >>> I'm fine with the change, one comment below, though. >>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/gpio/gpiolib-acpi.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c >>>> index 985e8589c58b..24cd86bf2c4c 100644 >>>> --- a/drivers/gpio/gpiolib-acpi.c >>>> +++ b/drivers/gpio/gpiolib-acpi.c >>>> @@ -1056,8 +1056,12 @@ int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int ind >>>> irq_flags = acpi_dev_get_irq_type(info.triggering, >>>> info.polarity); >>>> >>>> - /* Set type if specified and different than the current one */ >>>> - if (irq_flags != IRQ_TYPE_NONE && >>>> + /* >>>> + * If the IRQ is not already in use then set type >>>> + * if specified and different than the current one. >>>> + */ >>>> + if (can_request_irq(irq, irq_flags) && >>>> + irq_flags != IRQ_TYPE_NONE && >>>> irq_flags != irq_get_trigger_type(irq)) >>>> irq_set_irq_type(irq, irq_flags); >>> >>> What about issuing a debug (?) message >>> >>> if (can_request_irq(irq, irq_flags) { >>> if (irq_flags != IRQ_TYPE_NONE && >>> irq_flags != irq_get_trigger_type(irq)) >>> irq_set_irq_type(irq, irq_flags); >>> } else { >>> dev_dbg(..., FW_BUG "IRQ %d already in use\n", irq); >>> } >>> >>> ? >> >> That is a good idea, I would even be fine with making it a dev_warn, because it >> really is a FW_BUG if we get here. If we turn out to hit this too much we >> can always lower the log level later. >> >> Shall I submit a v2 with your suggestion, but then using a dev_warn ? > > Please do, it will be slightly easier for me. Ok, I've just send out a v2. Note I forgot to add -v2 to format-patch so the subject does not say it is v2, sorry. Regards, Hans
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 985e8589c58b..24cd86bf2c4c 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -1056,8 +1056,12 @@ int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int ind irq_flags = acpi_dev_get_irq_type(info.triggering, info.polarity); - /* Set type if specified and different than the current one */ - if (irq_flags != IRQ_TYPE_NONE && + /* + * If the IRQ is not already in use then set type + * if specified and different than the current one. + */ + if (can_request_irq(irq, irq_flags) && + irq_flags != IRQ_TYPE_NONE && irq_flags != irq_get_trigger_type(irq)) irq_set_irq_type(irq, irq_flags);
If the IRQ is already in use, then acpi_dev_gpio_irq_get_by() really should not change the type underneath the current owner. I specifically hit an issue with this an a Chuwi Hi8 Super (CWI509) Bay Trail tablet, when the Boot OS selection in the BIOS is set to Android. In this case _STA for a MAX17047 ACPI I2C device wrongly returns 0xf and the _CRS resources for this device include a GpioInt pointing to a GPIO already in use by an _AEI handler, with a different type then specified in the _CRS for the MAX17047 device. Leading to the acpi_dev_gpio_irq_get() call done by the i2c-core-acpi.c code changing the type breaking the _AEI handler. Now this clearly is a bug in the DSDT of this tablet (in Android mode), but in general calling irq_set_irq_type() on an IRQ which already is in use seems like a bad idea. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpio/gpiolib-acpi.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)