| Message ID | 1512409703-20881-5-git-send-email-arvind.yadav.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: Arvind Yadav <arvind.yadav.cs@gmail.com> Date: Mon, 4 Dec 2017 23:18:20 +0530 > @@ -120,9 +120,10 @@ static int sni_82596_probe(struct platform_device *dev) > netdevice->dev_addr[5] = readb(eth_addr + 0x06); > iounmap(eth_addr); > > - if (!netdevice->irq) { > + if (netdevice->irq <= 0) { > printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n", > __FILE__, netdevice->base_addr); > + retval = netdevice->irq ? netdevice->irq : -ENODEV; > goto probe_failed; > } Ok, thinking about this some more... It is impossible to use platform_get_irq() without every single call site having this funny: ret = val ? val : -ENODEV; sequence. This is unnecessary duplication and it is also error prone, so I really think this logic belongs in platform_get_irq() itself. It can convert '0' to -ENODEV and that way we need no special logic in the callers at all.
Hi David, On Monday 04 December 2017 11:55 PM, David Miller wrote: > From: Arvind Yadav <arvind.yadav.cs@gmail.com> > Date: Mon, 4 Dec 2017 23:18:20 +0530 > >> @@ -120,9 +120,10 @@ static int sni_82596_probe(struct platform_device *dev) >> netdevice->dev_addr[5] = readb(eth_addr + 0x06); >> iounmap(eth_addr); >> >> - if (!netdevice->irq) { >> + if (netdevice->irq <= 0) { >> printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n", >> __FILE__, netdevice->base_addr); >> + retval = netdevice->irq ? netdevice->irq : -ENODEV; >> goto probe_failed; >> } > Ok, thinking about this some more... > > It is impossible to use platform_get_irq() without every single call > site having this funny: > > ret = val ? val : -ENODEV; > > sequence. > > This is unnecessary duplication and it is also error prone, so I > really think this logic belongs in platform_get_irq() itself. It can > convert '0' to -ENODEV and that way we need no special logic in the > callers at all. platform_get_irq() will return 0 only for sparc, If sparc initialize platform data irq[PROMINTR_MAX] as zero. Otherwise platform_get_irq() will never return 0. It will return either IRQ number or error (as negative number). But I am getting review comment by reviewer/maintainer in other subsystem to add check for zero. So I have done same changes here. Please correct me if i am wrong. ~arvind
On 12/4/2017 9:25 PM, David Miller wrote: >> @@ -120,9 +120,10 @@ static int sni_82596_probe(struct platform_device *dev) >> netdevice->dev_addr[5] = readb(eth_addr + 0x06); >> iounmap(eth_addr); >> >> - if (!netdevice->irq) { >> + if (netdevice->irq <= 0) { >> printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n", >> __FILE__, netdevice->base_addr); >> + retval = netdevice->irq ? netdevice->irq : -ENODEV; >> goto probe_failed; >> } > > Ok, thinking about this some more... > > It is impossible to use platform_get_irq() without every single call > site having this funny: > > ret = val ? val : -ENODEV; > > sequence. > > This is unnecessary duplication and it is also error prone, so I > really think this logic belongs in platform_get_irq() itself. It can > convert '0' to -ENODEV and that way we need no special logic in the > callers at all. Well, we can have: return r && r->start ? r->start : -ENXIO; instead of: return r ? r->start : -ENXIO; at the end of platform_get_irq(). But I don't really think it's worth doing -- request_irq() doesn't filter out IRQ0 anyway, IIRC... MBR, Sergei
On Tue, Dec 05, 2017 at 01:12:23PM +0300, Sergei Shtylyov wrote: > Well, we can have: > > return r && r->start ? r->start : -ENXIO; > > instead of: > > return r ? r->start : -ENXIO; > > at the end of platform_get_irq(). But I don't really think it's worth doing > -- request_irq() doesn't filter out IRQ0 anyway, IIRC... There's a bunch of platforms in the kernel that still use IRQ0, and have done prior to Linus' comments on the subject. Such a change would regress them.
From: Arvind Yadav <arvind.yadav.cs@gmail.com> Date: Tue, 5 Dec 2017 11:04:55 +0530 > Hi David, > > > On Monday 04 December 2017 11:55 PM, David Miller wrote: >> From: Arvind Yadav <arvind.yadav.cs@gmail.com> >> Date: Mon, 4 Dec 2017 23:18:20 +0530 >> >>> @@ -120,9 +120,10 @@ static int sni_82596_probe(struct platform_device >>> *dev) >>> netdevice->dev_addr[5] = readb(eth_addr + 0x06); >>> iounmap(eth_addr); >>> - if (!netdevice->irq) { >>> + if (netdevice->irq <= 0) { >>> printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n", >>> __FILE__, netdevice->base_addr); >>> + retval = netdevice->irq ? netdevice->irq : -ENODEV; >>> goto probe_failed; >>> } >> Ok, thinking about this some more... >> >> It is impossible to use platform_get_irq() without every single call >> site having this funny: >> >> ret = val ? val : -ENODEV; >> >> sequence. >> >> This is unnecessary duplication and it is also error prone, so I >> really think this logic belongs in platform_get_irq() itself. It can >> convert '0' to -ENODEV and that way we need no special logic in the >> callers at all. > platform_get_irq() will return 0 only for sparc, If sparc initialize > platform > data irq[PROMINTR_MAX] as zero. Otherwise platform_get_irq() will > never return > 0. It will return either IRQ number or error (as negative number). But > I am getting > review comment by reviewer/maintainer in other subsystem to add check > for > zero. So I have done same changes here. Please correct me if i am > wrong. If you make the change that I suggest, you instead can check for '-ENODEV' to mean no IRQ.
On 12/05/2017 06:49 PM, David Miller wrote: >>> From: Arvind Yadav <arvind.yadav.cs@gmail.com> >>> Date: Mon, 4 Dec 2017 23:18:20 +0530 >>> >>>> @@ -120,9 +120,10 @@ static int sni_82596_probe(struct platform_device >>>> *dev) >>>> netdevice->dev_addr[5] = readb(eth_addr + 0x06); >>>> iounmap(eth_addr); >>>> - if (!netdevice->irq) { >>>> + if (netdevice->irq <= 0) { >>>> printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n", >>>> __FILE__, netdevice->base_addr); >>>> + retval = netdevice->irq ? netdevice->irq : -ENODEV; >>>> goto probe_failed; >>>> } >>> Ok, thinking about this some more... >>> >>> It is impossible to use platform_get_irq() without every single call >>> site having this funny: >>> >>> ret = val ? val : -ENODEV; >>> >>> sequence. >>> >>> This is unnecessary duplication and it is also error prone, so I >>> really think this logic belongs in platform_get_irq() itself. It can >>> convert '0' to -ENODEV and that way we need no special logic in the >>> callers at all. >> platform_get_irq() will return 0 only for sparc, If sparc initialize >> platform >> data irq[PROMINTR_MAX] as zero. Otherwise platform_get_irq() will >> never return >> 0. It will return either IRQ number or error (as negative number). But >> I am getting >> review comment by reviewer/maintainer in other subsystem to add check >> for >> zero. So I have done same changes here. Please correct me if i am >> wrong. > > If you make the change that I suggest, you instead can check for I assume such change is needed only for the SPARC-specific section of platform_get_irq()? > '-ENODEV' to mean no IRQ. No specific error check is needed, just irq < 0 check should be enough... Also, looking at platform_get_irq(), -ENXIO should be returned in this case. MBR, Sergei
Hi David, On Wednesday 06 December 2017 05:49 PM, Sergei Shtylyov wrote: > On 12/05/2017 06:49 PM, David Miller wrote: > >>>> From: Arvind Yadav <arvind.yadav.cs@gmail.com> >>>> Date: Mon, 4 Dec 2017 23:18:20 +0530 >>>> >>>>> @@ -120,9 +120,10 @@ static int sni_82596_probe(struct >>>>> platform_device >>>>> *dev) >>>>> netdevice->dev_addr[5] = readb(eth_addr + 0x06); >>>>> iounmap(eth_addr); >>>>> - if (!netdevice->irq) { >>>>> + if (netdevice->irq <= 0) { >>>>> printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n", >>>>> __FILE__, netdevice->base_addr); >>>>> + retval = netdevice->irq ? netdevice->irq : -ENODEV; >>>>> goto probe_failed; >>>>> } >>>> Ok, thinking about this some more... >>>> >>>> It is impossible to use platform_get_irq() without every single call >>>> site having this funny: >>>> >>>> ret = val ? val : -ENODEV; >>>> >>>> sequence. >>>> >>>> This is unnecessary duplication and it is also error prone, so I >>>> really think this logic belongs in platform_get_irq() itself. It can >>>> convert '0' to -ENODEV and that way we need no special logic in the >>>> callers at all. >>> platform_get_irq() will return 0 only for sparc, If sparc initialize >>> platform >>> data irq[PROMINTR_MAX] as zero. Otherwise platform_get_irq() will >>> never return >>> 0. It will return either IRQ number or error (as negative number). But >>> I am getting >>> review comment by reviewer/maintainer in other subsystem to add check >>> for >>> zero. So I have done same changes here. Please correct me if i am >>> wrong. >> >> If you make the change that I suggest, you instead can check for > > I assume such change is needed only for the SPARC-specific section > of platform_get_irq()? > >> '-ENODEV' to mean no IRQ. > > No specific error check is needed, just irq < 0 check should be > enough... > Also, looking at platform_get_irq(), -ENXIO should be returned in this > case. > > MBR, Sergei Is it ok. If We will add a check for only < 0. Regards Arvind
diff --git a/drivers/net/ethernet/i825xx/sni_82596.c b/drivers/net/ethernet/i825xx/sni_82596.c index b2c04a7..f2a11fc 100644 --- a/drivers/net/ethernet/i825xx/sni_82596.c +++ b/drivers/net/ethernet/i825xx/sni_82596.c @@ -120,9 +120,10 @@ static int sni_82596_probe(struct platform_device *dev) netdevice->dev_addr[5] = readb(eth_addr + 0x06); iounmap(eth_addr); - if (!netdevice->irq) { + if (netdevice->irq <= 0) { printk(KERN_ERR "%s: IRQ not found for i82596 at 0x%lx\n", __FILE__, netdevice->base_addr); + retval = netdevice->irq ? netdevice->irq : -ENODEV; goto probe_failed; }
The platform_get_irq() function returns negative number if an error occurs, Zero if No irq is found and positive number if irq gets successful. platform_get_irq() error checking only for zero is not correct. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- changes in v2: commit message was not correct. drivers/net/ethernet/i825xx/sni_82596.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)