On Thu, Jan 29, 2015 at 02:51:57PM +0300, Dan Carpenter wrote: > On Wed, Jan 28, 2015 at 11:30:11PM +0200, Aya Mahfouz wrote: > > On Wed, Jan 28, 2015 at 10:48:40AM -0600, Larry Finger wrote: > > > On 01/28/2015 09:53 AM, Heba Aamer wrote: > > > >This patch fixes the following checkpatch.pl warning: > > > >Prefer ether_addr_copy() over memcpy() > > > >if the Ethernet addresses are __aligned(2) > > > > > > > >I used the following coccinelle script: > > > > > > > >@@ > > > >expression E1,E2;constant E3; > > > >@@ > > > > > > > >- memcpy(E1, E2, E3) > > > >+ ether_addr_copy(E1, E2) > > > > > > > > > > > >pahole showed that the used structs are aligned to u16. > > > > > > I think you can stop here. The commit message is much too long for a 2-line patch. > > > > > > BTW, have you tested this patch? In particular, it needs to be tested on an > > > architecture where alignment is important. Using x86 is not sufficient. The > > > reason I ask is that there have been a lot of patches lately that change > > > locking and alignment issues that are only build tested, and have never been > > > tested with real hardware on any platform. > > > > > > One other thing, checkpatch only suggests that this change should be made. > > > It is certainly not mandatory. As you have not indicated that it has been > > > tested, > > > > > > NACK > > > > > > Larry > > > > > Hello Larry, > > > > Thank you for your patience. Heba has submitted this patch as part > > of a workshop she currently attends. She has checked the alignment > > through pahole and it showed that the variables of complex structs > > are aligned. She has attached the output of pahole, so that the > > community can verify her results and hence the lengthy output. > > > > She can also cross compile the kernel and verify the output for > > other architectures using pahole. Kindly let us know if this suits > > you. And please name any specific architecture that you would to see > > tested. If this is still not enough from your point of view, let > > us know what should be done further to verify the correctness of > > the patch. > > > > Really, I hate this checkpatch.pl warning, too. The patches are > difficult to review because you need a lot of context and there is a > small chance that the patch will introduce a bug. > > I was the person who introduced the rule that the patch submitter has to > prove the alignment is correct after two people told me basically that, > "The patch submitter's job is to sed the code and the maintainer's job > is to review the code." > > In this case we don't really need to use pahole. "mac" is a 6 byte > char array declared on the stack after a couple of integers. > pnetdev->dev_addr is a pointer. &pdata[0x12] is a pointer plus an even > offset. This patch is fine. But the changelog is too long and has a > lot of not at all relevant output from pahole. > Thanks for your analysis. > It's not really a practical thing to say that the patch writer has to > cross compile on a different arch. > I was trying to make ends meet. Thanks for the advice and ruling out a very difficult option. > regards, > dan carpenter > Heba, kindly resend the patch with an adjusted description. Include the relevant blocks of any struct and state more details in the last sentence. Kind Regards, Aya Saif El-yazal Mahfouz _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
