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 for only zero is not correct.
Removed Other 3 patch which is not related to this series.
Arvind Yadav (7):
[PATCH 1/7 v2] net: bcmgenet: Fix platform_get_irq's error checking
[PATCH 2/7 v2] net: ezchip: nps_enet: Fix platform_get_irq's error checking
[PATCH 3/7 v2] can: xilinx: Fix platform_get_irq's error checking
[PATCH 4/7 v2] net: ethernet: i825xx: Fix platform_get_irq's error checking
[PATCH 5/7 v2] net: ethernet: natsemi: Fix platform_get_irq's error checking
[PATCH 6/7 v2] net: ethernet: smsc: Fix platform_get_irq's error checking
[PATCH 7/7 v2] net: fjes: Fix platform_get_irq's error checking
drivers/net/can/xilinx_can.c | 4 ++++
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
drivers/net/ethernet/ezchip/nps_enet.c | 4 ++--
drivers/net/ethernet/i825xx/sni_82596.c | 3 ++-
drivers/net/ethernet/natsemi/jazzsonic.c | 5 +++++
drivers/net/ethernet/smsc/smc911x.c | 5 +++++
drivers/net/fjes/fjes_main.c | 5 +++++
7 files changed, 24 insertions(+), 4 deletions(-)
--
2.7.4
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 <[email protected]>
---
changes in v2:
commit message was not correct.
drivers/net/ethernet/ezchip/nps_enet.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c
index 659f1ad..7d4b628 100644
--- a/drivers/net/ethernet/ezchip/nps_enet.c
+++ b/drivers/net/ethernet/ezchip/nps_enet.c
@@ -623,9 +623,9 @@ static s32 nps_enet_probe(struct platform_device *pdev)
/* Get IRQ number */
priv->irq = platform_get_irq(pdev, 0);
- if (!priv->irq) {
+ if (priv->irq <= 0) {
dev_err(dev, "failed to retrieve <irq Rx-Tx> value from device tree\n");
- err = -ENODEV;
+ err = priv->irq ? priv->irq : -ENODEV;
goto out_netdev;
}
--
2.7.4
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 <[email protected]>
---
changes in v2:
commit message was not correct.
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 24b4f4c..e2f1268 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3371,7 +3371,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
priv->irq0 = platform_get_irq(pdev, 0);
priv->irq1 = platform_get_irq(pdev, 1);
priv->wol_irq = platform_get_irq(pdev, 2);
- if (!priv->irq0 || !priv->irq1) {
+ if (priv->irq0 <= 0 || priv->irq1 <= 0 || priv->wol_irq <= 0) {
dev_err(&pdev->dev, "can't find IRQs\n");
err = -EINVAL;
goto err;
--
2.7.4
platform_get_irq() can fail here and we must check its return value.
Signed-off-by: Arvind Yadav <[email protected]>
---
changes in v2:
commit message was not correct.
drivers/net/ethernet/natsemi/jazzsonic.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/natsemi/jazzsonic.c b/drivers/net/ethernet/natsemi/jazzsonic.c
index d5b2888..22424e9 100644
--- a/drivers/net/ethernet/natsemi/jazzsonic.c
+++ b/drivers/net/ethernet/natsemi/jazzsonic.c
@@ -242,6 +242,11 @@ static int jazz_sonic_probe(struct platform_device *pdev)
dev->base_addr = res->start;
dev->irq = platform_get_irq(pdev, 0);
+ if (dev->irq <= 0) {
+ err = dev->irq ? dev->irq : -ENODEV;
+ goto out;
+ }
+
err = sonic_probe1(dev);
if (err)
goto out;
--
2.7.4
platform_get_irq() can fail here and we must check its return value.
Signed-off-by: Arvind Yadav <[email protected]>
---
changes in v2:
commit message was not correct.
drivers/net/fjes/fjes_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/fjes/fjes_main.c b/drivers/net/fjes/fjes_main.c
index 750954b..af7204b 100644
--- a/drivers/net/fjes/fjes_main.c
+++ b/drivers/net/fjes/fjes_main.c
@@ -1268,6 +1268,11 @@ static int fjes_probe(struct platform_device *plat_dev)
hw->hw_res.start = res->start;
hw->hw_res.size = resource_size(res);
hw->hw_res.irq = platform_get_irq(plat_dev, 0);
+ if (hw->hw_res.irq <= 0) {
+ err = hw->hw_res.irq ? hw->hw_res.irq : -ENODEV;
+ goto err_free_netdev;
+ }
+
err = fjes_hw_init(&adapter->hw);
if (err)
goto err_free_netdev;
--
2.7.4
platform_get_irq() can fail here and we must check its return value.
Signed-off-by: Arvind Yadav <[email protected]>
---
changes in v2:
commit message was not correct.
drivers/net/ethernet/smsc/smc911x.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/smsc/smc911x.c b/drivers/net/ethernet/smsc/smc911x.c
index 0515744..5e3c7af 100644
--- a/drivers/net/ethernet/smsc/smc911x.c
+++ b/drivers/net/ethernet/smsc/smc911x.c
@@ -2088,6 +2088,11 @@ static int smc911x_drv_probe(struct platform_device *pdev)
ndev->dma = (unsigned char)-1;
ndev->irq = platform_get_irq(pdev, 0);
+ if (ndev->irq <= 0) {
+ ret = ndev->irq ? ndev->irq : -ENODEV;
+ goto release_both;
+ }
+
lp = netdev_priv(ndev);
lp->netdev = ndev;
#ifdef SMC_DYNAMIC_BUS_CONFIG
--
2.7.4
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 <[email protected]>
---
changes in v2:
commit message was not correct.
drivers/net/ethernet/i825xx/sni_82596.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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;
}
--
2.7.4
platform_get_irq() can fail here and we must check its return value.
Signed-off-by: Arvind Yadav <[email protected]>
---
changes in v2:
commit message was not correct.
drivers/net/can/xilinx_can.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 89aec07..1b859af 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -1111,6 +1111,10 @@ static int xcan_probe(struct platform_device *pdev)
/* Get IRQ for the device */
ndev->irq = platform_get_irq(pdev, 0);
+ if (ndev->irq <= 0) {
+ ret = ndev->irq ? ndev->irq : -ENODEV;
+ goto err_free;
+ }
ndev->flags |= IFF_ECHO; /* We support local echo */
platform_set_drvdata(pdev, ndev);
--
2.7.4
From: Arvind Yadav <[email protected]>
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.
On 12/04/2017 09:48 AM, Arvind Yadav wrote:
> 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 <[email protected]>
> ---
> changes in v2:
> commit message was not correct.
>
> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 24b4f4c..e2f1268 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3371,7 +3371,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
> priv->irq0 = platform_get_irq(pdev, 0);
> priv->irq1 = platform_get_irq(pdev, 1);
> priv->wol_irq = platform_get_irq(pdev, 2);
> - if (!priv->irq0 || !priv->irq1) {
> + if (priv->irq0 <= 0 || priv->irq1 <= 0 || priv->wol_irq <= 0) {
> dev_err(&pdev->dev, "can't find IRQs\n");
> err = -EINVAL;
> goto err;
>
The absence of a Wake-on-LAN interrupt (wol_irq <= 0) is not a terminal
error for the driver so it should not be included in this check.
The error checking for irq0 and irq1 is appropriate to add, but it
sounds like David Miller is proposing changing platform_get_irq() so
I'll let that dust settle before saying whether <= or < is appropriate.
Thanks,
Doug
Hi David,
On Monday 04 December 2017 11:55 PM, David Miller wrote:
> From: Arvind Yadav <[email protected]>
> 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 8:48 PM, Arvind Yadav wrote:
> 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.
No, returning 0 is not a failure indication anymore. It used to be but not
any longer...
> platform_get_irq() error checking for only zero is not correct.
[...]
MBR, Sergei
On 12/5/2017 12:54 PM, Sergei Shtylyov wrote:
>> 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.
>
> No, returning 0 is not a failure indication anymore! It used to be but not
> any longer...
And I fixed this function exactly to avoid overly complex error checks
(which you're trying to propose here).
>> platform_get_irq() error checking for only zero is not correct.
>
> [...]
MBR, Sergei
Hi Sergei,
On Tuesday 05 December 2017 03:27 PM, Sergei Shtylyov wrote:
> On 12/5/2017 12:54 PM, Sergei Shtylyov wrote:
>
>>> 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.
>>
>> No, returning 0 is not a failure indication anymore! It used to
>> be but not any longer...
>
> And I fixed this function exactly to avoid overly complex error
> checks (which you're trying to propose here).
>
Thanks for your comment. yes you are right. Now It'll not return 0. It
will return irq and error as negative.
I will not add a check for 0.
>>> platform_get_irq() error checking for only zero is not correct.
>>
>> [...]
>
> MBR, Sergei
Thank you,
~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.
--
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
From: Arvind Yadav <[email protected]>
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 <[email protected]>
>> 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 <[email protected]>
>>> 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 <[email protected]>
>>>> 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