On Sun, Oct 11, 2015 at 1:52 AM, punit vara <punitv...@gmail.com> wrote:
> On Sun, Oct 11, 2015 at 1:20 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>>
>>
>> On Sun, 11 Oct 2015, punit vara wrote:
>>
>>> On Sat, Oct 10, 2015 at 11:04 PM, Larry Finger
>>> <larry.fin...@lwfinger.net> 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 <punitv...@gmail.com>
>>> >> ---
>>> >> 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.
Thanks Julia I didn't know about that dwarves 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, pnetwork->network.MacAddress)
I will learn debugging with help of pahole and again send the patch.
Sorry for inconvenience previous mail send before adding because my
laptop was hanged.
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
