Skip to contentSkip to conversation

Staging: rtl8712: Fix coding style warnings

PATCH Superseded Punit Vara 5 participants 16 messages Oct 10, 2015 – Oct 11, 2015 Staging

lore
Share & tools
lore

Punit Vara

author
#0 lore raw

cover Staging: rtl8712: Fix coding style warnings

This set of patch fixes following warnings reported by checkpatch.pl:

-Prefer eth_broadcast_addr() over memset()
-Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp() -Block comments use * on subsequent lines
-Block comments use a trailing */ on a separate line -Comparisons should place the constant on the right side of the test

Punit Vara (4):
Staging: rtl8712: Fix warning prefer eth_broadcast_addr() over
memset()
Staging: rtl8712: Use ether_addr_equal() over memcmp()
Staging: rtl8712: Coding style warnings fix for block comments
Staging: rtl8712: fix warning for placing constant on the right side
of test

 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 47 ++++++++++++++-------------
 1 file changed, 25 insertions(+), 22 deletions(-)

--
2.5.3

author
#1 lore raw

PATCH 1/4 Staging: rtl8712: Fix warning prefer eth_broadcast_addr() over memset()

This patch is to the rtl871x_ioctl_linux.c that fixes up following warning by checkpatch.pl:

-Prefer eth_broadcast_addr() over memset()

Signed-off-by Punit Vara <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -2
index 143be0f..2ba055d 100644
@@ -1811,8 +1811,7 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
 	if (param == NULL)
 		return -ENOMEM;
 	param->cmd = IEEE_CMD_SET_ENCRYPTION;
-	memset(param->sta_addr, 0xff, ETH_ALEN);
-
+	eth_broadcast_addr(param->sta_addr);
 	strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
 	if (pext->ext_flags & IW_ENCODE_EXT_GROUP_KEY)
 		param->u.crypt.set_tx = 0;
Signature · 1 line
2.5.3
author
#2 lore raw

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

In reply to Punit Vara

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 <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -1
index 2ba055d..1c9092e 100644
@@ -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);
Signature · 1 line
2.5.3
NAK
#3 lore raw

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

Quoted 19 lines · Punit Vara

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 <[email protected]>
---
  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +0 -0
index 2ba055d..1c9092e 100644

drivers/staging/rtl8712/rtl871x_ioctl_linux.c static int r871x_get_ap_info(struct net_device *dev, · line 2013

Quoted 8 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -1
@@ -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

author
#4 lore raw

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

Quoted 45 lines · Larry Finger
Quoted 30 lines · Punit Vara

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 <[email protected]>
---
  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -1
index 2ba055d..1c9092e 100644
@@ -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],

Quoted 2 lines
                               &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().

Quoted 9 lines

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 ?

#5 lore raw

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

Quoted 61 lines · punit vara
Quoted 45 lines · Larry Finger
Quoted 30 lines · Punit Vara

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 <[email protected]>
---
  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -1
index 2ba055d..1c9092e 100644
@@ -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],

Quoted 2 lines
                               &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().

Quoted 9 lines

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().

Quoted 6 lines

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

author
#6 lore raw

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

Quoted 70 lines · Julia Lawall
Quoted 66 lines · punit vara
Quoted 45 lines · Larry Finger
Quoted 30 lines · Punit Vara

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 <[email protected]>
---
  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -1
index 2ba055d..1c9092e 100644
@@ -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],

Quoted 2 lines
                               &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().

Quoted 9 lines

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))

Quoted 7 lines
Quoted 1 line

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.

author
#7 lore raw

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

Quoted 82 lines · punit vara
Quoted 70 lines · Julia Lawall
Quoted 66 lines · punit vara
Quoted 45 lines · Larry Finger
Quoted 30 lines · Punit Vara

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 <[email protected]>
---
  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -1
index 2ba055d..1c9092e 100644
@@ -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],

Quoted 2 lines
                               &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().

Quoted 9 lines

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))

Quoted 7 lines
Quoted 1 line

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.

Quoted 6 lines

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.

build bot
#8 lore raw

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

In reply to Punit Vara
config: x86_64-randconfig-x005-201541 (attached as .config) reproduce:

Hi Punit,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-x005-201541 (attached as .config) reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All error/warnings (new ones prefixed by >>):

In file included from include/linux/linkage.h:4:0,

                    from include/linux/preempt.h:9,
                    from include/linux/spinlock.h:50,
                    from drivers/staging/rtl8712/osdep_service.h:32,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:
   drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'r871x_get_ap_info':
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:32: error: incompatible type for argument 2 of 'ether_addr_equal'
      if (!ether_addr_equal(bssid, pnetwork->network)) {
                                   ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:3: note: in expansion of macro 'if'
      if (!ether_addr_equal(bssid, pnetwork->network)) {
      ^
   In file included from drivers/staging/rtl8712/osdep_service.h:39:0,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:
   include/linux/etherdevice.h:310:20: note: expected 'const u8 * {aka const unsigned char *}' but argument is of type 'struct wlan_bssid_ex'
    static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
                       ^
   In file included from include/linux/linkage.h:4:0,
                    from include/linux/preempt.h:9,
                    from include/linux/spinlock.h:50,
                    from drivers/staging/rtl8712/osdep_service.h:32,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:32: error: incompatible type for argument 2 of 'ether_addr_equal'
      if (!ether_addr_equal(bssid, pnetwork->network)) {
                                   ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:3: note: in expansion of macro 'if'
      if (!ether_addr_equal(bssid, pnetwork->network)) {
      ^
   In file included from drivers/staging/rtl8712/osdep_service.h:39:0,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:
   include/linux/etherdevice.h:310:20: note: expected 'const u8 * {aka const unsigned char *}' but argument is of type 'struct wlan_bssid_ex'
    static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
                       ^
   In file included from include/linux/linkage.h:4:0,
                    from include/linux/preempt.h:9,
                    from include/linux/spinlock.h:50,
                    from drivers/staging/rtl8712/osdep_service.h:32,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:32: error: incompatible type for argument 2 of 'ether_addr_equal'
      if (!ether_addr_equal(bssid, pnetwork->network)) {
                                   ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:3: note: in expansion of macro 'if'
      if (!ether_addr_equal(bssid, pnetwork->network)) {
      ^
   In file included from drivers/staging/rtl8712/osdep_service.h:39:0,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:

include/linux/etherdevice.h:310:20: note: expected 'const u8 * {aka const unsigned char *}' but argument is of type 'struct wlan_bssid_ex'
static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
^

vim +/ether_addr_equal +2010 drivers/staging/rtl8712/rtl871x_ioctl_linux.c

2004 (u8 *)data);
2005 spin_unlock_irqrestore(&(pmlmepriv->scanned_queue.lock),
2006 irqL);
2007 return -EINVAL;
2008 }
2009 netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);

> 2010			if (!ether_addr_equal(bssid, pnetwork->network)) {

2011 /* BSSID match, then check if supporting wpa/wpa2 */
2012 pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
2013 &wpa_ielen, pnetwork->network.IELength-12);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

build bot
#9 lore raw

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

In reply to Punit Vara
reproduce:

Hi Punit,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

reproduce:

        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

drivers/staging/rtl8712/rtl871x_ioctl_linux.c:227:15: sparse: cast to restricted __le16

>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: sparse: incorrect type in argument 2 (different base types)
   drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54:    expected unsigned char const [usertype] *addr2
   drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54:    got struct wlan_bssid_ex network
   drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'r871x_get_ap_info':
   drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:32: error: incompatible type for argument 2 of 'ether_addr_equal'
      if (!ether_addr_equal(bssid, pnetwork->network)) {
                                   ^
   In file included from drivers/staging/rtl8712/osdep_service.h:39:0,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:

include/linux/etherdevice.h:310:20: note: expected 'const u8 * {aka const unsigned char *}' but argument is of type 'struct wlan_bssid_ex'
static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
^

vim +2010 drivers/staging/rtl8712/rtl871x_ioctl_linux.c

1994 return -EINVAL;
1995 spin_lock_irqsave(&(pmlmepriv->scanned_queue.lock), irqL);
1996 phead = &queue->queue;
1997 plist = phead->next;
1998 while (1) {
1999 if (end_of_queue_search(phead, plist) == true)
2000 break;
2001 pnetwork = LIST_CONTAINOR(plist, struct wlan_network, list);
2002 if (hwaddr_aton_i(data, bssid)) {
2003 netdev_info(dev, "r8712u: Invalid BSSID '%s'.\n",
2004 (u8 *)data);
2005 spin_unlock_irqrestore(&(pmlmepriv->scanned_queue.lock),
2006 irqL);
2007 return -EINVAL;
2008 }
2009 netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);

> 2010			if (!ether_addr_equal(bssid, pnetwork->network)) {

2011 /* BSSID match, then check if supporting wpa/wpa2 */
2012 pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
2013 &wpa_ielen, pnetwork->network.IELength-12);
2014 if (pbuf && (wpa_ielen > 0)) {
2015 pdata->flags = 1;
2016 break;
2017 }
2018 pbuf = r8712_get_wpa2_ie(&pnetwork->network.IEs[12],

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

author
#10 lore raw

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

Quoted 54 lines · kbuild test robot

Hi Punit,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

reproduce:

        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

   drivers/staging/rtl8712/rtl871x_ioctl_linux.c:227:15: sparse: cast to restricted __le16
Quoted 1 line
Quoted 1 line

drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: sparse: incorrect type in argument 2 (different base types)

drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: expected unsigned char const [usertype] *addr2
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: got struct wlan_bssid_ex network
drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'r871x_get_ap_info':
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:32: error: incompatible type for argument 2 of 'ether_addr_equal'

      if (!ether_addr_equal(bssid, pnetwork->network)) {
                                   ^
   In file included from drivers/staging/rtl8712/osdep_service.h:39:0,
                    from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:

include/linux/etherdevice.h:310:20: note: expected 'const u8 * {aka const unsigned char *}' but argument is of type 'struct wlan_bssid_ex'
static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
^

vim +2010 drivers/staging/rtl8712/rtl871x_ioctl_linux.c

1994 return -EINVAL;

  1995          spin_lock_irqsave(&(pmlmepriv->scanned_queue.lock), irqL);
  1996          phead = &queue->queue;
  1997          plist = phead->next;
  1998          while (1) {
  1999                  if (end_of_queue_search(phead, plist) == true)
  2000                          break;
  2001                  pnetwork = LIST_CONTAINOR(plist, struct wlan_network, list);
  2002                  if (hwaddr_aton_i(data, bssid)) {
  2003                          netdev_info(dev, "r8712u: Invalid BSSID '%s'.\n",
  2004                                      (u8 *)data);
  2005                          spin_unlock_irqrestore(&(pmlmepriv->scanned_queue.lock),
  2006                                                 irqL);
  2007                          return -EINVAL;
  2008                  }
  2009                  netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
> 2010                  if (!ether_addr_equal(bssid, pnetwork->network)) {

2011 /* BSSID match, then check if supporting wpa/wpa2 */
2012 pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
2013 &wpa_ielen, pnetwork->network.IELength-12);
2014 if (pbuf && (wpa_ielen > 0)) {
2015 pdata->flags = 1;
2016 break;
2017 }
2018 pbuf = r8712_get_wpa2_ie(&pnetwork->network.IEs[12],

Quoted 3 lines

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

That error is because I forgot MacAddress that I have already mention. Julia I have used pahole as you suggested me
following are the two structure need to be consider for alignment.

struct iw_pmksa {

__u32                      cmd;                     /*     0     4 */
struct sockaddr            bssid;                /*     4    16 */
__u8                       pmkid[16];              /*    20    16 */

/* size: 36, cachelines: 1, members: 3 */
/* last cacheline: 36 bytes */
};

struct wlan_bssid_ex {

u32                        Length;                           /*     0     4 */
unsigned char              MacAddress[6];        /*     4     6 */
u8                         Reserved[2];                    /*    10     2 */
struct ndis_802_11_ssid    Ssid;                  /*    12    36 */
u32                        Privacy;                          /*    48     4 */
s32                        Rssi;                               /*    52     4 */

enum NDIS_802_11_NETWORK_TYPE NetworkTypeInUse; /* 56 4 */
struct NDIS_802_11_CONFIGURATION Configuration; /* 60 32 */
/* --- cacheline 1 boundary (64 bytes) was 28 bytes ago --- */
enum NDIS_802_11_NETWORK_INFRASTRUCTURE InfrastructureMode; /* 92 4 */

u8                         rates[16];              /*    96    16 */
u32                        IELength;             /*   112     4 */
u8                         IEs[768];               /*   116   768 */

/* --- cacheline 13 boundary (832 bytes) was 52 bytes ago --- */

/* size: 884, cachelines: 14, members: 12 */
/* last cacheline: 52 bytes */
};

As I understood both are not aligned to u16 so
ether_addr_equal_unaligned() should be used.

Here ether_addr_equal_unaligned() return true if both address are equal.

so here correct answer would be if(ether_addr_equal_unaligned(bssid, pnetwork->network.MacAddress)) ??

If I am getting wrong any feedback are welcome otherwise I will resend patch with correct modification.

#11 lore raw

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

Quoted 6 lines · punit vara

following are the two structure need to be consider for alignment.

struct iw_pmksa {

__u32                      cmd;                     /*     0     4 */
struct sockaddr            bssid;                /*     4    16 */
__u8                       pmkid[16];              /*    20    16 */

wrong bssid, bssid here is on the stack
after a char *, so bssid is aligned on a
pointer boundary, either 4 or 8.

Quoted 5 lines

/* size: 36, cachelines: 1, members: 3 */
/* last cacheline: 36 bytes */
};

struct wlan_bssid_ex {

[]

Quoted 4 lines

unsigned char MacAddress[6]; /* 4 6 */

As I understood both are not aligned to u16 so
ether_addr_equal_unaligned() should be used.

u16s are aligned when on any even address

So ether_addr_equal could be used.

author
#12 lore raw

PATCH 3/4 Staging: rtl8712: Coding style warnings fix for block comments

In reply to Punit Vara

This patch is to the rtl871x_ioctl_linux.c file that fixes up following warnings reported by checkpatch.pl :

-Block comments use * on subsequent lines -Block comments use a trailing */ on a separate line

Signed-off-by Punit Vara <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +15 -11
index 1c9092e..fc1028a 100644
@@ -797,13 +797,13 @@ static int r871x_wx_set_pmkid(struct net_device *dev,
 	int intReturn = false;
 
 /*
-	There are the BSSID information in the bssid.sa_data array.
-	If cmd is IW_PMKSA_FLUSH, it means the wpa_supplicant wants to clear
-	all the PMKID information. If cmd is IW_PMKSA_ADD, it means the
-	wpa_supplicant wants to add a PMKID/BSSID to driver.
-	If cmd is IW_PMKSA_REMOVE, it means the wpa_supplicant wants to
-	remove a PMKID/BSSID from driver.
-*/
+ *	There are the BSSID information in the bssid.sa_data array.
+ *	If cmd is IW_PMKSA_FLUSH, it means the wpa_supplicant wants to clear
+ *	all the PMKID information. If cmd is IW_PMKSA_ADD, it means the
+ *	wpa_supplicant wants to add a PMKID/BSSID to driver.
+ *	If cmd is IW_PMKSA_REMOVE, it means the wpa_supplicant wants to
+ *	remove a PMKID/BSSID from driver.
+ */
 	if (pPMK == NULL)
 		return -EINVAL;
 	memcpy(strIssueBssid, pPMK->bssid.sa_data, ETH_ALEN);

@@ -818,7 +818,8 @@ static int r871x_wx_set_pmkid(struct net_device *dev,
 			if (!memcmp(psecuritypriv->PMKIDList[j].Bssid,
 			    strIssueBssid, ETH_ALEN)) {
 				/* BSSID is matched, the same AP => rewrite
-				 * with new PMKID. */
+				 * with new PMKID.
+				 */
 				netdev_info(dev, "r8712u: %s: BSSID exists in the PMKList.\n",
 					    __func__);
 				memcpy(psecuritypriv->PMKIDList[j].PMKID,

@@ -850,7 +851,8 @@ static int r871x_wx_set_pmkid(struct net_device *dev,
 			if (!memcmp(psecuritypriv->PMKIDList[j].Bssid,
 			    strIssueBssid, ETH_ALEN)) {
 				/* BSSID is matched, the same AP => Remove
-				 * this PMKID information and reset it. */
+				 * this PMKID information and reset it.
+				 */
 				eth_zero_addr(psecuritypriv->PMKIDList[j].Bssid);
 				psecuritypriv->PMKIDList[j].bUsed = false;
 				break;

@@ -1587,7 +1589,8 @@ static int r8711_wx_set_enc(struct net_device *dev,
 	} else {
 		wep.KeyLength = 0;
 		if (keyindex_provided == 1) { /* set key_id only, no given
-					       * KeyMaterial(erq->length==0).*/
+					       * KeyMaterial(erq->length==0).
+					       */
 			padapter->securitypriv.PrivacyKeyIndex = key;
 			switch (padapter->securitypriv.DefKeylen[key]) {
 			case 5:

@@ -2240,7 +2243,8 @@ static iw_handler r8711_handlers[] = {
 	r8711_wx_set_wap,		/* SIOCSIWAP */
 	r8711_wx_get_wap,		/* SIOCGIWAP */
 	r871x_wx_set_mlme,		/* request MLME operation;
-					 *  uses struct iw_mlme */
+					 *  uses struct iw_mlme
+					 */
 	dummy,				/* SIOCGIWAPLIST -- deprecated */
 	r8711_wx_set_scan,		/* SIOCSIWSCAN */
 	r8711_wx_get_scan,		/* SIOCGIWSCAN */
Signature · 1 line
2.5.3
author
#13 lore raw

PATCH 4/4 Staging: rtl8712: fix warning for placing constant on the right side of test

In reply to Punit Vara

This patch is to the rtl871x_ioctl_linux.c file that fixes up following warnings reported by checkpatch.pl:

-Comparisons should place the constant on the right side of the test

Signed-off-by Punit Vara <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +8 -8
index fc1028a..e9852ae 100644
@@ -959,7 +959,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 	if (IS_ERR(ext))
 		return PTR_ERR(ext);
 
-	if (0 == strcasecmp(ext, "RSSI")) {
+	if (strcasecmp(ext, "RSSI") == 0) {
 		/*Return received signal strength indicator in -db for */
 		/* current AP */
 		/*<ssid> Rssi xx */

@@ -976,7 +976,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 		} else {
 			sprintf(ext, "OK");
 		}
-	} else if (0 == strcasecmp(ext, "LINKSPEED")) {
+	} else if (strcasecmp(ext, "LINKSPEED") == 0) {
 		/*Return link speed in MBPS */
 		/*LinkSpeed xx */
 		union iwreq_data wrqd;

@@ -984,30 +984,30 @@ static int r871x_wx_set_priv(struct net_device *dev,
 		int mbps;
 
 		ret_inner = r8711_wx_get_rate(dev, info, &wrqd, extra);
-		if (0 != ret_inner)
+		if (ret_inner != 0)
 			mbps = 0;
 		else
 			mbps = wrqd.bitrate.value / 1000000;
 		sprintf(ext, "LINKSPEED %d", mbps);
-	} else if (0 == strcasecmp(ext, "MACADDR")) {
+	} else if (strcasecmp(ext, "MACADDR") == 0) {
 		/*Return mac address of the station */
 		/* Macaddr = xx:xx:xx:xx:xx:xx */
 		sprintf(ext, "MACADDR = %pM", dev->dev_addr);
-	} else if (0 == strcasecmp(ext, "SCAN-ACTIVE")) {
+	} else if (strcasecmp(ext, "SCAN-ACTIVE") == 0) {
 		/*Set scan type to active */
 		/*OK if successful */
 		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 		pmlmepriv->passive_mode = 1;
 		sprintf(ext, "OK");
-	} else if (0 == strcasecmp(ext, "SCAN-PASSIVE")) {
+	} else if (strcasecmp(ext, "SCAN-PASSIVE") == 0) {
 		/*Set scan type to passive */
 		/*OK if successful */
 		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 		pmlmepriv->passive_mode = 0;
 		sprintf(ext, "OK");
-	} else if (0 == strncmp(ext, "DCE-E", 5)) {
+	} else if (strncmp(ext, "DCE-E", 5) == 0) {
 		/*Set scan type to passive */
 		/*OK if successful */
 		r8712_disconnectCtrlEx_cmd(padapter

@@ -1017,7 +1017,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 			, 5000 /*u32 firstStageTO */
 		);
 		sprintf(ext, "OK");
-	} else if (0 == strncmp(ext, "DCE-D", 5)) {
+	} else if (strncmp(ext, "DCE-D", 5) == 0) {
 		/*Set scan type to passive */
 		/*OK if successfu */
 		r8712_disconnectCtrlEx_cmd(padapter
Signature · 1 line
2.5.3
#14 lore raw

Re: [PATCH 4/4] Staging: rtl8712: fix warning for placing constant on the right side of test

Quoted 1 line · Punit Vara

This patch is to the rtl871x_ioctl_linux.c file that fixes up following

The entire above line doesn't give any information (except the file name, but that can easily be seen from what comes just below). A commit message that would get to the point (what you did) more quickly could be something
like: "Put constant on the right side of a test. Problem found using checkpatch."

julia

Quoted 80 lines

warnings reported by checkpatch.pl:

-Comparisons should place the constant on the right side of the test

Signed-off-by Punit Vara <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +8 -9
index fc1028a..e9852ae 100644
@@ -959,7 +959,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 	if (IS_ERR(ext))
 		return PTR_ERR(ext);
 
-	if (0 == strcasecmp(ext, "RSSI")) {
+	if (strcasecmp(ext, "RSSI") == 0) {
 		/*Return received signal strength indicator in -db for */
 		/* current AP */
 		/*<ssid> Rssi xx */

@@ -976,7 +976,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 		} else {
 			sprintf(ext, "OK");
 		}
-	} else if (0 == strcasecmp(ext, "LINKSPEED")) {
+	} else if (strcasecmp(ext, "LINKSPEED") == 0) {
 		/*Return link speed in MBPS */
 		/*LinkSpeed xx */
 		union iwreq_data wrqd;

@@ -984,30 +984,30 @@ static int r871x_wx_set_priv(struct net_device *dev,
 		int mbps;
 
 		ret_inner = r8711_wx_get_rate(dev, info, &wrqd, extra);
-		if (0 != ret_inner)
+		if (ret_inner != 0)
 			mbps = 0;
 		else
 			mbps = wrqd.bitrate.value / 1000000;
 		sprintf(ext, "LINKSPEED %d", mbps);
-	} else if (0 == strcasecmp(ext, "MACADDR")) {
+	} else if (strcasecmp(ext, "MACADDR") == 0) {
 		/*Return mac address of the station */
 		/* Macaddr = xx:xx:xx:xx:xx:xx */
 		sprintf(ext, "MACADDR = %pM", dev->dev_addr);
-	} else if (0 == strcasecmp(ext, "SCAN-ACTIVE")) {
+	} else if (strcasecmp(ext, "SCAN-ACTIVE") == 0) {
 		/*Set scan type to active */
 		/*OK if successful */
 		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 		pmlmepriv->passive_mode = 1;
 		sprintf(ext, "OK");
-	} else if (0 == strcasecmp(ext, "SCAN-PASSIVE")) {
+	} else if (strcasecmp(ext, "SCAN-PASSIVE") == 0) {
 		/*Set scan type to passive */
 		/*OK if successful */
 		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 		pmlmepriv->passive_mode = 0;
 		sprintf(ext, "OK");
-	} else if (0 == strncmp(ext, "DCE-E", 5)) {
+	} else if (strncmp(ext, "DCE-E", 5) == 0) {
 		/*Set scan type to passive */
 		/*OK if successful */
 		r8712_disconnectCtrlEx_cmd(padapter

@@ -1017,7 +1017,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 			, 5000 /*u32 firstStageTO */
 		);
 		sprintf(ext, "OK");
-	} else if (0 == strncmp(ext, "DCE-D", 5)) {
+	} else if (strncmp(ext, "DCE-D", 5) == 0) {
 		/*Set scan type to passive */
 		/*OK if successfu */
 		r8712_disconnectCtrlEx_cmd(padapter
--

2.5.3

author
#15 lore raw

Re: [PATCH 4/4] Staging: rtl8712: fix warning for placing constant on the right side of test

Quoted 86 lines · Julia Lawall
Quoted 1 line · Punit Vara

This patch is to the rtl871x_ioctl_linux.c file that fixes up following

The entire above line doesn't give any information (except the file name, but that can easily be seen from what comes just below). A commit message that would get to the point (what you did) more quickly could be something
like: "Put constant on the right side of a test. Problem found using checkpatch."

julia

Quoted 72 lines

warnings reported by checkpatch.pl:

-Comparisons should place the constant on the right side of the test

Signed-off-by Punit Vara <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +7 -8
index fc1028a..e9852ae 100644
@@ -959,7 +959,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
      if (IS_ERR(ext))
              return PTR_ERR(ext);
 
-     if (0 == strcasecmp(ext, "RSSI")) {
+     if (strcasecmp(ext, "RSSI") == 0) {
              /*Return received signal strength indicator in -db for */
              /* current AP */
              /*<ssid> Rssi xx */

@@ -976,7 +976,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
              } else {
                      sprintf(ext, "OK");
              }
-     } else if (0 == strcasecmp(ext, "LINKSPEED")) {
+     } else if (strcasecmp(ext, "LINKSPEED") == 0) {
              /*Return link speed in MBPS */
              /*LinkSpeed xx */
              union iwreq_data wrqd;

@@ -984,30 +984,30 @@ static int r871x_wx_set_priv(struct net_device *dev,
              int mbps;
 
              ret_inner = r8711_wx_get_rate(dev, info, &wrqd, extra);
-             if (0 != ret_inner)
+             if (ret_inner != 0)
                      mbps = 0;
              else
                      mbps = wrqd.bitrate.value / 1000000;
              sprintf(ext, "LINKSPEED %d", mbps);
-     } else if (0 == strcasecmp(ext, "MACADDR")) {
+     } else if (strcasecmp(ext, "MACADDR") == 0) {
              /*Return mac address of the station */
              /* Macaddr = xx:xx:xx:xx:xx:xx */
              sprintf(ext, "MACADDR = %pM", dev->dev_addr);
-     } else if (0 == strcasecmp(ext, "SCAN-ACTIVE")) {
+     } else if (strcasecmp(ext, "SCAN-ACTIVE") == 0) {
              /*Set scan type to active */
              /*OK if successful */
              struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
              pmlmepriv->passive_mode = 1;
              sprintf(ext, "OK");
-     } else if (0 == strcasecmp(ext, "SCAN-PASSIVE")) {
+     } else if (strcasecmp(ext, "SCAN-PASSIVE") == 0) {
              /*Set scan type to passive */
              /*OK if successful */
              struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
              pmlmepriv->passive_mode = 0;
              sprintf(ext, "OK");
-     } else if (0 == strncmp(ext, "DCE-E", 5)) {
+     } else if (strncmp(ext, "DCE-E", 5) == 0) {
              /*Set scan type to passive */
              /*OK if successful */
              r8712_disconnectCtrlEx_cmd(padapter

@@ -1017,7 +1017,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
                      , 5000 /*u32 firstStageTO */
              );
              sprintf(ext, "OK");
-     } else if (0 == strncmp(ext, "DCE-D", 5)) {
Quoted 8 lines
Quoted 8 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -1
+     } else if (strncmp(ext, "DCE-D", 5) == 0) {
              /*Set scan type to passive */
              /*OK if successfu */
              r8712_disconnectCtrlEx_cmd(padapter
--

2.5.3

I thought warning is clearly showing that constant should be on right side that's why I did not include any extra detail just include one warning in patch. But surely from next time I will follow your as per your valuable feedback.

Thanks
Punit Vara