Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp()

[punit vara]

On Sun, Oct 11, 2015 at 1:20 AM, Julia Lawall <julia.lawall@xxxxxxx> wrote:
>
>
> On Sun, 11 Oct 2015, punit vara wrote:
>
>> On Sat, Oct 10, 2015 at 11:04 PM, Larry Finger
>> <Larry.Finger@xxxxxxxxxxxx> wrote:
>> > On 10/10/2015 11:58 AM, Punit Vara wrote:
>> >>
>> >> This patch is to the rtl871x_ioctl_linux.c file that fixes up following
>> >> warning reported by checkpatch.pl :
>> >>
>> >> -Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
>> >>
>> >> bssid has datatype u8 and pnetwork->network.MacAddress has data type
>> >>   unsigned char.
>> >>
>> >> Signed-off-by: Punit Vara <punitvara@xxxxxxxxx>
>> >> ---
>> >>   drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
>> >>   1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> index 2ba055d..1c9092e 100644
>> >> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> @@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
>> >>                         return -EINVAL;
>> >>                 }
>> >>                 netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
>> >> -               if (!memcmp(bssid, pnetwork->network.MacAddress,
>> >> ETH_ALEN)) {
>> >> +               if (!ether_addr_equal(bssid, pnetwork->network)) {
>> >>                         /* BSSID match, then check if supporting wpa/wpa2
>> >> */
>> >>                         pbuf =
>> >> r8712_get_wpa_ie(&pnetwork->network.IEs[12],
>> >>                                &wpa_ielen, pnetwork->network.IELength-12);
>> >>
>> >
>> > The types of the variables are not what is important - it is the alignment!
>> >
>> > I suggest that you read the definition of ether_addr_equal() and consider
>> > what happens if either of the two addresses is not aligned to u16!
>> >
>> > You also have a logic error. Routine memcmp() returns zero when the two
>> > arguments are equal. Thus !memcmp() will be true when they are equal.
>> > Routine ether_addr_equal() returns true when the arguments are equal.
>> > Whenever !memcmp() is correct and the arguments are aligned, then you need
>> > to replace it with ether_addr_equal().
>> >
>> > The checkpatch script is a good tool for locating things to be changed;
>> > however, if the change is other than cosmetic, you *MUST THINK* about what
>> > is happening. Yes, rtl8712 is ugly code, but it works. Please do not break
>> > it.
>> >
>> > NACK.
>> >
>> > Larry
>> >
>>
>> @Larry I am thankful for your high quality feedback.
>> if both address are aligned to u16 then we should use
>> ether_addr_equal() if not then ether_addr_equal_unaligned().
>>
>> Both return true if addr1 and addr2 matches. If I am understand
>> correctly bssid and MacAdress is not aligned to u16 .So right
>> correction would be
>> if(ether_addr_equal_unaligned(bssid, pnetwork->network))
>>
>> right ?
>
> You can use pahole to check the alignment.  Use git log to find other
> patches that mention the use of this tool to see what information to
> provide in the commit message.
>
> julia

Thanks Julia I didn't know about that dwarf utility I will go through
it .sorry I have forgot to write MacAddress.

So here solution could be either

ether_addr_equal(bssid, pnetwork->network.MacAddress) or
ether_addr_equal_unaligned(bssid->network.MacAddress)

I will learn debugging with help of pahole and again send the patch.
--
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/