* [PATCH] staging: rtl8723bs: do not use assignment in if condition @ 2018-06-21 18:22 Michael Straube 2018-06-22 10:40 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Michael Straube @ 2018-06-21 18:22 UTC (permalink / raw) To: gregkh; +Cc: devel, linux-kernel, Michael Straube Fix checkpatch error 'do not use assignment in if condition'. Signed-off-by: Michael Straube <michael.straube@posteo.de> --- drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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/drivers/staging/rtl8723bs/core/rtw_ieee80211.c @@ -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))) { - 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) { memcpy(mac_addr, addr, ETH_ALEN); } else { mac[0] = 0x00; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-21 18:22 [PATCH] staging: rtl8723bs: do not use assignment in if condition Michael Straube @ 2018-06-22 10:40 ` Dan Carpenter 2018-06-22 10:54 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2018-06-22 10:40 UTC (permalink / raw) To: Michael Straube; +Cc: gregkh, devel, linux-kernel On Thu, Jun 21, 2018 at 08:22:30PM +0200, Michael Straube wrote: > Fix checkpatch error 'do not use assignment in if condition'. > > Signed-off-by: Michael Straube <michael.straube@posteo.de> > --- > drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > 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/drivers/staging/rtl8723bs/core/rtw_ieee80211.c > @@ -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))) { > - 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) { regards, dan carpenter ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-22 10:40 ` Dan Carpenter @ 2018-06-22 10:54 ` Joe Perches 2018-06-22 10:57 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2018-06-22 10:54 UTC (permalink / raw) To: Dan Carpenter, Michael Straube; +Cc: gregkh, devel, linux-kernel 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 It could be written something like: if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) { bool assigned = false; if (np) { addr = of_get_property(np, "local-mac-address", &len); if (len == ETH_ALEN) { memcpy(mac_addr, addr, ETH_ALEN); assigned = true; } } if (!assigned) { mac_addr[0] = 0x00; mac_addr[1] = ... } } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-22 10:54 ` Joe Perches @ 2018-06-22 10:57 ` Dan Carpenter 2018-06-22 12:48 ` Michael Straube 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2018-06-22 10:57 UTC (permalink / raw) To: Joe Perches; +Cc: Michael Straube, gregkh, devel, linux-kernel 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. regards, dan carpenter regards, dan carpenter ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-22 10:57 ` Dan Carpenter @ 2018-06-22 12:48 ` Michael Straube 2018-06-22 17:28 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Michael Straube @ 2018-06-22 12:48 UTC (permalink / raw) To: Dan Carpenter, Joe Perches; +Cc: gregkh, devel, linux-kernel 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))) { addr = of_get_property(np, "local-mac-address", &len); if (addr && len == ETH_ALEN) { memcpy(mac_addr, addr, ETH_ALEN); } else { mac[0] = 0x00; ... } } If yes, I'm not sure how to proceed as these are the very first patches I send. Should I send a v2 patch with both changes or just a v2 with "np" removed and another one for adding 'is_broadcast_ether_addr' and 'is_zero_ether_addr' checks? Regards, Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-22 12:48 ` Michael Straube @ 2018-06-22 17:28 ` Joe Perches 2018-06-22 19:11 ` Michael Straube 2018-06-25 9:49 ` Andy Shevchenko 0 siblings, 2 replies; 9+ messages in thread From: Joe Perches @ 2018-06-22 17:28 UTC (permalink / raw) To: Michael Straube, Dan Carpenter; +Cc: gregkh, devel, linux-kernel 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 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. I think the below is ok: if ((is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) && ((addr = of_get_property(np, "local-mac-address", &len)) && len == ETH_ALEN)) memcpy(mac_addr, addr, ETH_ALEN); else 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 if ((is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) && ((addr = of_get_property(np, "local-mac-address", &len)) && len == ETH_ALEN)) memcpy(mac_addr, addr, ETH_ALEN); else eth_random_addr(mac_addr); > If yes, I'm not sure how to proceed as these are the very first patches I send. > Should I send a v2 patch with both changes or just a v2 with "np" removed and > another one for adding 'is_broadcast_ether_addr' and 'is_zero_ether_addr' checks? I'd send 1 patch. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-22 17:28 ` Joe Perches @ 2018-06-22 19:11 ` Michael Straube 2018-06-23 1:59 ` Joe Perches 2018-06-25 9:49 ` Andy Shevchenko 1 sibling, 1 reply; 9+ messages in thread From: Michael Straube @ 2018-06-22 19:11 UTC (permalink / raw) To: Joe Perches, Dan Carpenter; +Cc: gregkh, devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-22 19:11 ` Michael Straube @ 2018-06-23 1:59 ` Joe Perches 0 siblings, 0 replies; 9+ messages in thread From: Joe Perches @ 2018-06-23 1:59 UTC (permalink / raw) To: Michael Straube, Dan Carpenter; +Cc: gregkh, devel, linux-kernel On Fri, 2018-06-22 at 21:11 +0200, Michael Straube wrote: > On 06/22/18 19:28, Joe Perches wrote: > > 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? mac address duplication is bad, so if there were 2 instances of this device on the same ethernet network, yes. > > Thanks for your help and patience. > Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition 2018-06-22 17:28 ` Joe Perches 2018-06-22 19:11 ` Michael Straube @ 2018-06-25 9:49 ` Andy Shevchenko 1 sibling, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2018-06-25 9:49 UTC (permalink / raw) To: Joe Perches Cc: Michael Straube, Dan Carpenter, Greg Kroah-Hartman, devel, Linux Kernel Mailing List On Fri, Jun 22, 2018 at 8:28 PM, Joe Perches <joe@perches.com> wrote: > On Fri, 2018-06-22 at 14:48 +0200, Michael Straube wrote: >> On 06/22/18 12:57, Dan Carpenter wrote: > Output from checkpatch is not gospel and can be ignored > whenever appropriate. > > I think the below is ok: > > if ((is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) && > ((addr = of_get_property(np, "local-mac-address", &len)) && > len == ETH_ALEN)) > memcpy(mac_addr, addr, ETH_ALEN); > else > 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 ...and first one looks like ether_addr_copy(). > so maybe > > if ((is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) && > ((addr = of_get_property(np, "local-mac-address", &len)) && > > len == ETH_ALEN)) > memcpy(mac_addr, addr, ETH_ALEN); > else > eth_random_addr(mac_addr); > >> If yes, I'm not sure how to proceed as these are the very first patches I send. >> Should I send a v2 patch with both changes or just a v2 with "np" removed and >> another one for adding 'is_broadcast_ether_addr' and 'is_zero_ether_addr' checks? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-06-25 9:49 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-21 18:22 [PATCH] staging: rtl8723bs: do not use assignment in if condition Michael Straube 2018-06-22 10:40 ` Dan Carpenter 2018-06-22 10:54 ` Joe Perches 2018-06-22 10:57 ` Dan Carpenter 2018-06-22 12:48 ` Michael Straube 2018-06-22 17:28 ` Joe Perches 2018-06-22 19:11 ` Michael Straube 2018-06-23 1:59 ` Joe Perches 2018-06-25 9:49 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).