Re: [Outreachy kernel] [PATCH v4] staging: wlan-ng: replace memcpy with ether_addr_copy

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Oct 02, 2015 at 01:52:10PM +0300, Ioana Ciornei wrote:
> Replace memcpy with ether_addr_copy since the output of pahole
> shows that the addresses are aligned.
> 
> struct p80211_hdr_a3 {
>         __le16                     fc;                   /*     0     2 */
>         u16                        dur;                  /*     2     2 */
>         u8                         a1[6];                /*     4     6 */
>         u8                         a2[6];                /*    10     6 */
>         u8                         a3[6];                /*    16     6 */
>         u16                        seq;                  /*    22     2 */
> 
>         /* size: 24, cachelines: 1, members: 6 */
>         /* last cacheline: 24 bytes */
> };
> 
> struct net_device {
>         char                       name[16];             /*     0    16 */
>         struct hlist_node          name_hlist;           /*    16    16 */
> 
>      ...............
> 
>  /* --- cacheline 12 boundary (768 bytes) --- */
>         long unsigned int          last_rx;              /*   768     8 */
>         unsigned char *            dev_addr;             /*   776     8 */
>         struct netdev_rx_queue *   _rx;                  /*   784     8 */
> 
>     .................
> };
> 
> struct wlandevice {
>         struct wlandevice *        next;                 /*     0     8 */
>     .................
>         u8                         bssid[6];             /*   128     6 */
>     .................
> };
> 
> struct net_device {
>         char                       name[16];             /*     0    16 */
>     .................
>         unsigned char *            dev_addr;             /*   776     8 */
>     ................
> };
> 
> struct wlan_ethhdr {
>         u8                         daddr[6];             /*     0     6 */
>         u8                         saddr[6];             /*     6     6 */
>         u16                        type;                 /*    12     2 */
> 
>         /* size: 14, cachelines: 1, members: 3 */
>         /* last cacheline: 14 bytes */
> };
> 
> struct sockaddr {
>         sa_family_t                sa_family;            /*     0     2 */
>         char                       sa_data[14];          /*     2    14 */
> 
>         /* size: 16, cachelines: 1, members: 2 */
>         /* last cacheline: 16 bytes */
> };
> 
> struct p80211pstr6 {
>         u8                         len;                  /*     0     1 */
>         u8                         data[6];              /*     1     6 */
> 
>         /* size: 7, cachelines: 1, members: 2 */
>         /* last cacheline: 7 bytes */
> };
> 
> Coccinelle semantic patch:
> 
> @@
> expression E1,E2;
> @@
> 
> - memcpy(E1, E2, 6)
> + ether_addr_copy(E1, E2)
> 
> @@
> expression E1,E2;
> @@
> 
> - memcpy(E1, E2, ETH_ALEN)
> + ether_addr_copy(E1, E2)
> 
> Signed-off-by: Ioana Ciornei <ciorneiioana@gmail.com>
> ---
> Changes in v2:
>     - revised the cocci patch to be a more constrainting one
>     - applied cocci patch on the entire wlan-ng folder
>     - updated the patch description with further pahole output
> 
> Changes in v3:
>     - make an explicit cast from u8 (*)[6] to const u8 * to avoid compile warning
> 
> Changes in v4:
>     - fix compile and checkpatch warnings introduced in v3
> 
>  drivers/staging/wlan-ng/p80211conv.c   | 20 ++++++++++----------
>  drivers/staging/wlan-ng/p80211netdev.c |  2 +-
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211conv.c b/drivers/staging/wlan-ng/p80211conv.c
> index 1b02cdf..87d323a 100644
> --- a/drivers/staging/wlan-ng/p80211conv.c
> +++ b/drivers/staging/wlan-ng/p80211conv.c
> @@ -178,21 +178,21 @@ int skb_ether_to_p80211(wlandevice_t *wlandev, u32 ethconv,
>  
>  	switch (wlandev->macmode) {
>  	case WLAN_MACMODE_IBSS_STA:
> -		memcpy(p80211_hdr->a3.a1, &e_hdr.daddr, ETH_ALEN);
> -		memcpy(p80211_hdr->a3.a2, wlandev->netdev->dev_addr, ETH_ALEN);
> -		memcpy(p80211_hdr->a3.a3, wlandev->bssid, ETH_ALEN);
> +		ether_addr_copy(p80211_hdr->a3.a1, (const u8 *)&e_hdr.daddr);

When you have to add a cast like this, it's a good idea to try to figure
out _why_ that cast is needed.

I think you should just make the struct wlan_ethhdr daddr field be const
and then you don't need to make any casts when you convert to
ether_addr_copy().

So can you turn this into a two patch series?  The first one change the
structure variable type, and the second one this ether_addr_copy()
change?

thanks,

greg k-h