Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition

[Michael Straube]

On 06/22/18 19:28, Joe Perches wrote:
On Fri, 2018-06-22 at 14:48 +0200, Michael Straube wrote:
On 06/22/18 12:57, Dan Carpenter wrote:
On Fri, Jun 22, 2018 at 03:54:22AM -0700, Joe Perches wrote:
On Fri, 2018-06-22 at 13:40 +0300, Dan Carpenter wrote:
On Thu, Jun 21, 2018 at 08:22:30PM +0200, Michael Straube wrote:
Fix checkpatch error 'do not use assignment in if condition'.
[]
diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c 
b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index e55895632921..87a4ced41028 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/
@@ -1181,9 +1181,8 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
             (mac[3] == 0xff) && (mac[4] == 0xff) && (mac[5] == 0xff)) ||
            ((mac[0] == 0x00) && (mac[1] == 0x00) && (mac[2] == 0x00) &&
             (mac[3] == 0x00) && (mac[4] == 0x00) && (mac[5] == 0x00))) {

Should also use is_broadcast_ether_addr and is_zero_ether_addr

-               if (np &&
-                   (addr = of_get_property(np, "local-mac-address", &len)) &&
-                   len == ETH_ALEN) {
+               addr = of_get_property(np, "local-mac-address", &len);
+               if (np && addr && len == ETH_ALEN) {

You can remove the "np" check.

                if (addr && len == ETH_ALEN) {

It looks more like the rewrite is incorrect
as np is tested before of_get_property


That's what I was worried about too, but if "np" is NULL then
of_get_property() just returns NULL so it's fine.

So it should be this?

          if (((mac[0] == 0xff) && (mac[1] == 0xff) && (mac[2] == 0xff) &&
               (mac[3] == 0xff) && (mac[4] == 0xff) && (mac[5] == 0xff)) ||
              ((mac[0] == 0x00) && (mac[1] == 0x00) && (mac[2] == 0x00) &&
               (mac[3] == 0x00) && (mac[4] == 0x00) && (mac[5] == 0x00)) &&
              (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac))) {

No as the mac[] tests are the same as is_<foo>_ether_addr

Ok, I understand now.

and there's nothing really objectionable about embedding
the assignment in the if here.

Output from checkpatch is not gospel and can be ignored
whenever appropriate.

Ok, good to know.

                memcpy(mac_addr, ""\x00\xe0\x4c\x87\x00\x00", ETH_ALEN);

Although the last memcpy of a fixed mac address could
probably use eth_random_addr to reduce the likelihood
of mac address collision so maybe

                eth_random_addr(mac_addr);
 
Using a random address would be preffered?


Thanks for your help and patience.
Michael