* [PATCH v4] staging: wlan-ng: replace memcpy with ether_addr_copy
@ 2015-10-02 10:52 Ioana Ciornei
2015-10-04 7:27 ` [Outreachy kernel] " Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: Ioana Ciornei @ 2015-10-02 10:52 UTC (permalink / raw)
To: outreachy-kernel; +Cc: Ioana Ciornei
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);
+ ether_addr_copy(p80211_hdr->a3.a2, wlandev->netdev->dev_addr);
+ ether_addr_copy(p80211_hdr->a3.a3, wlandev->bssid);
break;
case WLAN_MACMODE_ESS_STA:
fc |= cpu_to_le16(WLAN_SET_FC_TODS(1));
- memcpy(p80211_hdr->a3.a1, wlandev->bssid, ETH_ALEN);
- memcpy(p80211_hdr->a3.a2, wlandev->netdev->dev_addr, ETH_ALEN);
- memcpy(p80211_hdr->a3.a3, &e_hdr.daddr, ETH_ALEN);
+ ether_addr_copy(p80211_hdr->a3.a1, wlandev->bssid);
+ ether_addr_copy(p80211_hdr->a3.a2, wlandev->netdev->dev_addr);
+ ether_addr_copy(p80211_hdr->a3.a3, (const u8 *)&e_hdr.daddr);
break;
case WLAN_MACMODE_ESS_AP:
fc |= cpu_to_le16(WLAN_SET_FC_FROMDS(1));
- memcpy(p80211_hdr->a3.a1, &e_hdr.daddr, ETH_ALEN);
- memcpy(p80211_hdr->a3.a2, wlandev->bssid, ETH_ALEN);
- memcpy(p80211_hdr->a3.a3, &e_hdr.saddr, ETH_ALEN);
+ ether_addr_copy(p80211_hdr->a3.a1, (const u8 *)&e_hdr.daddr);
+ ether_addr_copy(p80211_hdr->a3.a2, wlandev->bssid);
+ ether_addr_copy(p80211_hdr->a3.a3, (const u8 *)&e_hdr.saddr);
break;
default:
netdev_err(wlandev->netdev,
@@ -243,7 +243,7 @@ static void orinoco_spy_gather(wlandevice_t *wlandev, char *mac,
for (i = 0; i < wlandev->spy_number; i++) {
if (!memcmp(wlandev->spy_address[i], mac, ETH_ALEN)) {
- memcpy(wlandev->spy_address[i], mac, ETH_ALEN);
+ ether_addr_copy(wlandev->spy_address[i], mac);
wlandev->spy_stat[i].level = rxmeta->signal;
wlandev->spy_stat[i].noise = rxmeta->noise;
wlandev->spy_stat[i].qual =
diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
index a9c1e0b..14e3418 100644
--- a/drivers/staging/wlan-ng/p80211netdev.c
+++ b/drivers/staging/wlan-ng/p80211netdev.c
@@ -644,7 +644,7 @@ static int p80211knetdev_set_mac_address(netdevice_t *dev, void *addr)
macaddr->status = P80211ENUM_msgitem_status_data_ok;
macaddr->len = sizeof(macaddr->data);
macaddr->data.len = ETH_ALEN;
- memcpy(&macaddr->data.data, new_addr->sa_data, ETH_ALEN);
+ ether_addr_copy((u8 *)&macaddr->data.data, new_addr->sa_data);
/* Set up the resultcode argument */
resultcode->did = DIDmsg_dot11req_mibset_resultcode;
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: wlan-ng: replace memcpy with ether_addr_copy
2015-10-02 10:52 [PATCH v4] staging: wlan-ng: replace memcpy with ether_addr_copy Ioana Ciornei
@ 2015-10-04 7:27 ` Greg KH
2015-10-06 7:50 ` Ioana Ciornei
0 siblings, 1 reply; 4+ messages in thread
From: Greg KH @ 2015-10-04 7:27 UTC (permalink / raw)
To: Ioana Ciornei; +Cc: outreachy-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: wlan-ng: replace memcpy with ether_addr_copy
2015-10-04 7:27 ` [Outreachy kernel] " Greg KH
@ 2015-10-06 7:50 ` Ioana Ciornei
2015-10-06 14:15 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: Ioana Ciornei @ 2015-10-06 7:50 UTC (permalink / raw)
To: Greg KH; +Cc: outreachy-kernel
[-- Attachment #1: Type: text/plain, Size: 5402 bytes --]
On Sun, Oct 4, 2015 at 10:27 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 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?
>
Changing the type of daddr and saddr fields from wlan_ethhdr to const is
not enough in this situation.
The type should be changed to const u8* from u8 (*) [6] but this introduces
other compile time warnings like 'discards ‘const’ qualifier from pointer
target type'.
Also, if I make the change, every time a variable of wlan_ethhdr is used
memory should be allocated for daddr and saddr fields.
In this circumstance, should I continue with the patchset?
Thanks,
Ioana
[-- Attachment #2: Type: text/html, Size: 7241 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH v4] staging: wlan-ng: replace memcpy with ether_addr_copy
2015-10-06 7:50 ` Ioana Ciornei
@ 2015-10-06 14:15 ` Greg KH
0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2015-10-06 14:15 UTC (permalink / raw)
To: Ioana Ciornei; +Cc: outreachy-kernel
On Tue, Oct 06, 2015 at 10:50:21AM +0300, Ioana Ciornei wrote:
>
>
> On Sun, Oct 4, 2015 at 10:27 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > 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?
> >
>
> Changing the type of daddr and saddr fields from wlan_ethhdr to const is not
> enough in this situation.
> The type should be changed to const u8* from u8 (*) [6] but this introduces
> other compile time warnings like 'discards ‘const’ qualifier from pointer
> target type'.
Then fix those up :)
> Also, if I make the change, every time a variable of wlan_ethhdr is used memory
> should be allocated for daddr and saddr fields.
Hm, no, that shouldn't be needed at all.
> In this circumstance, should I continue with the patchset?
I don't really know, that's up to you.
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-06 14:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-02 10:52 [PATCH v4] staging: wlan-ng: replace memcpy with ether_addr_copy Ioana Ciornei
2015-10-04 7:27 ` [Outreachy kernel] " Greg KH
2015-10-06 7:50 ` Ioana Ciornei
2015-10-06 14:15 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.