Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
On Fri, Jun 22, 2018 at 8:28 PM, Joe Perches 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
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__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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
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__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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
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] = ...
}
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition
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
> ---
> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8723bs: do not use assignment in if condition
Fix checkpatch error 'do not use assignment in if condition'.
Signed-off-by: Michael Straube
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
