Messages in this thread | | | Date | Thu, 29 Jan 2015 15:08:02 -0600 | From | Larry Finger <> | Subject | Re: [PATCH] staging: rtl8712: fix Prefer ether_addr_copy() over memcpy() |
| |
On 01/29/2015 01:54 PM, Aya Mahfouz wrote: > 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.
I agree; however, keep the commit message short. I think stating that any members of a struct have been checked with pahole will be suffifient. Obviously, the local arrays will be aligned correctly.
Larry
| |