Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-30 Thread Joe Perches 
On Sun, 2014-03-30 at 02:29 +0300, Dan Carpenter wrote:
 These days in the kernel we treat checkpatch.pl and GCC
 warnings the same so it sucks when they are something conditional.

Treating checkpatch messages like gcc compilation warnings
and failures has got to change.

There is _no way_ checkpatch can have no false positives.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-30 Thread Dan Carpenter 
On Sat, Mar 29, 2014 at 06:23:26PM -0700, Joe Perches wrote:
 On Sun, 2014-03-30 at 02:29 +0300, Dan Carpenter wrote:
  These days in the kernel we treat checkpatch.pl and GCC
  warnings the same so it sucks when they are something conditional.
 
 Treating checkpatch messages like gcc compilation warnings
 and failures has got to change.
 
 There is _no way_ checkpatch can have no false positives.
 

We could argue back and forth, but for now lets just revert the
ether_addr_copy() check because people ignore the alignement requirement
and it just encourages people to introduce bugs.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-28 Thread Dan Carpenter 
On Fri, Mar 14, 2014 at 12:39:11AM +0900, Jérôme Pinot wrote:
 On 03/13/14 02:28, Greg Kroah-Hartman wrote:
  On Thu, Mar 13, 2014 at 10:21:44AM +0900, Jérôme Pinot wrote:
 [...]
   diff --git a/drivers/staging/ozwpan/ozcdev.c 
   b/drivers/staging/ozwpan/ozcdev.c
   index 5de5981..10c0a96 100644
   --- a/drivers/staging/ozwpan/ozcdev.c
   +++ b/drivers/staging/ozwpan/ozcdev.c
   @@ -217,7 +217,7 @@ static int oz_set_active_pd(const u8 *addr)
 pd = oz_pd_find(addr);
 if (pd) {
 spin_lock_bh(g_cdev.lock);
   - memcpy(g_cdev.active_addr, addr, ETH_ALEN);
   + ether_addr_copy(g_cdev.active_addr, addr);
  
  Are you sure this will work?
 
 No. But the ozwpan driver uses already ether_addr_equal which is not
 alignment-safe. As
 https://www.kernel.org/doc/Documentation/unaligned-memory-access.txt 
 said:
 
 This alignment-unsafe function is still useful as it is a decent
 optimization for the cases when you can ensure alignment, which is
 true almost all of the time in ethernet networking context.
 
 I expected the maintainer to confirm/infirm this. I'm just seeing that's
 actually Chris Kelly who did write this part of code, so I'm CC'ing him
 too.
 

It is aligned ok, but don't rely on the maintainer to fix your bugs.
Don't send patches which you are not sure about.

Joe, this seems like a very bad warning message from checkpatch.pl
because people will constantly send us patches over and over which
introduce bugs and they rely on the maintainer to catch it every time.
Can we get rid of the warning or move it under --strict or something?

Do we have a mailing list yet for checkpatch issues?

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-28 Thread Joe Perches 
(adding Andrew Morton, David Miller and LKML to cc's)

On Fri, 2014-03-28 at 14:18 +0300, Dan Carpenter wrote:
 On Fri, Mar 14, 2014 at 12:39:11AM +0900, Jérôme Pinot wrote:
  On 03/13/14 02:28, Greg Kroah-Hartman wrote:
   On Thu, Mar 13, 2014 at 10:21:44AM +0900, Jérôme Pinot wrote:
  [...]
diff --git a/drivers/staging/ozwpan/ozcdev.c 
b/drivers/staging/ozwpan/ozcdev.c
index 5de5981..10c0a96 100644
--- a/drivers/staging/ozwpan/ozcdev.c
+++ b/drivers/staging/ozwpan/ozcdev.c
@@ -217,7 +217,7 @@ static int oz_set_active_pd(const u8 *addr)
pd = oz_pd_find(addr);
if (pd) {
spin_lock_bh(g_cdev.lock);
-   memcpy(g_cdev.active_addr, addr, ETH_ALEN);
+   ether_addr_copy(g_cdev.active_addr, addr);
   
   Are you sure this will work?
  
  No. But the ozwpan driver uses already ether_addr_equal which is not
  alignment-safe. As
  https://www.kernel.org/doc/Documentation/unaligned-memory-access.txt 
  said:
  
  This alignment-unsafe function is still useful as it is a decent
  optimization for the cases when you can ensure alignment, which is
  true almost all of the time in ethernet networking context.
  
  I expected the maintainer to confirm/infirm this. I'm just seeing that's
  actually Chris Kelly who did write this part of code, so I'm CC'ing him
  too.
  
 
 It is aligned ok, but don't rely on the maintainer to fix your bugs.
 Don't send patches which you are not sure about.
 
 Joe, this seems like a very bad warning message from checkpatch.pl
 because people will constantly send us patches over and over which
 introduce bugs and they rely on the maintainer to catch it every time.
 Can we get rid of the warning or move it under --strict or something?

Hi Dan.

Maybe.

The checkpatch message is:

Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)

My personal preference would be to add YA inline function
for unaligned copies ether_addr_copy_unaligned for symmetry
to ether_addr_equal_unaligned to etherdevice.h though.

Then the message could be changed to something like
Prefer ether_addr_copy[_unaligned] to memcpy(foo, bar, ETH_ALEN)

 Do we have a mailing list yet for checkpatch issues?

No and I'm not going to advocate for one. I think the
subscriber count would be about 4 people total.

You could add yourself to the checkpatch MAINTAINERS entry
if you want to see more of the patches and discussions.

They are pretty rare.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-17 Thread Rupesh Gujare 

On 13/03/14 01:21, Jérôme Pinot wrote:

This fixes the following issues detected by checkpatch.pl:

  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
  #220: FILE: drivers/staging/ozwpan/ozcdev.c:220:
  +  memcpy(g_cdev.active_addr, addr, ETH_ALEN);

  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
  #286: FILE: drivers/staging/ozwpan/ozcdev.c:286:
  +  memcpy(addr, g_cdev.active_addr, ETH_ALEN);

  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
  #176: FILE: drivers/staging/ozwpan/ozpd.c:176:
  +  memcpy(pd-mac_addr, mac_addr, ETH_ALEN);

  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
  #795: FILE: drivers/staging/ozwpan/ozproto.c:795:
  +  memcpy(addr[count++], pd-mac_addr, ETH_ALEN);

Signed-off-by: Jerome Pinot ngc...@gmail.com
---
  drivers/staging/ozwpan/ozcdev.c  | 4 ++--
  drivers/staging/ozwpan/ozpd.c| 2 +-
  drivers/staging/ozwpan/ozproto.c | 2 +-
  3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/ozwpan/ozcdev.c b/drivers/staging/ozwpan/ozcdev.c
index 5de5981..10c0a96 100644
--- a/drivers/staging/ozwpan/ozcdev.c
+++ b/drivers/staging/ozwpan/ozcdev.c
@@ -217,7 +217,7 @@ static int oz_set_active_pd(const u8 *addr)
pd = oz_pd_find(addr);
if (pd) {
spin_lock_bh(g_cdev.lock);
-   memcpy(g_cdev.active_addr, addr, ETH_ALEN);
+   ether_addr_copy(g_cdev.active_addr, addr);
old_pd = g_cdev.active_pd;
g_cdev.active_pd = pd;
spin_unlock_bh(g_cdev.lock);
@@ -283,7 +283,7 @@ static long oz_cdev_ioctl(struct file *filp, unsigned int 
cmd,
u8 addr[ETH_ALEN];
oz_dbg(ON, OZ_IOCTL_GET_ACTIVE_PD\n);
spin_lock_bh(g_cdev.lock);
-   memcpy(addr, g_cdev.active_addr, ETH_ALEN);
+   ether_addr_copy(addr, g_cdev.active_addr);
spin_unlock_bh(g_cdev.lock);
if (copy_to_user((void __user *)arg, addr, ETH_ALEN))
return -EFAULT;
diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c
index 29a23a3..4740238 100644
--- a/drivers/staging/ozwpan/ozpd.c
+++ b/drivers/staging/ozwpan/ozpd.c
@@ -173,7 +173,7 @@ struct oz_pd *oz_pd_alloc(const u8 *mac_addr)
pd-last_rx_pkt_num = 0x;
oz_pd_set_state(pd, OZ_PD_S_IDLE);
pd-max_tx_size = OZ_MAX_TX_SIZE;
-   memcpy(pd-mac_addr, mac_addr, ETH_ALEN);
+   ether_addr_copy(pd-mac_addr, mac_addr);
if (0 != oz_elt_buf_init(pd-elt_buff)) {
kfree(pd);
pd = NULL;
diff --git a/drivers/staging/ozwpan/ozproto.c b/drivers/staging/ozwpan/ozproto.c
index f09acd0..a7e953d 100644
--- a/drivers/staging/ozwpan/ozproto.c
+++ b/drivers/staging/ozwpan/ozproto.c
@@ -792,7 +792,7 @@ int oz_get_pd_list(struct oz_mac_addr *addr, int max_count)
if (count = max_count)
break;
pd = container_of(e, struct oz_pd, link);
-   memcpy(addr[count++], pd-mac_addr, ETH_ALEN);
+   ether_addr_copy(addr[count++], pd-mac_addr);
}
spin_unlock_bh(g_polling_lock);
return count;



Acked-by: Rupesh Gujare rupesh.guj...@atmel.com

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-13 Thread Jérôme Pinot 
On 03/13/14 02:28, Greg Kroah-Hartman wrote:
 On Thu, Mar 13, 2014 at 10:21:44AM +0900, Jérôme Pinot wrote:
[...]
  diff --git a/drivers/staging/ozwpan/ozcdev.c 
  b/drivers/staging/ozwpan/ozcdev.c
  index 5de5981..10c0a96 100644
  --- a/drivers/staging/ozwpan/ozcdev.c
  +++ b/drivers/staging/ozwpan/ozcdev.c
  @@ -217,7 +217,7 @@ static int oz_set_active_pd(const u8 *addr)
  pd = oz_pd_find(addr);
  if (pd) {
  spin_lock_bh(g_cdev.lock);
  -   memcpy(g_cdev.active_addr, addr, ETH_ALEN);
  +   ether_addr_copy(g_cdev.active_addr, addr);
 
 Are you sure this will work?

No. But the ozwpan driver uses already ether_addr_equal which is not
alignment-safe. As
https://www.kernel.org/doc/Documentation/unaligned-memory-access.txt 
said:

This alignment-unsafe function is still useful as it is a decent
optimization for the cases when you can ensure alignment, which is
true almost all of the time in ethernet networking context.

I expected the maintainer to confirm/infirm this. I'm just seeing that's
actually Chris Kelly who did write this part of code, so I'm CC'ing him
too.

 You have to check the alignment of the variable.
 
 Also, this breaks the build, you need to include some kind of .h file to
 get this to work, please ALWAYS test your patches to make sure they
 don't break things.

My bad, sorry. I'll send a better patch. Thanks for your time.

 greg k-h

-- 
Jérôme Pinot
http://ngc891.blogdns.net/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-12 Thread Jérôme Pinot 
This fixes the following issues detected by checkpatch.pl:

 WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
 #220: FILE: drivers/staging/ozwpan/ozcdev.c:220:
 +  memcpy(g_cdev.active_addr, addr, ETH_ALEN);

 WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
 #286: FILE: drivers/staging/ozwpan/ozcdev.c:286:
 +  memcpy(addr, g_cdev.active_addr, ETH_ALEN);

 WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
 #176: FILE: drivers/staging/ozwpan/ozpd.c:176:
 +  memcpy(pd-mac_addr, mac_addr, ETH_ALEN);

 WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are 
__aligned(2)
 #795: FILE: drivers/staging/ozwpan/ozproto.c:795:
 +  memcpy(addr[count++], pd-mac_addr, ETH_ALEN);

Signed-off-by: Jerome Pinot ngc...@gmail.com
---
 drivers/staging/ozwpan/ozcdev.c  | 4 ++--
 drivers/staging/ozwpan/ozpd.c| 2 +-
 drivers/staging/ozwpan/ozproto.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/ozwpan/ozcdev.c b/drivers/staging/ozwpan/ozcdev.c
index 5de5981..10c0a96 100644
--- a/drivers/staging/ozwpan/ozcdev.c
+++ b/drivers/staging/ozwpan/ozcdev.c
@@ -217,7 +217,7 @@ static int oz_set_active_pd(const u8 *addr)
pd = oz_pd_find(addr);
if (pd) {
spin_lock_bh(g_cdev.lock);
-   memcpy(g_cdev.active_addr, addr, ETH_ALEN);
+   ether_addr_copy(g_cdev.active_addr, addr);
old_pd = g_cdev.active_pd;
g_cdev.active_pd = pd;
spin_unlock_bh(g_cdev.lock);
@@ -283,7 +283,7 @@ static long oz_cdev_ioctl(struct file *filp, unsigned int 
cmd,
u8 addr[ETH_ALEN];
oz_dbg(ON, OZ_IOCTL_GET_ACTIVE_PD\n);
spin_lock_bh(g_cdev.lock);
-   memcpy(addr, g_cdev.active_addr, ETH_ALEN);
+   ether_addr_copy(addr, g_cdev.active_addr);
spin_unlock_bh(g_cdev.lock);
if (copy_to_user((void __user *)arg, addr, ETH_ALEN))
return -EFAULT;
diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c
index 29a23a3..4740238 100644
--- a/drivers/staging/ozwpan/ozpd.c
+++ b/drivers/staging/ozwpan/ozpd.c
@@ -173,7 +173,7 @@ struct oz_pd *oz_pd_alloc(const u8 *mac_addr)
pd-last_rx_pkt_num = 0x;
oz_pd_set_state(pd, OZ_PD_S_IDLE);
pd-max_tx_size = OZ_MAX_TX_SIZE;
-   memcpy(pd-mac_addr, mac_addr, ETH_ALEN);
+   ether_addr_copy(pd-mac_addr, mac_addr);
if (0 != oz_elt_buf_init(pd-elt_buff)) {
kfree(pd);
pd = NULL;
diff --git a/drivers/staging/ozwpan/ozproto.c b/drivers/staging/ozwpan/ozproto.c
index f09acd0..a7e953d 100644
--- a/drivers/staging/ozwpan/ozproto.c
+++ b/drivers/staging/ozwpan/ozproto.c
@@ -792,7 +792,7 @@ int oz_get_pd_list(struct oz_mac_addr *addr, int max_count)
if (count = max_count)
break;
pd = container_of(e, struct oz_pd, link);
-   memcpy(addr[count++], pd-mac_addr, ETH_ALEN);
+   ether_addr_copy(addr[count++], pd-mac_addr);
}
spin_unlock_bh(g_polling_lock);
return count;
-- 
1.8.4

-- 
Jérôme Pinot
http://ngc891.blogdns.net/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy

2014-03-12 Thread Greg Kroah-Hartman 
On Thu, Mar 13, 2014 at 10:21:44AM +0900, Jérôme Pinot wrote:
 This fixes the following issues detected by checkpatch.pl:
 
  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses 
 are __aligned(2)
  #220: FILE: drivers/staging/ozwpan/ozcdev.c:220:
  +  memcpy(g_cdev.active_addr, addr, ETH_ALEN);
 
  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses 
 are __aligned(2)
  #286: FILE: drivers/staging/ozwpan/ozcdev.c:286:
  +  memcpy(addr, g_cdev.active_addr, ETH_ALEN);
 
  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses 
 are __aligned(2)
  #176: FILE: drivers/staging/ozwpan/ozpd.c:176:
  +  memcpy(pd-mac_addr, mac_addr, ETH_ALEN);
 
  WARNING: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses 
 are __aligned(2)
  #795: FILE: drivers/staging/ozwpan/ozproto.c:795:
  +  memcpy(addr[count++], pd-mac_addr, ETH_ALEN);
 
 Signed-off-by: Jerome Pinot ngc...@gmail.com
 ---
  drivers/staging/ozwpan/ozcdev.c  | 4 ++--
  drivers/staging/ozwpan/ozpd.c| 2 +-
  drivers/staging/ozwpan/ozproto.c | 2 +-
  3 files changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/staging/ozwpan/ozcdev.c b/drivers/staging/ozwpan/ozcdev.c
 index 5de5981..10c0a96 100644
 --- a/drivers/staging/ozwpan/ozcdev.c
 +++ b/drivers/staging/ozwpan/ozcdev.c
 @@ -217,7 +217,7 @@ static int oz_set_active_pd(const u8 *addr)
   pd = oz_pd_find(addr);
   if (pd) {
   spin_lock_bh(g_cdev.lock);
 - memcpy(g_cdev.active_addr, addr, ETH_ALEN);
 + ether_addr_copy(g_cdev.active_addr, addr);

Are you sure this will work?

You have to check the alignment of the variable.

Also, this breaks the build, you need to include some kind of .h file to
get this to work, please ALWAYS test your patches to make sure they
don't break things.

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel