[PATCH] driver core: platform: return -ENXIO for missing GpioInt

[PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Brian Norris]

Commit daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in
platform_get_irq()") broke the Embedded Controller driver on most LPC
Chromebooks (i.e., most x86 Chromebooks), because cros_ec_lpc expects
platform_get_irq() to return -ENXIO for non-existent IRQs.
Unfortunately, acpi_dev_gpio_irq_get() doesn't follow this convention
and returns -ENOENT instead. So we get this error from cros_ec_lpc:

   couldn't retrieve IRQ number (-2)

I see a variety of drivers that treat -ENXIO specially, so rather than
fix all of them, let's fix up the API to restore its previous behavior.

I reported this on v2 of this patch:

https://lore.kernel.org/lkml/20190220180538.GA42642@google.com/

but apparently the patch had already been merged before v3 got sent out:

https://lore.kernel.org/lkml/20190221193429.161300-1-egranata@chromium.org/

and the result is that the bug landed and remains unfixed.

I differ from the v3 patch by:
 * allowing for ret==0, even though acpi_dev_gpio_irq_get() specifically
   documents (and enforces) that 0 is not a valid return value (noted on
   the v3 review)
 * adding a small comment

Reported-by: Brian Norris <briannorris@chromium.org>
Reported-by: Salvatore Bellizzi <salvatore.bellizzi@linux.seppia.net>
Cc: Enrico Granata <egranata@chromium.org>
Cc: <stable@vger.kernel.org>
Fixes: daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in platform_get_irq()")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Side note: it might have helped alleviate some of this pain if there
were email notifications to the mailing list when a patch gets applied.
I didn't realize (and I'm not sure if Enrico did) that v2 was already
merged by the time I noted its mistakes. If I had known, I would have
suggested a follow-up patch, not a v3.

I know some maintainers' "tip bots" do this, but not all apparently.

 drivers/base/platform.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 506a0175a5a7..ec974ba9c0c4 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -157,8 +157,13 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 	 * the device will only expose one IRQ, and this fallback
 	 * allows a common code path across either kind of resource.
 	 */
-	if (num == 0 && has_acpi_companion(&dev->dev))
-		return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
+	if (num == 0 && has_acpi_companion(&dev->dev)) {
+		int ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
+
+		/* Our callers expect -ENXIO for missing IRQs. */
+		if (ret >= 0 || ret == -EPROBE_DEFER)
+			return ret;
+	}
 
 	return -ENXIO;
 #endif
-- 
2.22.0.709.g102302147b-goog

Re: [PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Nathan Chancellor]

On Mon, Jul 29, 2019 at 01:49:54PM -0700, Brian Norris wrote:
> Commit daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in
> platform_get_irq()") broke the Embedded Controller driver on most LPC
> Chromebooks (i.e., most x86 Chromebooks), because cros_ec_lpc expects
> platform_get_irq() to return -ENXIO for non-existent IRQs.
> Unfortunately, acpi_dev_gpio_irq_get() doesn't follow this convention
> and returns -ENOENT instead. So we get this error from cros_ec_lpc:
> 
>    couldn't retrieve IRQ number (-2)
> 
> I see a variety of drivers that treat -ENXIO specially, so rather than
> fix all of them, let's fix up the API to restore its previous behavior.
> 
> I reported this on v2 of this patch:
> 
> https://lore.kernel.org/lkml/20190220180538.GA42642@google.com/
> 
> but apparently the patch had already been merged before v3 got sent out:
> 
> https://lore.kernel.org/lkml/20190221193429.161300-1-egranata@chromium.org/
> 
> and the result is that the bug landed and remains unfixed.
> 
> I differ from the v3 patch by:
>  * allowing for ret==0, even though acpi_dev_gpio_irq_get() specifically
>    documents (and enforces) that 0 is not a valid return value (noted on
>    the v3 review)
>  * adding a small comment
> 
> Reported-by: Brian Norris <briannorris@chromium.org>
> Reported-by: Salvatore Bellizzi <salvatore.bellizzi@linux.seppia.net>
> Cc: Enrico Granata <egranata@chromium.org>
> Cc: <stable@vger.kernel.org>
> Fixes: daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in platform_get_irq()")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Side note: it might have helped alleviate some of this pain if there
> were email notifications to the mailing list when a patch gets applied.
> I didn't realize (and I'm not sure if Enrico did) that v2 was already
> merged by the time I noted its mistakes. If I had known, I would have
> suggested a follow-up patch, not a v3.

I've found this to be fairly reliable for getting notified when
something gets applied if it is a tree that shows up in -next.

https://www.kernel.org/get-notifications-for-your-patches.html

Cheers,
Nathan

Re: [PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Brian Norris]

On Mon, Jul 29, 2019 at 1:54 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Mon, Jul 29, 2019 at 01:49:54PM -0700, Brian Norris wrote:
> > Side note: it might have helped alleviate some of this pain if there
> > were email notifications to the mailing list when a patch gets applied.
> > I didn't realize (and I'm not sure if Enrico did) that v2 was already
> > merged by the time I noted its mistakes. If I had known, I would have
> > suggested a follow-up patch, not a v3.
>
> I've found this to be fairly reliable for getting notified when
> something gets applied if it is a tree that shows up in -next.
>
> https://www.kernel.org/get-notifications-for-your-patches.html

I didn't send the original patch. I was only debugging/reviewing
someone else's patch, and jumped in after its authorship (as it hit
issues in our own CI system). So it was more of a "drive-by" scenario,
and it doesn't sound like that page addresses this situation.

Brian

Re: [PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Enrico Granata]

On Mon, Jul 29, 2019 at 2:03 PM Brian Norris <briannorris@chromium.org> wrote:
>
> On Mon, Jul 29, 2019 at 1:54 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > On Mon, Jul 29, 2019 at 01:49:54PM -0700, Brian Norris wrote:
> > > Side note: it might have helped alleviate some of this pain if there
> > > were email notifications to the mailing list when a patch gets applied.
> > > I didn't realize (and I'm not sure if Enrico did) that v2 was already
> > > merged by the time I noted its mistakes. If I had known, I would have
> > > suggested a follow-up patch, not a v3.
> >
> > I've found this to be fairly reliable for getting notified when
> > something gets applied if it is a tree that shows up in -next.
> >
> > https://www.kernel.org/get-notifications-for-your-patches.html
>
> I didn't send the original patch. I was only debugging/reviewing
> someone else's patch, and jumped in after its authorship (as it hit
> issues in our own CI system). So it was more of a "drive-by" scenario,
> and it doesn't sound like that page addresses this situation.
>
> Brian

Your assessment sounds about right - I suspect the tl;dr is that *I*
(the original patch author) should have subscribed to that bot, so I
could figure out that v2 had already merged, and done the right thing
sending out a follow-up CL once you brought the issue to my attention.

Re: [PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Enrico Granata]

On Mon, Jul 29, 2019 at 1:50 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Commit daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in
> platform_get_irq()") broke the Embedded Controller driver on most LPC
> Chromebooks (i.e., most x86 Chromebooks), because cros_ec_lpc expects
> platform_get_irq() to return -ENXIO for non-existent IRQs.
> Unfortunately, acpi_dev_gpio_irq_get() doesn't follow this convention
> and returns -ENOENT instead. So we get this error from cros_ec_lpc:
>
>    couldn't retrieve IRQ number (-2)
>
> I see a variety of drivers that treat -ENXIO specially, so rather than
> fix all of them, let's fix up the API to restore its previous behavior.
>
> I reported this on v2 of this patch:
>
> https://lore.kernel.org/lkml/20190220180538.GA42642@google.com/
>
> but apparently the patch had already been merged before v3 got sent out:
>
> https://lore.kernel.org/lkml/20190221193429.161300-1-egranata@chromium.org/
>
> and the result is that the bug landed and remains unfixed.
>
> I differ from the v3 patch by:
>  * allowing for ret==0, even though acpi_dev_gpio_irq_get() specifically
>    documents (and enforces) that 0 is not a valid return value (noted on
>    the v3 review)
>  * adding a small comment
>
> Reported-by: Brian Norris <briannorris@chromium.org>
> Reported-by: Salvatore Bellizzi <salvatore.bellizzi@linux.seppia.net>
> Cc: Enrico Granata <egranata@chromium.org>
> Cc: <stable@vger.kernel.org>
> Fixes: daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in platform_get_irq()")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Side note: it might have helped alleviate some of this pain if there
> were email notifications to the mailing list when a patch gets applied.
> I didn't realize (and I'm not sure if Enrico did) that v2 was already
> merged by the time I noted its mistakes. If I had known, I would have
> suggested a follow-up patch, not a v3.
>
> I know some maintainers' "tip bots" do this, but not all apparently.
>
>  drivers/base/platform.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 506a0175a5a7..ec974ba9c0c4 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -157,8 +157,13 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>          * the device will only expose one IRQ, and this fallback
>          * allows a common code path across either kind of resource.
>          */
> -       if (num == 0 && has_acpi_companion(&dev->dev))
> -               return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +       if (num == 0 && has_acpi_companion(&dev->dev)) {
> +               int ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +               /* Our callers expect -ENXIO for missing IRQs. */
> +               if (ret >= 0 || ret == -EPROBE_DEFER)
> +                       return ret;
> +       }
>
>         return -ENXIO;
>  #endif
> --
> 2.22.0.709.g102302147b-goog
>

Acked-by: Enrico Granata <egranata@google.com>

Re: [PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Andy Shevchenko]

On Mon, Jul 29, 2019 at 01:49:54PM -0700, Brian Norris wrote:
> Commit daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in
> platform_get_irq()") broke the Embedded Controller driver on most LPC
> Chromebooks (i.e., most x86 Chromebooks), because cros_ec_lpc expects
> platform_get_irq() to return -ENXIO for non-existent IRQs.
> Unfortunately, acpi_dev_gpio_irq_get() doesn't follow this convention
> and returns -ENOENT instead. So we get this error from cros_ec_lpc:
> 
>    couldn't retrieve IRQ number (-2)
> 
> I see a variety of drivers that treat -ENXIO specially, so rather than
> fix all of them, let's fix up the API to restore its previous behavior.
> 
> I reported this on v2 of this patch:
> 
> https://lore.kernel.org/lkml/20190220180538.GA42642@google.com/
> 
> but apparently the patch had already been merged before v3 got sent out:
> 
> https://lore.kernel.org/lkml/20190221193429.161300-1-egranata@chromium.org/
> 
> and the result is that the bug landed and remains unfixed.
> 
> I differ from the v3 patch by:
>  * allowing for ret==0, even though acpi_dev_gpio_irq_get() specifically
>    documents (and enforces) that 0 is not a valid return value (noted on
>    the v3 review)
>  * adding a small comment

Thank you for fixing this.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Reported-by: Brian Norris <briannorris@chromium.org>
> Reported-by: Salvatore Bellizzi <salvatore.bellizzi@linux.seppia.net>
> Cc: Enrico Granata <egranata@chromium.org>
> Cc: <stable@vger.kernel.org>
> Fixes: daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in platform_get_irq()")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Side note: it might have helped alleviate some of this pain if there
> were email notifications to the mailing list when a patch gets applied.
> I didn't realize (and I'm not sure if Enrico did) that v2 was already
> merged by the time I noted its mistakes. If I had known, I would have
> suggested a follow-up patch, not a v3.
> 
> I know some maintainers' "tip bots" do this, but not all apparently.
> 
>  drivers/base/platform.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 506a0175a5a7..ec974ba9c0c4 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -157,8 +157,13 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>  	 * the device will only expose one IRQ, and this fallback
>  	 * allows a common code path across either kind of resource.
>  	 */
> -	if (num == 0 && has_acpi_companion(&dev->dev))
> -		return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +	if (num == 0 && has_acpi_companion(&dev->dev)) {
> +		int ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +		/* Our callers expect -ENXIO for missing IRQs. */
> +		if (ret >= 0 || ret == -EPROBE_DEFER)
> +			return ret;
> +	}
>  
>  	return -ENXIO;
>  #endif
> -- 
> 2.22.0.709.g102302147b-goog
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Greg Kroah-Hartman]

On Mon, Jul 29, 2019 at 01:49:54PM -0700, Brian Norris wrote:
> Commit daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in
> platform_get_irq()") broke the Embedded Controller driver on most LPC
> Chromebooks (i.e., most x86 Chromebooks), because cros_ec_lpc expects
> platform_get_irq() to return -ENXIO for non-existent IRQs.
> Unfortunately, acpi_dev_gpio_irq_get() doesn't follow this convention
> and returns -ENOENT instead. So we get this error from cros_ec_lpc:
> 
>    couldn't retrieve IRQ number (-2)
> 
> I see a variety of drivers that treat -ENXIO specially, so rather than
> fix all of them, let's fix up the API to restore its previous behavior.
> 
> I reported this on v2 of this patch:
> 
> https://lore.kernel.org/lkml/20190220180538.GA42642@google.com/
> 
> but apparently the patch had already been merged before v3 got sent out:
> 
> https://lore.kernel.org/lkml/20190221193429.161300-1-egranata@chromium.org/
> 
> and the result is that the bug landed and remains unfixed.
> 
> I differ from the v3 patch by:
>  * allowing for ret==0, even though acpi_dev_gpio_irq_get() specifically
>    documents (and enforces) that 0 is not a valid return value (noted on
>    the v3 review)
>  * adding a small comment
> 
> Reported-by: Brian Norris <briannorris@chromium.org>
> Reported-by: Salvatore Bellizzi <salvatore.bellizzi@linux.seppia.net>
> Cc: Enrico Granata <egranata@chromium.org>
> Cc: <stable@vger.kernel.org>
> Fixes: daaef255dc96 ("driver: platform: Support parsing GpioInt 0 in platform_get_irq()")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Enrico Granata <egranata@google.com>
> ---
> Side note: it might have helped alleviate some of this pain if there
> were email notifications to the mailing list when a patch gets applied.
> I didn't realize (and I'm not sure if Enrico did) that v2 was already
> merged by the time I noted its mistakes. If I had known, I would have
> suggested a follow-up patch, not a v3.
> 
> I know some maintainers' "tip bots" do this, but not all apparently.

We can't drown out mailing list traffic with a ton of "this patch was
applied" emails.  We send them directly to the people involved in it.

Note, you can always set up your own "watch" for stuff like this by
pulling linux-next every day and sending yourself any new patches that
get applied for any specific files/directories you are concerned about.

thanks,

greg k-h

Re: [PATCH] driver core: platform: return -ENXIO for missing GpioInt

[Brian Norris]

On Mon, Jul 29, 2019 at 1:50 PM Brian Norris <briannorris@chromium.org> wrote:
> Side note: it might have helped alleviate some of this pain if there
> were email notifications to the mailing list when a patch gets applied.
> I didn't realize (and I'm not sure if Enrico did) that v2 was already
> merged by the time I noted its mistakes. If I had known, I would have
> suggested a follow-up patch, not a v3.

I guess I'll be the bot this time: 'twas applied by Greg on Tuesday,
July 30 UTC-07:00.

Thanks,
Brian