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

[punit vara]

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 ?
--
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/