[PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[Joel Peláez Jorge]

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

Signed-off-by: Joel Pelaez Jorge <joelpelaez@gmail.com>
---
diff --git a/drivers/staging/slicoss/slicoss.c 
b/drivers/staging/slicoss/slicoss.c
index 12aafe3..4ff39aa 100644
--- a/drivers/staging/slicoss/slicoss.c
+++ b/drivers/staging/slicoss/slicoss.c
@@ -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;

[PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[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 <joelpelaez@gmail.com>
---
diff --git a/drivers/staging/slicoss/slicoss.c 
b/drivers/staging/slicoss/slicoss.c
index 12aafe3..4ff39aa 100644
--- a/drivers/staging/slicoss/slicoss.c
+++ b/drivers/staging/slicoss/slicoss.c
@@ -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;

Re: [PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[Jingoo Han]

On Tuesday, March 18, 2014 12:18 PM, Joel Pelaez Jorge wrote:
> 
> This patch fixes the following checkpatch.pl issues caused by the new
> function: ether_addr_copy
> 
> Signed-off-by: Joel Pelaez Jorge <joelpelaez@gmail.com>
> ---
> diff --git a/drivers/staging/slicoss/slicoss.c
> b/drivers/staging/slicoss/slicoss.c
> index 12aafe3..4ff39aa 100644
> --- a/drivers/staging/slicoss/slicoss.c
> +++ b/drivers/staging/slicoss/slicoss.c
> @@ -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);

In addition to this, there are the same cases like this,
in this file as below. Please search other same cases.
For instance, 'grep' will be a good way to find it.

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));
                if (is_zero_ether_addr(adapter->currmacaddr))
                        memcpy(adapter->currmacaddr, adapter->macaddr,
                               ETH_ALEN);
                if (adapter->netdev)
                        memcpy(adapter->netdev->dev_addr, adapter->currmacaddr,
                               ETH_ALEN);
        }
}

Joe Perches,
These are not spotted by checkpatch.pl.
However, after modifying it as below, checkpatch warnings are
printed. Would you confirm it?

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


Best regards,
Jingoo Han


> 
>       mcaddr->next = adapter->mcastaddrs;
>       adapter->mcastaddrs = mcaddr;

Re: [PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[Joe Perches]

On Tue, 2014-03-18 at 12:38 +0900, Jingoo Han wrote:
> In addition to this, there are the same cases like this,
> in this file as below. Please search other same cases.
> For instance, 'grep' will be a good way to find it.
[]
> static void slic_adapter_set_hwaddr(struct adapter *adapter)
> {
>         struct sliccard *card = adapter->card;
[]
>                 if (is_zero_ether_addr(adapter->currmacaddr))
>                         memcpy(adapter->currmacaddr, adapter->macaddr,
>                                ETH_ALEN);
>                 if (adapter->netdev)
>                         memcpy(adapter->netdev->dev_addr, adapter->currmacaddr,
>                                ETH_ALEN);
>         }
> }
> 
> Joe Perches,
> These are not spotted by checkpatch.pl.
> However, after modifying it as below, checkpatch warnings are
> printed. Would you confirm it?

If it's not on a single line, checkpatch won't find it.

[PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[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 <joelpelaez@gmail.com>
---
  drivers/staging/slicoss/slicoss.c |   20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/slicoss/slicoss.c 
b/drivers/staging/slicoss/slicoss.c
index 12aafe3..0e0e374 100644
--- a/drivers/staging/slicoss/slicoss.c
+++ b/drivers/staging/slicoss/slicoss.c
@@ -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;
-- 
1.7.10.4

Re: [PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[Jingoo Han]

On Tuesday, March 18, 2014 2:04 PM, Joel Pelaez Jorge wrote:
> 
> This patch fixes the following checkpatch.pl issues caused by the new
> function: ether_addr_copy
> 
> Signed-off-by: Joel Pelaez Jorge <joelpelaez@gmail.com>
> ---
>   drivers/staging/slicoss/slicoss.c |   20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/slicoss/slicoss.c
> b/drivers/staging/slicoss/slicoss.c
> index 12aafe3..0e0e374 100644
> --- a/drivers/staging/slicoss/slicoss.c
> +++ b/drivers/staging/slicoss/slicoss.c
> @@ -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];
};

>   		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

> 
> @@ -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

> 
>   	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

Re: [PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[Greg Kroah-Hartman]

On Mon, Mar 17, 2014 at 09:17:35PM -0600, Joel Pelaez Jorge wrote:
> This patch fixes the following checkpatch.pl issues caused by the new
> function: ether_addr_copy
> 
> Signed-off-by: Joel Pelaez Jorge <joelpelaez@gmail.com>
> ---
> diff --git a/drivers/staging/slicoss/slicoss.c
> b/drivers/staging/slicoss/slicoss.c
> index 12aafe3..4ff39aa 100644
> --- a/drivers/staging/slicoss/slicoss.c
> +++ b/drivers/staging/slicoss/slicoss.c
> @@ -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;

The patch is line-wrapped, and tabs converted to spaces, making it
impossible to apply the patch.

Please fix your email client and try again.

greg k-h

Re: [PATCH] staging: slicoss: Fix prefer ether_addr_copy over memcpy

[Joel Pelaez Jorge]

El 17/03/14 23:49, Jingoo Han escribió:
> On Tuesday, March 18, 2014 2:04 PM, Joel Pelaez Jorge wrote:
>>
>>
>> @@ -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
>
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