netdev - Re: [PATCH] net: tulip: remove redundant assignment to variable new

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220125141401.GV1951@kadam>
Date:   Tue, 25 Jan 2022 17:14:01 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Colin Ian King <colin.i.king@...il.com>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-parisc@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: tulip: remove redundant assignment to variable
 new_csr6

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