* [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).