[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Krzysztof Kozlowski]

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 at 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;
-- 
2.7.4

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Wolfram Sang]

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 at 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!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180302/998dce37/attachment.sig>

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Dan Carpenter]

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

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Dan Carpenter]

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

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Wolfram Sang]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180302/3201d244/attachment.sig>

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Dan Carpenter]

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

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Dan Carpenter]

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

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Wolfram Sang]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180302/9150d762/attachment.sig>

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Russell King - ARM Linux]

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?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Wolfram Sang]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180302/d2724e24/attachment-0001.sig>

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Dan Carpenter]

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

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Russell King - ARM Linux]

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Mark Rutland]

On Fri, Mar 02, 2018 at 03:32:22PM +0000, Russell King - ARM Linux wrote:
> How do we break this status quo and finally solve the IRQ 0 and
> NO_IRQ issue?

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

How about we start with a warning? That'll be visible, but shouldn't
result in broken systems while we wait for people to fix things up.

e.g. something like the below.

Mark.

---->8----
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f1bf7b38d91c..bd42eeffd2aa 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -126,7 +126,12 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
                irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
        }
 
-       return r ? r->start : -ENXIO;
+       if (!r)
+               return -ENXIO;
+
+       WARN_ONCE(!r->start, "Platform uses zero as a valid IRQ.");
+
+       return r->start;
 #endif
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Andy Shevchenko]

On Fri, Mar 2, 2018 at 6:28 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Mar 02, 2018 at 03:32:22PM +0000, Russell King - ARM Linux wrote:
>> How do we break this status quo and finally solve the IRQ 0 and
>> NO_IRQ issue?

Guys, the question: Wouldn't be request_irq() failed when it gets a
wrong number on input?

-- 
With Best Regards,
Andy Shevchenko

[PATCH] i2c: s3c2410: Properly handle interrupts of number 0

[Russell King - ARM Linux]

On Sat, Mar 03, 2018 at 06:25:17PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 2, 2018 at 6:28 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Mar 02, 2018 at 03:32:22PM +0000, Russell King - ARM Linux wrote:
> >> How do we break this status quo and finally solve the IRQ 0 and
> >> NO_IRQ issue?
> 
> Guys, the question: Wouldn't be request_irq() failed when it gets a
> wrong number on input?

Unfortunately not - IRQ 0 is kind of valid on x86 (it's the i8253 PIT)
and an exception is made for x86 arch code with regard to this.  It
gets setup using setup_irq() rather than request_irq() (see
arch/x86/kernel/time.c::setup_default_timer_irq()).

request_irq() doesn't deny IRQ 0 - it denies IRQ_NOTCONNECTED and
anything that irq_to_desc() returns NULL for, which can be a radix
tree lookup or simply any unsigned IRQ number less than NR_IRQS for
legacy platforms.

If you're on a DT platform, then the IRQ subsystem avoids allocating
IRQ0 for any DT IRQ controller, so DT platforms should be fine.  It's
just the legacy platforms that continue to be an ongoing issue wrt
the IRQ 0 / NO_IRQ business, and those will generally be using the
non-radix tree version of irq_to_desc().

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up