| Message ID | 1519936484-23132-1-git-send-email-krzk@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: s3c2410: Properly handle interrupts of number 0 | expand |
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!
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
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
> 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.
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.
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.
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?
> 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.
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
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.
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);
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?
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().
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;
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(-)