re: net: pxa168_eth: set the mac address on the Ethernet controller

re: net: pxa168_eth: set the mac address on the Ethernet controller

[Dan Carpenter]

Hello Antoine Ténart,

The patch 39830689ef0a: "net: pxa168_eth: set the mac address on the
Ethernet controller" from Sep 30, 2014, leads to the following static
checker warning:

	drivers/net/ethernet/marvell/pxa168_eth.c:640 pxa168_eth_set_mac_address()
	warn: using signed char for bitops

drivers/net/ethernet/marvell/pxa168_eth.c
   625  static int pxa168_eth_set_mac_address(struct net_device *dev, void *addr)
   626  {
   627          struct sockaddr *sa = addr;
   628          struct pxa168_eth_private *pep = netdev_priv(dev);
   629          unsigned char oldMac[ETH_ALEN];
   630          u32 mac_h, mac_l;
   631  
   632          if (!is_valid_ether_addr(sa->sa_data))
   633                  return -EADDRNOTAVAIL;
   634          memcpy(oldMac, dev->dev_addr, ETH_ALEN);
   635          memcpy(dev->dev_addr, sa->sa_data, ETH_ALEN);
   636  
   637          mac_h = sa->sa_data[0] << 24;
   638          mac_h |= sa->sa_data[1] << 16;
   639          mac_h |= sa->sa_data[2] << 8;
   640          mac_h |= sa->sa_data[3];

You may end up with weird signedness bugs doing this (depending of if
the highest bit is ever used).

   641          mac_l = sa->sa_data[4] << 8;
   642          mac_l |= sa->sa_data[5];
   643          wrl(pep, MAC_ADDR_HIGH, mac_h);
   644          wrl(pep, MAC_ADDR_LOW, mac_l);
   645  
   646          netif_addr_lock_bh(dev);
   647          update_hash_table_mac_address(pep, oldMac, dev->dev_addr);
   648          netif_addr_unlock_bh(dev);
   649          return 0;
   650  }

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: net: pxa168_eth: set the mac address on the Ethernet controller

[Antoine Tenart]

Hi Dan,

On Wed, Oct 01, 2014 at 07:54:02PM +0300, Dan Carpenter wrote:
> Hello Antoine Ténart,
> 
> The patch 39830689ef0a: "net: pxa168_eth: set the mac address on the
> Ethernet controller" from Sep 30, 2014, leads to the following static
> checker warning:
> 
> 	drivers/net/ethernet/marvell/pxa168_eth.c:640 pxa168_eth_set_mac_address()
> 	warn: using signed char for bitops

Thanks for reporting this!

> 
> drivers/net/ethernet/marvell/pxa168_eth.c
>    625  static int pxa168_eth_set_mac_address(struct net_device *dev, void *addr)
>    626  {
>    627          struct sockaddr *sa = addr;
>    628          struct pxa168_eth_private *pep = netdev_priv(dev);
>    629          unsigned char oldMac[ETH_ALEN];
>    630          u32 mac_h, mac_l;
>    631  
>    632          if (!is_valid_ether_addr(sa->sa_data))
>    633                  return -EADDRNOTAVAIL;
>    634          memcpy(oldMac, dev->dev_addr, ETH_ALEN);
>    635          memcpy(dev->dev_addr, sa->sa_data, ETH_ALEN);
>    636  
>    637          mac_h = sa->sa_data[0] << 24;
>    638          mac_h |= sa->sa_data[1] << 16;
>    639          mac_h |= sa->sa_data[2] << 8;
>    640          mac_h |= sa->sa_data[3];
> 
> You may end up with weird signedness bugs doing this (depending of if
> the highest bit is ever used).

Since dev->dev_addr is of type unsigned char *, I could use it instead.
That's what most of the other net drivers do, at least.

While having a look on other net drivers, I spotted
drivers/net/ethernet/toshiba/spider_net.c which is also using signed
char for bitops and may need a fix:

        macu = (addr->sa_data[0]<<24) + (addr->sa_data[1]<<16) +
                (addr->sa_data[2]<<8) + (addr->sa_data[3]);
        macl = (addr->sa_data[4]<<8) + (addr->sa_data[5]);


I'll cook up a fix for both of them if the solution's OK with you.


Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: net: pxa168_eth: set the mac address on the Ethernet controller

[Dan Carpenter]

On Fri, Oct 03, 2014 at 03:40:33PM +0200, Antoine Tenart wrote:
> 
> I'll cook up a fix for both of them if the solution's OK with you.

Fantastic.  Thanks for finding the spider_net issue as well!

regards,
dan carpenter