| Message ID | 1451143726-28195-7-git-send-email-Julia.Lawall@lip6.fr |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 12/26/2015 6:28 PM, Julia Lawall wrote: > Return a negative error code on failure. > > A simplified version of the semantic match that finds this problem is as > follows: (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > identifier ret; expression e1,e2; > @@ > ( > if (\(ret < 0\|ret != 0\)) > { ... return ret; } > | > ret = 0 > ) > ... when != ret = e1 > when != &ret > *if(...) > { > ... when != ret = e2 > when forall > return ret; > } > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > drivers/net/ethernet/ti/cpsw.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index 3409e80..6a76992 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -2448,8 +2448,10 @@ static int cpsw_probe(struct platform_device *pdev) > > /* RX IRQ */ > irq = platform_get_irq(pdev, 1); > - if (irq < 0) > + if (irq < 0) { > + ret = -ENOENT; Why not just propagate an error returned by that function? > goto clean_ale_ret; > + } > > priv->irqs_table[0] = irq; > ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt, > @@ -2461,8 +2463,10 @@ static int cpsw_probe(struct platform_device *pdev) > > /* TX IRQ */ > irq = platform_get_irq(pdev, 2); > - if (irq < 0) > + if (irq < 0) { > + ret = -ENOENT; Likewise? [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > diff --git a/drivers/net/ethernet/ti/cpsw.c > > b/drivers/net/ethernet/ti/cpsw.c > > index 3409e80..6a76992 100644 > > --- a/drivers/net/ethernet/ti/cpsw.c > > +++ b/drivers/net/ethernet/ti/cpsw.c > > @@ -2448,8 +2448,10 @@ static int cpsw_probe(struct platform_device *pdev) > > > > /* RX IRQ */ > > irq = platform_get_irq(pdev, 1); > > - if (irq < 0) > > + if (irq < 0) { > > + ret = -ENOENT; > > Why not just propagate an error returned by that function? OK, I did what was done a few lines before in the same function: ndev->irq = platform_get_irq(pdev, 1); if (ndev->irq < 0) { dev_err(priv->dev, "error getting irq resource\n"); ret = -ENOENT; goto clean_ale_ret; } Maybe they should all be changed? julia > > goto clean_ale_ret; > > + } > > > > priv->irqs_table[0] = irq; > > ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt, > > @@ -2461,8 +2463,10 @@ static int cpsw_probe(struct platform_device *pdev) > > > > /* TX IRQ */ > > irq = platform_get_irq(pdev, 2); > > - if (irq < 0) > > + if (irq < 0) { > > + ret = -ENOENT; > > Likewise? > > [...] > > MBR, Sergei > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/26/2015 8:40 PM, Julia Lawall wrote: >>> diff --git a/drivers/net/ethernet/ti/cpsw.c >>> b/drivers/net/ethernet/ti/cpsw.c >>> index 3409e80..6a76992 100644 >>> --- a/drivers/net/ethernet/ti/cpsw.c >>> +++ b/drivers/net/ethernet/ti/cpsw.c >>> @@ -2448,8 +2448,10 @@ static int cpsw_probe(struct platform_device *pdev) >>> >>> /* RX IRQ */ >>> irq = platform_get_irq(pdev, 1); >>> - if (irq < 0) >>> + if (irq < 0) { >>> + ret = -ENOENT; >> >> Why not just propagate an error returned by that function? > > OK, I did what was done a few lines before in the same function: > > ndev->irq = platform_get_irq(pdev, 1); > if (ndev->irq < 0) { > dev_err(priv->dev, "error getting irq resource\n"); > ret = -ENOENT; > goto clean_ale_ret; > } > > Maybe they should all be changed? Yeah, I'd vote for it. I'm seeing no sense in overriding an actual error. [...] > julia MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 12/26/2015 08:50 PM, Sergei Shtylyov wrote: >>>> diff --git a/drivers/net/ethernet/ti/cpsw.c >>>> b/drivers/net/ethernet/ti/cpsw.c >>>> index 3409e80..6a76992 100644 >>>> --- a/drivers/net/ethernet/ti/cpsw.c >>>> +++ b/drivers/net/ethernet/ti/cpsw.c >>>> @@ -2448,8 +2448,10 @@ static int cpsw_probe(struct platform_device *pdev) >>>> >>>> /* RX IRQ */ >>>> irq = platform_get_irq(pdev, 1); >>>> - if (irq < 0) >>>> + if (irq < 0) { >>>> + ret = -ENOENT; >>> >>> Why not just propagate an error returned by that function? >> >> OK, I did what was done a few lines before in the same function: >> >> ndev->irq = platform_get_irq(pdev, 1); >> if (ndev->irq < 0) { >> dev_err(priv->dev, "error getting irq resource\n"); >> ret = -ENOENT; >> goto clean_ale_ret; >> } >> >> Maybe they should all be changed? > > Yeah, I'd vote for it. I'm seeing no sense in overriding an actual error. Hm, I decided to check drivers/base/dd.c and I think I maybe know the reason now: -ENXIO, usually returned by platform_get_irq(), is silently "swallowed" by really_probe(); to be precise, -ENODEV and -ENXIO are only reported with pr_debug(), while -ENOENT causes printk(KERN_WARNING, ...)... > [...] > >> julia MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 26 Dec 2015, Sergei Shtylyov wrote: > Hello. > > On 12/26/2015 08:50 PM, Sergei Shtylyov wrote: > > > > > > diff --git a/drivers/net/ethernet/ti/cpsw.c > > > > > b/drivers/net/ethernet/ti/cpsw.c > > > > > index 3409e80..6a76992 100644 > > > > > --- a/drivers/net/ethernet/ti/cpsw.c > > > > > +++ b/drivers/net/ethernet/ti/cpsw.c > > > > > @@ -2448,8 +2448,10 @@ static int cpsw_probe(struct platform_device > > > > > *pdev) > > > > > > > > > > /* RX IRQ */ > > > > > irq = platform_get_irq(pdev, 1); > > > > > - if (irq < 0) > > > > > + if (irq < 0) { > > > > > + ret = -ENOENT; > > > > > > > > Why not just propagate an error returned by that function? > > > > > > OK, I did what was done a few lines before in the same function: > > > > > > ndev->irq = platform_get_irq(pdev, 1); > > > if (ndev->irq < 0) { > > > dev_err(priv->dev, "error getting irq resource\n"); > > > ret = -ENOENT; > > > goto clean_ale_ret; > > > } > > > > > > Maybe they should all be changed? > > > > Yeah, I'd vote for it. I'm seeing no sense in overriding an actual > > error. > > Hm, I decided to check drivers/base/dd.c and I think I maybe know the > reason now: -ENXIO, usually returned by platform_get_irq(), is silently > "swallowed" by really_probe(); to be precise, -ENODEV and -ENXIO are only > reported with pr_debug(), while -ENOENT causes printk(KERN_WARNING, ...)... Sorry, I'm confused... What should it be? v1 or v2? Here are the counts of the different constants returned on failure of platform_get_irq: ENODEV: 84 ENXIO: 67 EINVAL: 61 ENOENT: 29 EBUSY: 11 EIO: 2 EPROBE_DEFER: 1 julia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/26/2015 11:07 PM, Julia Lawall wrote: >>>>>> diff --git a/drivers/net/ethernet/ti/cpsw.c >>>>>> b/drivers/net/ethernet/ti/cpsw.c >>>>>> index 3409e80..6a76992 100644 >>>>>> --- a/drivers/net/ethernet/ti/cpsw.c >>>>>> +++ b/drivers/net/ethernet/ti/cpsw.c >>>>>> @@ -2448,8 +2448,10 @@ static int cpsw_probe(struct platform_device >>>>>> *pdev) >>>>>> >>>>>> /* RX IRQ */ >>>>>> irq = platform_get_irq(pdev, 1); >>>>>> - if (irq < 0) >>>>>> + if (irq < 0) { >>>>>> + ret = -ENOENT; >>>>> >>>>> Why not just propagate an error returned by that function? >>>> >>>> OK, I did what was done a few lines before in the same function: >>>> >>>> ndev->irq = platform_get_irq(pdev, 1); >>>> if (ndev->irq < 0) { >>>> dev_err(priv->dev, "error getting irq resource\n"); >>>> ret = -ENOENT; >>>> goto clean_ale_ret; >>>> } >>>> >>>> Maybe they should all be changed? >>> >>> Yeah, I'd vote for it. I'm seeing no sense in overriding an actual >>> error. >> >> Hm, I decided to check drivers/base/dd.c and I think I maybe know the >> reason now: -ENXIO, usually returned by platform_get_irq(), is silently >> "swallowed" by really_probe(); to be precise, -ENODEV and -ENXIO are only >> reported with pr_debug(), while -ENOENT causes printk(KERN_WARNING, ...)... > Sorry, I'm confused... What should it be? v1 or v2? Here are the counts > of the different constants returned on failure of platform_get_irq: I was somewhat confused myself but then I remembered about the deferred probing -- overriding error code basically just disables it in this case. > ENODEV: 84 > ENXIO: 67 Those 2 totally make no sense. :-) > EINVAL: 61 > ENOENT: 29 > EBUSY: 11 Hm... > EIO: 2 > EPROBE_DEFER: 1 Hm, and that last one is unconditional? > julia MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index 3409e80..6a76992 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -2448,8 +2448,10 @@ static int cpsw_probe(struct platform_device *pdev) /* RX IRQ */ irq = platform_get_irq(pdev, 1); - if (irq < 0) + if (irq < 0) { + ret = -ENOENT; goto clean_ale_ret; + } priv->irqs_table[0] = irq; ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt, @@ -2461,8 +2463,10 @@ static int cpsw_probe(struct platform_device *pdev) /* TX IRQ */ irq = platform_get_irq(pdev, 2); - if (irq < 0) + if (irq < 0) { + ret = -ENOENT; goto clean_ale_ret; + } priv->irqs_table[1] = irq; ret = devm_request_irq(&pdev->dev, irq, cpsw_tx_interrupt,
Return a negative error code on failure. A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/) // <smpl> @@ identifier ret; expression e1,e2; @@ ( if (\(ret < 0\|ret != 0\)) { ... return ret; } | ret = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; } // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- drivers/net/ethernet/ti/cpsw.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html