| Message ID | 1333383895-23889-1-git-send-email-srinivas.kandagatla@st.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2012-04-02 at 17:24 +0100, Srinivas KANDAGATLA wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@st.com> > > Compile tested. > remove unnecessary code that matches this coccinelle pattern > > ret = phy_write(x, y , z) > if (ret < 0) > return ret; > return 0; > > As phy_write returns error code, we dont need to do not need extra check > before returning. Do these really make any functional difference? Doesn't the compiler generate the same output? Many times, there's a code pattern that precedes these calls has a similar pattern and changing the pattern for the last call in a sequence can be jarring to a reader and changing the pattern can sometimes introduce errors as well. -- 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 02/04/12 17:42, Joe Perches wrote: > On Mon, 2012-04-02 at 17:24 +0100, Srinivas KANDAGATLA wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@st.com> >> >> Compile tested. >> remove unnecessary code that matches this coccinelle pattern >> >> ret = phy_write(x, y , z) >> if (ret < 0) >> return ret; >> return 0; >> >> As phy_write returns error code, we dont need to do not need extra check >> before returning. > > Do these really make any functional difference? No it does not make any functional difference. > Doesn't the compiler generate the same output? > I think it will not generate same output. > Many times, there's a code pattern that precedes these > calls has a similar pattern and changing the pattern > for the last call in a sequence can be jarring to a > reader and changing the pattern can sometimes introduce > errors as well. There is a purpose(error handling) of having similar pattern for the code above last call, However there is no value for doing an additional check before returning. If we look at other phy files(ex:boardcom.c..), we can see they do something similar to what the patch does in config_init. > > -- 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 Mon, 2012-04-02 at 17:51 +0100, Srinivas KANDAGATLA wrote: > On 02/04/12 17:42, Joe Perches wrote: > > On Mon, 2012-04-02 at 17:24 +0100, Srinivas KANDAGATLA wrote: > > Do these really make any functional difference? > No it does not make any functional difference. > > Doesn't the compiler generate the same output? > I think it will not generate same output. Yes, you are right. The code currently returns 0 for non-error cases and you've changed it to possibly return a positive value. Have you checked all the callers to make sure this doesn't introduce a new defect? -- 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
Yes, I have audited all the drivers. none of them return positive values. In general, I think phy_write should return either 0 or an error code and *NOT* positive values. Any mdio bus driver returning positive values for phy_writes will break the phylib too. On 02/04/12 18:02, Joe Perches wrote: > On Mon, 2012-04-02 at 17:51 +0100, Srinivas KANDAGATLA wrote: >> On 02/04/12 17:42, Joe Perches wrote: >>> On Mon, 2012-04-02 at 17:24 +0100, Srinivas KANDAGATLA wrote: >>> Do these really make any functional difference? >> No it does not make any functional difference. >>> Doesn't the compiler generate the same output? >> I think it will not generate same output. > Yes, you are right. The code currently returns 0 for > non-error cases and you've changed it to possibly > return a positive value. > > Have you checked all the callers to make sure this > doesn't introduce a new defect? > -- 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
From: Srinivas KANDAGATLA <srinivas.kandagatla@st.com> Date: Tue, 03 Apr 2012 10:49:41 +0100 > Yes, I have audited all the drivers. none of them return positive values. > In general, I think phy_write should return either 0 or an error code > and *NOT* positive values. > Any mdio bus driver returning positive values for phy_writes will break > the phylib too. I've applied all three patches to net-next, thanks. -- 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/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c index e16f98c..cd802eb 100644 --- a/drivers/net/phy/bcm63xx.c +++ b/drivers/net/phy/bcm63xx.c @@ -39,10 +39,7 @@ static int bcm63xx_config_init(struct phy_device *phydev) MII_BCM63XX_IR_SPEED | MII_BCM63XX_IR_LINK) | MII_BCM63XX_IR_EN; - err = phy_write(phydev, MII_BCM63XX_IR, reg); - if (err < 0) - return err; - return 0; + return phy_write(phydev, MII_BCM63XX_IR, reg); } static int bcm63xx_ack_interrupt(struct phy_device *phydev)