Message ID | 20200103121907.5769-1-yukuai3@huawei.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | net: 3com: 3c59x: remove set but not used variable 'mii_reg1' | expand |
On Fri, Jan 03, 2020 at 08:19:07PM +0800, yu kuai wrote: > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/net/ethernet/3com/3c59x.c: In function ‘vortex_up’: > drivers/net/ethernet/3com/3c59x.c:1551:9: warning: variable > ‘mii_reg1’ set but not used [-Wunused-but-set-variable] > > It is never used, and so can be removed. ... > if (dev->if_port == XCVR_MII || dev->if_port == XCVR_NWAY) { > - mii_reg1 = mdio_read(dev, vp->phys[0], MII_BMSR); > mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA); I know nothing about the MII interface, but in general this is not a safe thing to do. You're removing a read from a device register. Register reads can have side effects. I'm actually quite surprised that GCC emits this warning, since there should be some kind of volatile cast in mdio_read() to let GCC know that something unusual is going on here.
On Fri, Jan 03, 2020 at 06:46:23AM -0800, Matthew Wilcox wrote: > On Fri, Jan 03, 2020 at 08:19:07PM +0800, yu kuai wrote: > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > drivers/net/ethernet/3com/3c59x.c: In function ‘vortex_up’: > > drivers/net/ethernet/3com/3c59x.c:1551:9: warning: variable > > ‘mii_reg1’ set but not used [-Wunused-but-set-variable] > > > > It is never used, and so can be removed. > ... > > if (dev->if_port == XCVR_MII || dev->if_port == XCVR_NWAY) { > > - mii_reg1 = mdio_read(dev, vp->phys[0], MII_BMSR); > > mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA); > > I know nothing about the MII interface, but in general this is not > a safe thing to do. You're removing a read from a device register. > Register reads can have side effects. I'm actually quite surprised > that GCC emits this warning, since there should be some kind of > volatile cast in mdio_read() to let GCC know that something unusual > is going on here. Removing the call may be wrong (and certainly isn't obviously correct) but the warning makes sense, IMHO: if the return value is not used anywhere, there is no point assigning it to a variable. Michal Kubecek
On Fri, Jan 03, 2020 at 06:46:23AM -0800, Matthew Wilcox wrote: > On Fri, Jan 03, 2020 at 08:19:07PM +0800, yu kuai wrote: > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > drivers/net/ethernet/3com/3c59x.c: In function ‘vortex_up’: > > drivers/net/ethernet/3com/3c59x.c:1551:9: warning: variable > > ‘mii_reg1’ set but not used [-Wunused-but-set-variable] > > > > It is never used, and so can be removed. > ... > > if (dev->if_port == XCVR_MII || dev->if_port == XCVR_NWAY) { > > - mii_reg1 = mdio_read(dev, vp->phys[0], MII_BMSR); > > mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA); > > I know nothing about the MII interface, but in general this is not > a safe thing to do. Hi Matthew I fully agree about the general case. However, reading the MII_BMSR should not have any side affects. It would be an odd Ethernet PHY if it did. But I am curious why this read is here. There is a slim change the MDIO bus is broken, and this is a workaround. So it would be good if somebody dug into the history and found out when this read was added and if there are any comments about why it is there. Or if the usage of mii_reg1 as been removed at some point, and the read was not cleaned up. Andrew
On Fri, Jan 03, 2020 at 06:53:18PM +0100, Andrew Lunn wrote: > On Fri, Jan 03, 2020 at 06:46:23AM -0800, Matthew Wilcox wrote: > > On Fri, Jan 03, 2020 at 08:19:07PM +0800, yu kuai wrote: > > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > > > drivers/net/ethernet/3com/3c59x.c: In function ‘vortex_up’: > > > drivers/net/ethernet/3com/3c59x.c:1551:9: warning: variable > > > ‘mii_reg1’ set but not used [-Wunused-but-set-variable] > > > > > > It is never used, and so can be removed. > > ... > > > if (dev->if_port == XCVR_MII || dev->if_port == XCVR_NWAY) { > > > - mii_reg1 = mdio_read(dev, vp->phys[0], MII_BMSR); > > > mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA); > > > > I know nothing about the MII interface, but in general this is not > > a safe thing to do. > > Hi Matthew > > I fully agree about the general case. However, reading the MII_BMSR > should not have any side affects. It would be an odd Ethernet PHY if > it did. > > But I am curious why this read is here. There is a slim change the > MDIO bus is broken, and this is a workaround. So it would be good if > somebody dug into the history and found out when this read was added > and if there are any comments about why it is there. Or if the usage > of mii_reg1 as been removed at some point, and the read was not > cleaned up. I tried to dig a bit and found that originally (before git), there was also if (vortex_debug > 1) printk(KERN_INFO "%s: MII #%d status %4.4x, link partner capability %4.4x," " info1 %04x, setting %s-duplex.\n", dev->name, vp->phys[0], mii_reg1, mii_reg5, vp->info1, ((vp->info1 & 0x8000) || vp->full_duplex) ? "full" : "half"); The whole mii code in vortex_up() was removed by commit 125d5ce8a4e9 ("[PATCH] 3c59x: use mii_check_media") and later the mdio_read() calls were restored by commit 09ce3512dcad ("[PATCH] 3c59x: fix networking for 10base2 NICs") with "Also brought back some mii stuff to be sure that it does not break something else", but without the debugging printk(). It's not really a proof that reading MII_BMSR was only needed for the log message but there is a good chance it was. I'm afraid we won't be able to find anyone who would be able to tell for sure after all those years. Michal
Hi Andrew, On Fri, 3 Jan 2020 at 19:54, Andrew Lunn <andrew@lunn.ch> wrote: > > I fully agree about the general case. However, reading the MII_BMSR > should not have any side affects. It would be an odd Ethernet PHY if > it did. This is not really correct. As far as I know the clause 22 spec requires the link status bit in BMSR to be latching low, so that momentary losses of link can be caught post-facto. In fact, even genphy_update_link treats this case: /* The link state is latched low so that momentary link * drops can be detected. Do not double-read the status * in polling mode to detect such short link drops. */ if (!phy_polling_mode(phydev)) { status = phy_read(phydev, MII_BMSR); if (status < 0) return status; else if (status & BMSR_LSTATUS) goto done; } So no, reading BMSR generally is not without side effects, and that does not make the PHY odd. Whether clearing the latching-low status bits is of any relevance to the 3com 3c59x driver bookkeeping, that I have not clue. > > Andrew Regards, -Vladimir
On 1/3/20 11:13 AM, Vladimir Oltean wrote: > Hi Andrew, > > On Fri, 3 Jan 2020 at 19:54, Andrew Lunn <andrew@lunn.ch> wrote: >> >> I fully agree about the general case. However, reading the MII_BMSR >> should not have any side affects. It would be an odd Ethernet PHY if >> it did. > > This is not really correct. As far as I know the clause 22 spec > requires the link status bit in BMSR to be latching low, so that > momentary losses of link can be caught post-facto. > In fact, even genphy_update_link treats this case: > > /* The link state is latched low so that momentary link > * drops can be detected. Do not double-read the status > * in polling mode to detect such short link drops. > */ > if (!phy_polling_mode(phydev)) { > status = phy_read(phydev, MII_BMSR); > if (status < 0) > return status; > else if (status & BMSR_LSTATUS) > goto done; > } > > So no, reading BMSR generally is not without side effects, and that > does not make the PHY odd. > > Whether clearing the latching-low status bits is of any relevance to > the 3com 3c59x driver bookkeeping, that I have not clue. And since more reviewers are on the same boat, the fix should probably look to eliminate the warning by doing something like: (void)mdio_read(dev, vp->phys[0], MII_BMSR);
> And since more reviewers are on the same boat, the fix should probably > look to eliminate the warning by doing something like: > > (void)mdio_read(dev, vp->phys[0], MII_BMSR); Yes, this is the safe option. Andrew
On Fri, Jan 3, 2020 at 2:40 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > And since more reviewers are on the same boat, the fix should probably > > look to eliminate the warning by doing something like: > > > > (void)mdio_read(dev, vp->phys[0], MII_BMSR); > > Yes, this is the safe option. I have actual hardware I can test the proposed patches on, if desired. - Matthew Whitehead
On 2020/1/4 3:37, Andrew Lunn wrote: >> And since more reviewers are on the same boat, the fix should probably >> look to eliminate the warning by doing something like: >> >> (void)mdio_read(dev, vp->phys[0], MII_BMSR); > > Yes, this is the safe option. > > Andrew > > . Thank you all for your response! Yu Kuai
diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c index fc046797c0ea..6e537e5dc208 100644 --- a/drivers/net/ethernet/3com/3c59x.c +++ b/drivers/net/ethernet/3com/3c59x.c @@ -1548,7 +1548,7 @@ vortex_up(struct net_device *dev) struct vortex_private *vp = netdev_priv(dev); void __iomem *ioaddr = vp->ioaddr; unsigned int config; - int i, mii_reg1, mii_reg5, err = 0; + int i, mii_reg5, err = 0; if (VORTEX_PCI(vp)) { pci_set_power_state(VORTEX_PCI(vp), PCI_D0); /* Go active */ @@ -1605,7 +1605,6 @@ vortex_up(struct net_device *dev) window_write32(vp, config, 3, Wn3_Config); if (dev->if_port == XCVR_MII || dev->if_port == XCVR_NWAY) { - mii_reg1 = mdio_read(dev, vp->phys[0], MII_BMSR); mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA); vp->partner_flow_ctrl = ((mii_reg5 & 0x0400) != 0); vp->mii.full_duplex = vp->full_duplex;
Fixes gcc '-Wunused-but-set-variable' warning: drivers/net/ethernet/3com/3c59x.c: In function ‘vortex_up’: drivers/net/ethernet/3com/3c59x.c:1551:9: warning: variable ‘mii_reg1’ set but not used [-Wunused-but-set-variable] It is never used, and so can be removed. Signed-off-by: yu kuai <yukuai3@huawei.com> --- drivers/net/ethernet/3com/3c59x.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)