| Message ID | 20220123183440.112495-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: tulip: remove redundant assignment to variable new_csr6 | expand |
On Sun, 23 Jan 2022 18:34:40 +0000 Colin Ian King wrote: > Variable new_csr6 is being initialized with a value that is never > read, it is being re-assigned later on. The assignment is redundant > and can be removed. > @@ -21,7 +21,7 @@ void pnic_do_nway(struct net_device *dev) > struct tulip_private *tp = netdev_priv(dev); > void __iomem *ioaddr = tp->base_addr; > u32 phy_reg = ioread32(ioaddr + 0xB8); > - u32 new_csr6 = tp->csr6 & ~0x40C40200; > + u32 new_csr6; > > if (phy_reg & 0x78000000) { /* Ignore baseT4 */ > if (phy_reg & 0x20000000) dev->if_port = 5; I can't say I see what you mean, it's not set in some cases: if (tp->medialock) { } else if (tp->nwayset && (dev->if_port & 1)) { next_tick = 1*HZ; } else if (dev->if_port == 0) { dev->if_port = 3; iowrite32(0x33, ioaddr + CSR12); new_csr6 = 0x01860000; iowrite32(0x1F868, ioaddr + 0xB8); } else { dev->if_port = 0; iowrite32(0x32, ioaddr + CSR12); new_csr6 = 0x00420000; iowrite32(0x1F078, ioaddr + 0xB8); } if (tp->csr6 != new_csr6) { tp->csr6 = new_csr6; That said clang doesn't complain so maybe I'm missing something static analysis had figured out about this code.
On Mon, Jan 24, 2022 at 10:30:38AM -0800, Jakub Kicinski wrote: > On Sun, 23 Jan 2022 18:34:40 +0000 Colin Ian King wrote: > > Variable new_csr6 is being initialized with a value that is never > > read, it is being re-assigned later on. The assignment is redundant > > and can be removed. > > > @@ -21,7 +21,7 @@ void pnic_do_nway(struct net_device *dev) > > struct tulip_private *tp = netdev_priv(dev); > > void __iomem *ioaddr = tp->base_addr; > > u32 phy_reg = ioread32(ioaddr + 0xB8); > > - u32 new_csr6 = tp->csr6 & ~0x40C40200; > > + u32 new_csr6; > > > > if (phy_reg & 0x78000000) { /* Ignore baseT4 */ > > if (phy_reg & 0x20000000) dev->if_port = 5; > > I can't say I see what you mean, it's not set in some cases: > > if (tp->medialock) { > } else if (tp->nwayset && (dev->if_port & 1)) { > next_tick = 1*HZ; > } else if (dev->if_port == 0) { > dev->if_port = 3; > iowrite32(0x33, ioaddr + CSR12); > new_csr6 = 0x01860000; > iowrite32(0x1F868, ioaddr + 0xB8); > } else { > dev->if_port = 0; > iowrite32(0x32, ioaddr + CSR12); > new_csr6 = 0x00420000; > iowrite32(0x1F078, ioaddr + 0xB8); > } > if (tp->csr6 != new_csr6) { > tp->csr6 = new_csr6; > > > That said clang doesn't complain so maybe I'm missing something static > analysis had figured out about this code. You're looking at the wrong function. This is pnic_do_nway() and you're looking at pnic_timer(). Of course, Colin's patch assumes the current behavior is correct... I guess the current behavior can't be that terrible since it predates git and no one has complained. drivers/net/ethernet/dec/tulip/pnic.c 19 void pnic_do_nway(struct net_device *dev) 20 { 21 struct tulip_private *tp = netdev_priv(dev); 22 void __iomem *ioaddr = tp->base_addr; 23 u32 phy_reg = ioread32(ioaddr + 0xB8); 24 u32 new_csr6 = tp->csr6 & ~0x40C40200; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 25 26 if (phy_reg & 0x78000000) { /* Ignore baseT4 */ 27 if (phy_reg & 0x20000000) dev->if_port = 5; 28 else if (phy_reg & 0x40000000) dev->if_port = 3; 29 else if (phy_reg & 0x10000000) dev->if_port = 4; 30 else if (phy_reg & 0x08000000) dev->if_port = 0; 31 tp->nwayset = 1; 32 new_csr6 = (dev->if_port & 1) ? 0x01860000 : 0x00420000; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 33 iowrite32(0x32 | (dev->if_port & 1), ioaddr + CSR12); 34 if (dev->if_port & 1) 35 iowrite32(0x1F868, ioaddr + 0xB8); 36 if (phy_reg & 0x30000000) { 37 tp->full_duplex = 1; 38 new_csr6 |= 0x00000200; 39 } 40 if (tulip_debug > 1) 41 netdev_dbg(dev, "PNIC autonegotiated status %08x, %s\n", 42 phy_reg, medianame[dev->if_port]); 43 if (tp->csr6 != new_csr6) { 44 tp->csr6 = new_csr6; 45 /* Restart Tx */ 46 tulip_restart_rxtx(tp); 47 netif_trans_update(dev); 48 } 49 } 50 } regards, dan carpenter
On Tue, 25 Jan 2022 17:14:01 +0300 Dan Carpenter wrote: > You're looking at the wrong function. This is pnic_do_nway() and you're > looking at pnic_timer(). Ah, that explains it! Thanks, applied now. > Of course, Colin's patch assumes the current behavior is correct... I > guess the current behavior can't be that terrible since it predates git > and no one has complained. Entirely possible this driver was never used in the git era.
diff --git a/drivers/net/ethernet/dec/tulip/pnic.c b/drivers/net/ethernet/dec/tulip/pnic.c index 3fb39e32e1b4..653bde48ef44 100644 --- a/drivers/net/ethernet/dec/tulip/pnic.c +++ b/drivers/net/ethernet/dec/tulip/pnic.c @@ -21,7 +21,7 @@ void pnic_do_nway(struct net_device *dev) struct tulip_private *tp = netdev_priv(dev); void __iomem *ioaddr = tp->base_addr; u32 phy_reg = ioread32(ioaddr + 0xB8); - u32 new_csr6 = tp->csr6 & ~0x40C40200; + u32 new_csr6; if (phy_reg & 0x78000000) { /* Ignore baseT4 */ if (phy_reg & 0x20000000) dev->if_port = 5;
Variable new_csr6 is being initialized with a value that is never read, it is being re-assigned later on. The assignment is redundant and can be removed. Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- drivers/net/ethernet/dec/tulip/pnic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)