lkml.org 
[lkml]   [2014]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy
From
Date
(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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2014-03-28 14:41    [W:0.059 / U:14.784 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site