diff mbox

i2c: s3c2410: Properly handle interrupts of number 0

Message ID 1519936484-23132-1-git-send-email-krzk@kernel.org (mailing list archive)
State Not Applicable
Headers show

Commit Message

Krzysztof Kozlowski March 1, 2018, 8:34 p.m. UTC
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(-)

Comments

Wolfram Sang March 2, 2018, 10:29 a.m. UTC | #1
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.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: stable@vger.kernel.org
> Fixes: e0d1ec97853f ("i2c-s3c2410: Change IRQ to be plain integer.")

Please configure git to use 14 digits here.

> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Applied to for-current, thanks!
Dan Carpenter March 2, 2018, 11 a.m. UTC | #2
On Fri, Mar 02, 2018 at 11:29:26AM +0100, Wolfram Sang 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.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Cc: stable@vger.kernel.org
> > Fixes: e0d1ec97853f ("i2c-s3c2410: Change IRQ to be plain integer.")
> 
> Please configure git to use 14 digits here.

Wait, when did we decide that 12 wasn't enough?

I just did a `git log | grep Fixes | tee baz | head -n 200` and only on
my git tree tehre were only 2 which used exactly 14 digits.  The
standard is 12.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter March 2, 2018, 11:02 a.m. UTC | #3
On Fri, Mar 02, 2018 at 02:00:03PM +0300, Dan Carpenter wrote:
> On Fri, Mar 02, 2018 at 11:29:26AM +0100, Wolfram Sang 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.
> > > 
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Cc: stable@vger.kernel.org
> > > Fixes: e0d1ec97853f ("i2c-s3c2410: Change IRQ to be plain integer.")
> > 
> > Please configure git to use 14 digits here.
> 
> Wait, when did we decide that 12 wasn't enough?
> 
> I just did a `git log | grep Fixes | tee baz | head -n 200` and only on
> my git tree tehre were only 2 which used exactly 14 digits.  The
> standard is 12.

Wow...  I clearly did not read that email before sending it...  :(

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang March 2, 2018, 11:08 a.m. UTC | #4
> Wait, when did we decide that 12 wasn't enough?

I stand corrected. Sorry. I recall some discussion about using 14 and I
changed my git config according to that back then. I see now in
submitting patches that 12 is documented.

I will change my setting back. Sorry for the noise.
Russell King (Oracle) March 2, 2018, 11:19 a.m. UTC | #5
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.
Dan Carpenter March 2, 2018, 11:49 a.m. UTC | #6
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King (Oracle) March 2, 2018, 12:19 p.m. UTC | #7
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.
Dan Carpenter March 2, 2018, 12:26 p.m. UTC | #8
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King (Oracle) March 2, 2018, 12:34 p.m. UTC | #9
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.
Wolfram Sang March 2, 2018, 12:46 p.m. UTC | #10
So, maybe some words why I accepted this patch.

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.)

I am aware of that situation and I totally agree with the reasoning.

> You need to number your platform interrupts from one rather than zero.

Ack.

> 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.

I'd much agree to such an approach, yet I didn't see it coming along so
far for years(?) now.

The reason I applied this patch is consistency with the current
interface. The code is wrong with the way platform_get_irq right now
works. This is independent of platform_get_irq should work, I thought.
This needs to be fixed in a seperate series. This patch will not harm
that transition.
Russell King (Oracle) March 2, 2018, 12:59 p.m. UTC | #11
On Fri, Mar 02, 2018 at 01:46:47PM +0100, Wolfram Sang wrote:
> 
> So, maybe some words why I accepted this patch.
> 
> On Fri, Mar 02, 2018 at 11:19:31AM +0000, Russell King - ARM Linux wrote:
> > 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.
> 
> I'd much agree to such an approach, yet I didn't see it coming along so
> far for years(?) now.

It needs platform maintainers to be motivated to fix it, and one way to
provide that motivation is for subsystem maintainers to say no to patches
like this.  If patches like this get accepted, then the "problem" gets
solved, and there is very little motivation to fix the platform itself.

If you look back at the history of this, the times when platforms have
been fixed is when they have a problem exactly like this, and they're
told to fix their platforms IRQ numbering instead of the patch to "fix"
the driver being accepted.

Why fix the interrupt numbering if patches to "fix" drivers get
accepted?
Wolfram Sang March 2, 2018, 1:58 p.m. UTC | #12
> It needs platform maintainers to be motivated to fix it, and one way to
> provide that motivation is for subsystem maintainers to say no to patches
> like this.  If patches like this get accepted, then the "problem" gets
> solved, and there is very little motivation to fix the platform itself.

Yes, I can see this. I will drop / revert the patch.
Dan Carpenter March 2, 2018, 2:09 p.m. UTC | #13
On Fri, Mar 02, 2018 at 02:58:54PM +0100, Wolfram Sang wrote:
> 
> > It needs platform maintainers to be motivated to fix it, and one way to
> > provide that motivation is for subsystem maintainers to say no to patches
> > like this.  If patches like this get accepted, then the "problem" gets
> > solved, and there is very little motivation to fix the platform itself.
> 
> Yes, I can see this. I will drop / revert the patch.
> 

TBH, I can't find the threads from November so I feel a bit lost and
there is no documentation for platform_get_irq().

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King (Oracle) March 2, 2018, 3:32 p.m. UTC | #14
On Fri, Mar 02, 2018 at 05:09:01PM +0300, Dan Carpenter wrote:
> On Fri, Mar 02, 2018 at 02:58:54PM +0100, Wolfram Sang wrote:
> > 
> > > It needs platform maintainers to be motivated to fix it, and one way to
> > > provide that motivation is for subsystem maintainers to say no to patches
> > > like this.  If patches like this get accepted, then the "problem" gets
> > > solved, and there is very little motivation to fix the platform itself.
> > 
> > Yes, I can see this. I will drop / revert the patch.
> > 
> 
> TBH, I can't find the threads from November so I feel a bit lost and
> there is no documentation for platform_get_irq().

Start from here:

http://archive.armlinux.org.uk/lurker/search/20380101.000000.00000000@ml:linux-arm-kernel,sb:platform%5Fget%5Firq.en.html

With the right list archiving software with a built-in search facility,
it becomes much easier to find stuff!  There's quite a number of messages
there though, as there were multiple patch series posted.

Some specific messages:

http://archive.armlinux.org.uk/lurker/message/20171204.182556.775e16ab.en.html
http://archive.armlinux.org.uk/lurker/message/20171120.164840.87002f9c.en.html
http://archive.armlinux.org.uk/lurker/message/20171118.182704.3e1a5553.en.html

The reason it hasn't be trivially done (just by changing
platform_get_irq() now) is that doing so will cause a bunch of
regressions, precisely because people _are_ using IRQ 0 with some
platform drivers.

The patch series above has died a death, so yet again the IRQ0/NO_IRQ
issue has disappeared off people's radars and there's no reason to
fix the situation.  So, we're yet again back to the status quo of
almost nothing happening.

How do we break this status quo and finally solve the IRQ 0 and
NO_IRQ issue?

I believe that we have to bite the bullet and start by saying no to
these trivial patches which try to preserve the current situation.
That at least provides some motivation for things to get fixed in
the right way.

Another possibility would be to change platform_get_irq() and
suffer the regressions that will cause, telling people that fixing
their platform IRQ numbering is the only solution (but this
requires breaking our ideals about regressions.)

The alternative is everyone (including Linus) stops whinging about
NO_IRQ and IRQ0 and put up with the fact that some platforms treat
IRQ0 as a valid interrupt - which, I think we can all agree, isn't
going to happen.
diff mbox

Patch

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;