Re: [PATCH] staging: rtl8712: fix Prefer ether_addr_copy() over memcpy()

[Larry Finger <Larry.Finger@lwfinger.net>]

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