* Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy [not found] ` <20140328111801.GD6991@mwanda> @ 2014-03-28 13:08 ` Joe Perches 2014-03-29 23:29 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Joe Perches @ 2014-03-28 13:08 UTC (permalink / raw) To: Dan Carpenter Cc: Jérôme Pinot, Greg Kroah-Hartman, devel, Chris Kelly, Andrew Morton, David Miller, LKML (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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy 2014-03-28 13:08 ` [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy Joe Perches @ 2014-03-29 23:29 ` Dan Carpenter 2014-03-30 1:23 ` Joe Perches 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2014-03-29 23:29 UTC (permalink / raw) To: Joe Perches Cc: Jérôme Pinot, Greg Kroah-Hartman, devel, Chris Kelly, Andrew Morton, David Miller, LKML On Fri, Mar 28, 2014 at 06:08:27AM -0700, Joe Perches wrote: > (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)" > Sure if we had a function which didn't introduce bugs then that would be great. As it is submitters ignore the alignment requirements and rely on us to catch bugs. In Smatch, I constantly explain to people that I don't care about false positives because this is not GCC where you have to eliminate every warning. These days in the kernel we treat checkpatch.pl and GCC warnings the same so it sucks when they are something conditional. regards, dan carpenter ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy 2014-03-29 23:29 ` Dan Carpenter @ 2014-03-30 1:23 ` Joe Perches 2014-03-30 10:43 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Joe Perches @ 2014-03-30 1:23 UTC (permalink / raw) To: Dan Carpenter Cc: Jérôme Pinot, Greg Kroah-Hartman, devel, Chris Kelly, Andrew Morton, David Miller, LKML 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy 2014-03-30 1:23 ` Joe Perches @ 2014-03-30 10:43 ` Dan Carpenter 0 siblings, 0 replies; 4+ messages in thread From: Dan Carpenter @ 2014-03-30 10:43 UTC (permalink / raw) To: Joe Perches Cc: Jérôme Pinot, Greg Kroah-Hartman, devel, Andrew Morton, David Miller, LKML 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-03-30 10:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20140312100132.GA18954@star>
[not found] ` <20140312144533.GA3410@kroah.com>
[not found] ` <20140313012144.GD30138@star>
[not found] ` <20140313022821.GA9084@kroah.com>
[not found] ` <20140313153911.GA17881@star>
[not found] ` <20140328111801.GD6991@mwanda>
2014-03-28 13:08 ` [PATCH 3/3] staging/ozwpan: coding style ether_addr_copy Joe Perches
2014-03-29 23:29 ` Dan Carpenter
2014-03-30 1:23 ` Joe Perches
2014-03-30 10:43 ` Dan Carpenter
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.