Interrupt number 0 (returned by platform_get_irq()) might be a valid IRQ so do not treat it as an error. If interrupt 0 was configured, the driver would exit the probe early, before finishing initialization, but with 0-exit status.
Reported-by: Dan Carpenter dan.carpenter@oracle.com Cc: stable@vger.kernel.org Fixes: e0d1ec97853f ("i2c-s3c2410: Change IRQ to be plain integer.") Signed-off-by: Krzysztof Kozlowski krzk@kernel.org --- drivers/i2c/busses/i2c-s3c2410.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index 5d97510ee48b..783a93404f47 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -1178,7 +1178,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev) */ if (!(i2c->quirks & QUIRK_POLL)) { i2c->irq = ret = platform_get_irq(pdev, 0); - if (ret <= 0) { + if (ret < 0) { dev_err(&pdev->dev, "cannot find IRQ\n"); clk_unprepare(i2c->clk); return ret;
On Thu, Mar 01, 2018 at 09:34:44PM +0100, Krzysztof Kozlowski wrote:
Interrupt number 0 (returned by platform_get_irq()) might be a valid IRQ so do not treat it as an error. If interrupt 0 was configured, the driver would exit the probe early, before finishing initialization, but with 0-exit status.
The official position (as stated by Linus) is that interrupt zero is not a valid interrupt for peripheral drivers (it may be valid within architecture code for things like the x86 PIT, but nothing else.)
You need to number your platform interrupts from one rather than zero.
Note that there have been patches proposed to make platform_get_irq() return an error rather than returning a value of zero, so changing the driver in this way is not a good idea.
On Fri, Mar 02, 2018 at 11:19:31AM +0000, Russell King - ARM Linux wrote:
On Thu, Mar 01, 2018 at 09:34:44PM +0100, Krzysztof Kozlowski wrote:
Interrupt number 0 (returned by platform_get_irq()) might be a valid IRQ so do not treat it as an error. If interrupt 0 was configured, the driver would exit the probe early, before finishing initialization, but with 0-exit status.
The official position (as stated by Linus) is that interrupt zero is not a valid interrupt for peripheral drivers (it may be valid within architecture code for things like the x86 PIT, but nothing else.)
You need to number your platform interrupts from one rather than zero.
Note that there have been patches proposed to make platform_get_irq() return an error rather than returning a value of zero, so changing the driver in this way is not a good idea.
Those patches to make platform_get_irq() return error codes were merged 12 years ago in commit 305b3228f9ff ("[PATCH] driver core: platform_get_irq*(): return -ENXIO on error").
This patch just drops the check for zero which is should be fine.
regards, dan carpenter
On Fri, Mar 02, 2018 at 02:49:09PM +0300, Dan Carpenter wrote:
On Fri, Mar 02, 2018 at 11:19:31AM +0000, Russell King - ARM Linux wrote:
On Thu, Mar 01, 2018 at 09:34:44PM +0100, Krzysztof Kozlowski wrote:
Interrupt number 0 (returned by platform_get_irq()) might be a valid IRQ so do not treat it as an error. If interrupt 0 was configured, the driver would exit the probe early, before finishing initialization, but with 0-exit status.
The official position (as stated by Linus) is that interrupt zero is not a valid interrupt for peripheral drivers (it may be valid within architecture code for things like the x86 PIT, but nothing else.)
You need to number your platform interrupts from one rather than zero.
Note that there have been patches proposed to make platform_get_irq() return an error rather than returning a value of zero, so changing the driver in this way is not a good idea.
Those patches to make platform_get_irq() return error codes were merged 12 years ago in commit 305b3228f9ff ("[PATCH] driver core: platform_get_irq*(): return -ENXIO on error").
Rubbish. Please look at the commit you're quoting, it doesn't have much to do with what I'm saying, and I think you're mis-remembering on two counts.
The discussion came up recently (last November) about making platform_get_irq() return an error rather than zero - in other words, it will never return zero. This is entirely different from making platform_get_irq() return -ENXIO when an error occurs (not finding the resource.) This discussion was sparked by patch sets from Arvind Yadav.
Further information can be found by looking up the discussions around killing "NO_IRQ", particularly messages from Linus.
Secondly, the code today does:
int platform_get_irq(struct platform_device *dev, unsigned int num) { ... return r ? r->start : -ENXIO; }
So if the IRQ resource is not found, then yes, it will return -ENXIO. If on the other hand the resource is found, then it will return whatever is found in r->start, which can be zero.
As stated, IRQ 0 shall not be taken by drivers to be a valid interrupt.
Ok, but in that case the original code is still wrong because it returns early with success. I guess it could be changed to:
if (irq <= 0) return -ENXIO;
regards, dan carpenter
On Fri, Mar 02, 2018 at 03:26:56PM +0300, Dan Carpenter wrote:
Ok, but in that case the original code is still wrong because it returns early with success. I guess it could be changed to:
if (irq <= 0) return -ENXIO;
What if platform_get_irq() returns -EPROBE_DEFER or some other error code?
So we're now re-hashing all the discussion from last November.
linux-stable-mirror@lists.linaro.org
-
Dan Carpenter -
Krzysztof Kozlowski -
Russell King - ARM Linux