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 <[email protected]>
---
drivers/net/ethernet/3com/3c59x.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
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;
--
2.17.2
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 <[email protected]> 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 <[email protected]> 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);
--
Florian
> 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 <[email protected]> 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