staging: slicoss: Fix prefer ether_addr_copy over memcpy

PATCH In review Joel Pelaez Jorge 2 participants 3 messages Mar 18, 2014 Staging

lore

Share & tools

lore

Joel Pelaez Jorge

author

2014-03-18 05:04 UTC #0 lore raw

PATCH 1/1 staging: slicoss: Fix prefer ether_addr_copy over memcpy

This patch fixes the following checkpatch.pl issues caused by the new function: ether_addr_copy

Signed-off-by Joel Pelaez Jorge <[email protected]>

---
 drivers/staging/slicoss/slicoss.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/staging/slicoss/slicoss.c +10 ~~-10~~

index 12aafe3..0e0e374 100644
@@ -581,15 +581,15 @@ static void slic_adapter_set_hwaddr(struct adapter *adapter)
 	struct sliccard *card = adapter->card;
 
 	if ((adapter->card) && (card->config_set)) {
-		memcpy(adapter->macaddr,
-		       card->config.MacInfo[adapter->functionnumber].macaddrA,
-		       sizeof(struct slic_config_mac));
+		ether_addr_copy(adapter->macaddr,
+				card->config.MacInfo[adapter->functionnumber]
+				.macaddrA);
 		if (is_zero_ether_addr(adapter->currmacaddr))
-			memcpy(adapter->currmacaddr, adapter->macaddr,
-			       ETH_ALEN);
+			ether_addr_copy(adapter->currmacaddr,
+					adapter->macaddr);
 		if (adapter->netdev)
-			memcpy(adapter->netdev->dev_addr, adapter->currmacaddr,
-			       ETH_ALEN);
+			ether_addr_copy(adapter->netdev->dev_addr,
+					adapter->currmacaddr);
 	}
 }

@@ -810,8 +810,8 @@ static int slic_mac_set_address(struct net_device *dev, void *ptr)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EINVAL;
 
-	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
-	memcpy(adapter->currmacaddr, addr->sa_data, dev->addr_len);
+	ether_addr_copy(dev->dev_addr, addr->sa_data);
+	ether_addr_copy(adapter->currmacaddr, addr->sa_data);
 
 	slic_config_set(adapter, true);
 	return 0;

@@ -2313,7 +2313,7 @@ static int slic_mcast_add_list(struct adapter *adapter, char *address)
 	if (mcaddr == NULL)
 		return 1;
 
-	memcpy(mcaddr->address, address, ETH_ALEN);
+	ether_addr_copy(mcaddr->address, address);
 
 	mcaddr->next = adapter->mcastaddrs;
 	adapter->mcastaddrs = mcaddr;

Signature · 1 line

1.7.10.4

Jingoo Han

2014-03-18 05:49 UTC #1 lore raw

Quoted 15 lines · Joel Pelaez Jorge

This patch fixes the following checkpatch.pl issues caused by the new
function: ether_addr_copy
Signed-off-by Joel Pelaez Jorge <[email protected]>
---
  drivers/staging/slicoss/slicoss.c |   20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)
drivers/staging/slicoss/slicoss.c +0 -0
index 12aafe3..0e0e374 100644

drivers/staging/slicoss/slicoss.c static void slic_adapter_set_hwaddr(struct adapter · line 587

Quoted 10 lines

drivers/staging/slicoss/slicoss.c +3 -3

@@ -581,15 +581,15 @@ static void slic_adapter_set_hwaddr(struct adapter
 *adapter)
  	struct sliccard *card = adapter->card;
 
  	if ((adapter->card) && (card->config_set)) {
-		memcpy(adapter->macaddr,
-		       card->config.MacInfo[adapter->functionnumber].macaddrA,
-		       sizeof(struct slic_config_mac));
+		ether_addr_copy(adapter->macaddr,
+				card->config.MacInfo[adapter->functionnumber]
+				.macaddrA);

As declared in ./include/linux/etherdevice.h,
Copied size should be a six-byte.

static inline void ether_addr_copy(u8 *dst, const u8 *src) {

#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
        *(u32 *)dst = *(const u32 *)src;
        *(u16 *)(dst + 4) = *(const u16 *)(src + 4);
#else
        u16 *a = (u16 *)dst;
        const u16 *b = (const u16 *)src;

        a[0] = b[0];
        a[1] = b[1];
        a[2] = b[2];

#endif
}

In this case, sizeof(struct slic_config_mac) will be 6 byte, as below. So, it looks good.

./drivers/staging/slicoss/slichw.h

struct slic_config_mac {
        u8 macaddrA[6];
};

drivers/staging/slicoss/slicoss.c static void slic_adapter_set_hwaddr(struct adapter *adapter) · line 594

Quoted 12 lines

drivers/staging/slicoss/slicoss.c +4 -4

  		if (is_zero_ether_addr(adapter->currmacaddr))
-			memcpy(adapter->currmacaddr, adapter->macaddr,
-			       ETH_ALEN);
+			ether_addr_copy(adapter->currmacaddr,
+					adapter->macaddr);
  		if (adapter->netdev)
-			memcpy(adapter->netdev->dev_addr, adapter->currmacaddr,
-			       ETH_ALEN);
+			ether_addr_copy(adapter->netdev->dev_addr,
+					adapter->currmacaddr);
  	}
  }

ETH_ALEN is defined as 6. It looks good, too.

./include/uapi/linux/if_ether.h
#define ETH_ALEN 6

drivers/staging/slicoss/slicoss.c static int slic_mac_set_address(struct net_device · line 815

Quoted 10 lines

drivers/staging/slicoss/slicoss.c +2 -2

@@ -810,8 +810,8 @@ static int slic_mac_set_address(struct net_device
 *dev, void *ptr)
  	if (!is_valid_ether_addr(addr->sa_data))
  		return -EINVAL;
 
-	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
-	memcpy(adapter->currmacaddr, addr->sa_data, dev->addr_len);
+	ether_addr_copy(dev->dev_addr, addr->sa_data);
+	ether_addr_copy(adapter->currmacaddr, addr->sa_data);

By the way, I am wondering if 'dev->addr_len' is 6 bytes. Is there anyone who can confirm it?

If nobody can confirm 'dev->addr_len' is 6 bytes, it should not be changed to 'ether_addr_copy()'.

Best regards,
Jingoo Han

Quoted 15 lines

drivers/staging/slicoss/slicoss.c +1 -1

  	slic_config_set(adapter, true);
  	return 0;

@@ -2313,7 +2313,7 @@ static int slic_mcast_add_list(struct adapter
 *adapter, char *address)
  	if (mcaddr == NULL)
  		return 1;
 
-	memcpy(mcaddr->address, address, ETH_ALEN);
+	ether_addr_copy(mcaddr->address, address);
 
  	mcaddr->next = adapter->mcastaddrs;

adapter->mcastaddrs = mcaddr;
--
1.7.10.4

Joel Pelaez Jorge

author

2014-03-18 23:08 UTC #2 lore raw

Quoted 16 lines · Jingoo Han

drivers/staging/slicoss/slicoss.c static int slic_mac_set_address(struct net_device · line 815
Quoted 11 lines · Joel Pelaez Jorge
drivers/staging/slicoss/slicoss.c +2 -2
@@ -810,8 +810,8 @@ static int slic_mac_set_address(struct net_device
 *dev, void *ptr)
   	if (!is_valid_ether_addr(addr->sa_data))
   		return -EINVAL;
 
-	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
-	memcpy(adapter->currmacaddr, addr->sa_data, dev->addr_len);
+	ether_addr_copy(dev->dev_addr, addr->sa_data);
+	ether_addr_copy(adapter->currmacaddr, addr->sa_data);
By the way, I am wondering if 'dev->addr_len' is 6 bytes. Is there anyone who can confirm it?

Quoted 6 lines

If nobody can confirm 'dev->addr_len' is 6 bytes, it should not be changed to 'ether_addr_copy()'.
Best regards,
Jingoo Han

In the first case, is necessary use memcpy for copy te ethernet address to dev->dev_addr because this is a pointer, uses dev->addr_len for define size.

But in the second, adapter->currmacaddr is fixed to 6 size, so that here one can use ether_addr_copy without issues.

--
Best regards,
Joel Pelaez Jorge
Joel Pelaez Jorge