| Message ID | 20200828145744.3636-1-krzk@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7d50f6656dacf085a00beeedbc48b19a37d17881 |
| Headers | show |
| Series | [v2,1/4] Input: ep93xx_keypad - Fix handling of platform_get_irq() error | expand |
On Fri, Aug 28, 2020 at 04:57:41PM +0200, Krzysztof Kozlowski wrote: > platform_get_irq() returns -ERRNO on error. In such case comparison > to 0 would pass the check. > > Fixes: 60214f058f44 ("Input: ep93xx_keypad - update driver to new core support") > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > Changes since v1: > 1. None > --- > drivers/input/keyboard/ep93xx_keypad.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Hi Dmitry, Any comments on the series? Best regards, Krzysztof
Hi Krzysztof, On Fri, Aug 28, 2020 at 04:57:41PM +0200, Krzysztof Kozlowski wrote: > platform_get_irq() returns -ERRNO on error. In such case comparison > to 0 would pass the check. platform_get_irq() is a bit of a mess. Historically we allowed defining interrupt resource with r->start == 0 and for such cases non-OF non-ACPI code will return 0 from platform_get_irq() to indicate that IRQ is not assigned. We either need to stop doing this in platform_get_irq(), or the conditions in this patch and followups should be "irq <= 0" and we need to make sure we do not accidentally return 0 from probe ... Thanks.
On Mon, 14 Sep 2020 at 08:51, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Krzysztof, > > On Fri, Aug 28, 2020 at 04:57:41PM +0200, Krzysztof Kozlowski wrote: > > platform_get_irq() returns -ERRNO on error. In such case comparison > > to 0 would pass the check. > > platform_get_irq() is a bit of a mess. Historically we allowed defining > interrupt resource with r->start == 0 and for such cases non-OF non-ACPI > code will return 0 from platform_get_irq() to indicate that IRQ is not > assigned. > > We either need to stop doing this in platform_get_irq(), or the > conditions in this patch and followups should be "irq <= 0" and we need > to make sure we do not accidentally return 0 from probe ... Hi, It's then contradictory to platform_get_irq documentation which explicitly says - zero will not be returned on error. This was also clarified in commit e330b9a6bb35 ("platform: don't return 0 from platform_get_irq[_byname]() on error"). As I understood the input drivers code, they check for errors so the coe in my patch is correct. Any "<=0" is not correct with current documentation and implementation. Best regards, Krzysztof
On Tue, Sep 15, 2020 at 06:05:22PM +0200, Krzysztof Kozlowski wrote: > On Mon, 14 Sep 2020 at 08:51, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > Hi Krzysztof, > > > > On Fri, Aug 28, 2020 at 04:57:41PM +0200, Krzysztof Kozlowski wrote: > > > platform_get_irq() returns -ERRNO on error. In such case comparison > > > to 0 would pass the check. > > > > platform_get_irq() is a bit of a mess. Historically we allowed defining > > interrupt resource with r->start == 0 and for such cases non-OF non-ACPI > > code will return 0 from platform_get_irq() to indicate that IRQ is not > > assigned. > > > > We either need to stop doing this in platform_get_irq(), or the > > conditions in this patch and followups should be "irq <= 0" and we need > > to make sure we do not accidentally return 0 from probe ... > > Hi, > > It's then contradictory to platform_get_irq documentation which > explicitly says - zero will not be returned on error. This was also > clarified in commit e330b9a6bb35 ("platform: don't return 0 from > platform_get_irq[_byname]() on error"). It was for OF/ACPI, but not for resources described via DEFINE_RES_IRQ or other means. However I see we added a big fat WARN_ON() in case if we end up returning 0 still, so I will be applying your patches. Thanks.
diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c index 7c70492d9d6b..f831f01501d5 100644 --- a/drivers/input/keyboard/ep93xx_keypad.c +++ b/drivers/input/keyboard/ep93xx_keypad.c @@ -250,8 +250,8 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) } keypad->irq = platform_get_irq(pdev, 0); - if (!keypad->irq) { - err = -ENXIO; + if (keypad->irq < 0) { + err = keypad->irq; goto failed_free; }
platform_get_irq() returns -ERRNO on error. In such case comparison to 0 would pass the check. Fixes: 60214f058f44 ("Input: ep93xx_keypad - update driver to new core support") Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- Changes since v1: 1. None --- drivers/input/keyboard/ep93xx_keypad.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)