Messages in this thread | | | Subject | Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy | From | Joe Perches <> | Date | Fri, 28 Mar 2014 06:08:27 -0700 |
| |
(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/
| |