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

From: Larry Finger
Date: Thu Jan 29 2015 - 16:08:11 EST


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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/