[PATCH v3 1/4] staging: rtl8723bs: fix comparsion to NULL - coding style

[PATCH v3 1/4] staging: rtl8723bs: fix comparsion to NULL - coding style

[Michael Straube]

Fix comparsion to NULL issues found by checkpatch.
Use !x instead of x == NULL.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index 0822e440204e..e55895632921 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -747,7 +747,7 @@ u8 rtw_is_wps_ie(u8 *ie_ptr, uint *wps_ielen)
 	u8 match = false;
 	u8 eid, wps_oui[4] = {0x0, 0x50, 0xf2, 0x04};
 
-	if (ie_ptr == NULL)
+	if (!ie_ptr)
 		return match;
 
 	eid = ie_ptr[0];
@@ -1163,7 +1163,7 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
 	const unsigned char *addr;
 	int len;
 
-	if (mac_addr == NULL)
+	if (!mac_addr)
 		return;
 
 	if (rtw_initmac) {	/* 	Users specify the mac address */
-- 
2.18.0

[PATCH v3 2/4] staging: rtl8723bs: refactor rtw_macaddr_cfg()

[Michael Straube]

Using is_broadcast_ether_addr() and is_zero_ether_addr() instead of
testing each byte of the mac[] array for 0xff and 0x00 shortens the
code and improves readability.

If np == NULL, of_get_property() returns NULL, hence the "np" check
is not needed.

Instead of a fixed default mac address use a random one to reduce the
likelihood of mac address collision.

Thanks to Joe Perches and Dan Carpenter.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index e55895632921..7aa00d1391f7 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -1177,24 +1177,13 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
 		memcpy(mac, mac_addr, ETH_ALEN);
 	}
 
-	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))) {
-		if (np &&
-		    (addr = of_get_property(np, "local-mac-address", &len)) &&
+	if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
+		if ((addr = of_get_property(np, "local-mac-address", &len)) &&
 		    len == ETH_ALEN) {
 			memcpy(mac_addr, addr, ETH_ALEN);
 		} else {
-			mac[0] = 0x00;
-			mac[1] = 0xe0;
-			mac[2] = 0x4c;
-			mac[3] = 0x87;
-			mac[4] = 0x00;
-			mac[5] = 0x00;
-			/*  use default mac addresss */
-			memcpy(mac_addr, mac, ETH_ALEN);
-			DBG_871X("MAC Address from efuse error, assign default one !!!\n");
+			eth_random_addr(mac_addr);
+			DBG_871X("MAC Address from efuse error, assign random one !!!\n");
 		}
 	}
 
-- 
2.18.0

[PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Michael Straube]

Use ether_addr_copy() instead of memcpy() to copy the mac address.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index 7aa00d1391f7..8af4a89e632f 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -1172,15 +1172,15 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
 		for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
 			mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
 		}
-		memcpy(mac_addr, mac, ETH_ALEN);
+		ether_addr_copy(mac_addr, mac);
 	} else{	/* 	Use the mac address stored in the Efuse */
-		memcpy(mac, mac_addr, ETH_ALEN);
+		ether_addr_copy(mac, mac_addr);
 	}
 
 	if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
 		if ((addr = of_get_property(np, "local-mac-address", &len)) &&
 		    len == ETH_ALEN) {
-			memcpy(mac_addr, addr, ETH_ALEN);
+			ether_addr_copy(mac_addr, addr);
 		} else {
 			eth_random_addr(mac_addr);
 			DBG_871X("MAC Address from efuse error, assign random one !!!\n");
-- 
2.18.0

[PATCH v3 4/4] staging: rtl8723bs: use mac_pton() in rtw_macaddr_cfg()

[Michael Straube]

Use the mac_pton() helper to convert the mac address string.

The functions key_char2num() and key_2char2num() are not used
anywhere else and can be removed.

This also has the benefit of validating the input since mac_pton()
returns false if the string is not valid.

Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 .../staging/rtl8723bs/core/rtw_ieee80211.c    | 30 +++----------------
 .../staging/rtl8723bs/os_dep/ioctl_linux.c    |  3 --
 2 files changed, 4 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index 8af4a89e632f..8e0025e1ff14 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -1137,25 +1137,6 @@ ParseRes rtw_ieee802_11_parse_elems(u8 *start, uint len,
 
 }
 
-static u8 key_char2num(u8 ch);
-static u8 key_char2num(u8 ch)
-{
-		if ((ch >= '0') && (ch <= '9'))
-			return ch - '0';
-		else if ((ch >= 'a') && (ch <= 'f'))
-			return ch - 'a' + 10;
-		else if ((ch >= 'A') && (ch <= 'F'))
-			return ch - 'A' + 10;
-		else
-			return 0xff;
-}
-
-u8 key_2char2num(u8 hch, u8 lch);
-u8 key_2char2num(u8 hch, u8 lch)
-{
-		return ((key_char2num(hch) << 4) | key_char2num(lch));
-}
-
 void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
 {
 	u8 mac[ETH_ALEN];
@@ -1166,14 +1147,11 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
 	if (!mac_addr)
 		return;
 
-	if (rtw_initmac) {	/* 	Users specify the mac address */
-		int jj, kk;
-
-		for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
-			mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
-		}
+	if (rtw_initmac && mac_pton(rtw_initmac, mac)) {
+		/* Users specify the mac address */
 		ether_addr_copy(mac_addr, mac);
-	} else{	/* 	Use the mac address stored in the Efuse */
+	} else {
+		/* Use the mac address stored in the Efuse */
 		ether_addr_copy(mac, mac_addr);
 	}
 
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
index 39502156f652..4eb51f45b387 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
@@ -32,9 +32,6 @@
 #define WEXT_CSCAN_HOME_DWELL_SECTION	'H'
 #define WEXT_CSCAN_TYPE_SECTION		'T'
 
-
-extern u8 key_2char2num(u8 hch, u8 lch);
-
 static u32 rtw_rates[] = {1000000, 2000000, 5500000, 11000000,
 	6000000, 9000000, 12000000, 18000000, 24000000, 36000000, 48000000, 54000000};
 
-- 
2.18.0

Re: [PATCH v3 2/4] staging: rtl8723bs: refactor rtw_macaddr_cfg()

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
<straube.linux@gmail.com> wrote:
> Using is_broadcast_ether_addr() and is_zero_ether_addr() instead of
> testing each byte of the mac[] array for 0xff and 0x00 shortens the
> code and improves readability.
>
> If np == NULL, of_get_property() returns NULL, hence the "np" check
> is not needed.
>
> Instead of a fixed default mac address use a random one to reduce the
> likelihood of mac address collision.
>

> Thanks to Joe Perches and Dan Carpenter.

I guess you may use Suggested-by tag as well.

>
> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> ---
>  .../staging/rtl8723bs/core/rtw_ieee80211.c    | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index e55895632921..7aa00d1391f7 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -1177,24 +1177,13 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
>                 memcpy(mac, mac_addr, ETH_ALEN);
>         }
>
> -       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))) {
> -               if (np &&
> -                   (addr = of_get_property(np, "local-mac-address", &len)) &&
> +       if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
> +               if ((addr = of_get_property(np, "local-mac-address", &len)) &&
>                     len == ETH_ALEN) {
>                         memcpy(mac_addr, addr, ETH_ALEN);
>                 } else {
> -                       mac[0] = 0x00;
> -                       mac[1] = 0xe0;
> -                       mac[2] = 0x4c;
> -                       mac[3] = 0x87;
> -                       mac[4] = 0x00;
> -                       mac[5] = 0x00;
> -                       /*  use default mac addresss */
> -                       memcpy(mac_addr, mac, ETH_ALEN);
> -                       DBG_871X("MAC Address from efuse error, assign default one !!!\n");
> +                       eth_random_addr(mac_addr);
> +                       DBG_871X("MAC Address from efuse error, assign random one !!!\n");
>                 }
>         }
>
> --
> 2.18.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
<straube.linux@gmail.com> wrote:
> Use ether_addr_copy() instead of memcpy() to copy the mac address.
>

Suggested-by ?

Btw, ensure that the source and destination buffers are aligned to u16
as required by API.

(It applies to all changes to ether_*addr_*() replacements)

> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index 7aa00d1391f7..8af4a89e632f 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -1172,15 +1172,15 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
>                 for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
>                         mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
>                 }
> -               memcpy(mac_addr, mac, ETH_ALEN);
> +               ether_addr_copy(mac_addr, mac);
>         } else{ /*      Use the mac address stored in the Efuse */
> -               memcpy(mac, mac_addr, ETH_ALEN);
> +               ether_addr_copy(mac, mac_addr);
>         }
>
>         if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
>                 if ((addr = of_get_property(np, "local-mac-address", &len)) &&
>                     len == ETH_ALEN) {
> -                       memcpy(mac_addr, addr, ETH_ALEN);
> +                       ether_addr_copy(mac_addr, addr);
>                 } else {
>                         eth_random_addr(mac_addr);
>                         DBG_871X("MAC Address from efuse error, assign random one !!!\n");
> --
> 2.18.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] staging: rtl8723bs: use mac_pton() in rtw_macaddr_cfg()

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
<straube.linux@gmail.com> wrote:
> Use the mac_pton() helper to convert the mac address string.
>
> The functions key_char2num() and key_2char2num() are not used
> anywhere else and can be removed.
>
> This also has the benefit of validating the input since mac_pton()
> returns false if the string is not valid.
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> ---
>  .../staging/rtl8723bs/core/rtw_ieee80211.c    | 30 +++----------------
>  .../staging/rtl8723bs/os_dep/ioctl_linux.c    |  3 --
>  2 files changed, 4 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index 8af4a89e632f..8e0025e1ff14 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -1137,25 +1137,6 @@ ParseRes rtw_ieee802_11_parse_elems(u8 *start, uint len,
>
>  }
>
> -static u8 key_char2num(u8 ch);
> -static u8 key_char2num(u8 ch)
> -{
> -               if ((ch >= '0') && (ch <= '9'))
> -                       return ch - '0';
> -               else if ((ch >= 'a') && (ch <= 'f'))
> -                       return ch - 'a' + 10;
> -               else if ((ch >= 'A') && (ch <= 'F'))
> -                       return ch - 'A' + 10;
> -               else
> -                       return 0xff;
> -}
> -
> -u8 key_2char2num(u8 hch, u8 lch);
> -u8 key_2char2num(u8 hch, u8 lch)
> -{
> -               return ((key_char2num(hch) << 4) | key_char2num(lch));
> -}
> -
>  void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
>  {
>         u8 mac[ETH_ALEN];
> @@ -1166,14 +1147,11 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
>         if (!mac_addr)
>                 return;
>
> -       if (rtw_initmac) {      /*      Users specify the mac address */
> -               int jj, kk;
> -
> -               for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
> -                       mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
> -               }
> +       if (rtw_initmac && mac_pton(rtw_initmac, mac)) {
> +               /* Users specify the mac address */
>                 ether_addr_copy(mac_addr, mac);
> -       } else{ /*      Use the mac address stored in the Efuse */
> +       } else {
> +               /* Use the mac address stored in the Efuse */
>                 ether_addr_copy(mac, mac_addr);
>         }
>
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> index 39502156f652..4eb51f45b387 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> @@ -32,9 +32,6 @@
>  #define WEXT_CSCAN_HOME_DWELL_SECTION  'H'
>  #define WEXT_CSCAN_TYPE_SECTION                'T'
>
> -
> -extern u8 key_2char2num(u8 hch, u8 lch);
> -
>  static u32 rtw_rates[] = {1000000, 2000000, 5500000, 11000000,
>         6000000, 9000000, 12000000, 18000000, 24000000, 36000000, 48000000, 54000000};
>
> --
> 2.18.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/4] staging: rtl8723bs: refactor rtw_macaddr_cfg()

[Michael Straube]

On 06/26/18 19:29, Andy Shevchenko wrote:
> On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
> <straube.linux@gmail.com> wrote:
>> Using is_broadcast_ether_addr() and is_zero_ether_addr() instead of
>> testing each byte of the mac[] array for 0xff and 0x00 shortens the
>> code and improves readability.
>>
>> If np == NULL, of_get_property() returns NULL, hence the "np" check
>> is not needed.
>>
>> Instead of a fixed default mac address use a random one to reduce the
>> likelihood of mac address collision.
>>
> 
>> Thanks to Joe Perches and Dan Carpenter.
> 
> I guess you may use Suggested-by tag as well.
> 

Thanks for the hint, I will add it.

Regards
Michael

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Michael Straube]

On 06/26/18 19:32, Andy Shevchenko wrote:
> On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
> <straube.linux@gmail.com> wrote:
>> Use ether_addr_copy() instead of memcpy() to copy the mac address.
>>
> 
> Suggested-by ?
> 

I'll add it. Sorry, I was not aware of the Suggested-by tag.

> Btw, ensure that the source and destination buffers are aligned to u16
> as required by API.

To be honest I'm not sure how to do that excactly.

Use __align(2) in the array declarations? e.g.:

         u8 mac[ETH_ALEN] __align(2);


Thanks for your help.
Michael

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Joe Perches]

On Tue, 2018-06-26 at 21:44 +0200, Michael Straube wrote:
> On 06/26/18 19:32, Andy Shevchenko wrote:
> > On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
> > <straube.linux@gmail.com> wrote:
> > > Use ether_addr_copy() instead of memcpy() to copy the mac address.
> > > 
> > 
> > Suggested-by ?
> > 
> 
> I'll add it. Sorry, I was not aware of the Suggested-by tag.
> 
> > Btw, ensure that the source and destination buffers are aligned to u16
> > as required by API.
> 
> To be honest I'm not sure how to do that excactly.
> 
> Use __align(2) in the array declarations? e.g.:
> 
>          u8 mac[ETH_ALEN] __align(2);

All initial function automatics are naturally aligned.
> 

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Michael Straube]

On 06/26/18 22:17, Joe Perches wrote:
> On Tue, 2018-06-26 at 21:44 +0200, Michael Straube wrote:
>> On 06/26/18 19:32, Andy Shevchenko wrote:
>>> On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
>>> <straube.linux@gmail.com> wrote:
>>>> Use ether_addr_copy() instead of memcpy() to copy the mac address.
>>>>
>>>
>>> Suggested-by ?
>>>
>>
>> I'll add it. Sorry, I was not aware of the Suggested-by tag.
>>
>>> Btw, ensure that the source and destination buffers are aligned to u16
>>> as required by API.
>>
>> To be honest I'm not sure how to do that excactly.
>>
>> Use __align(2) in the array declarations? e.g.:
>>
>>           u8 mac[ETH_ALEN] __align(2);
> 
> All initial function automatics are naturally aligned.
>>
So there is nothing to change? Now I'm confused.

Re: [PATCH v3 4/4] staging: rtl8723bs: use mac_pton() in rtw_macaddr_cfg()

[Michael Straube]

On 06/26/18 19:34, Andy Shevchenko wrote:
> On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
> <straube.linux@gmail.com> wrote:
>> Use the mac_pton() helper to convert the mac address string.
>>
>> The functions key_char2num() and key_2char2num() are not used
>> anywhere else and can be removed.
>>
>> This also has the benefit of validating the input since mac_pton()
>> returns false if the string is not valid.
>>
> 
> FWIW,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 

Thanks.

>> Signed-off-by: Michael Straube <straube.linux@gmail.com>
>> ---
>>   .../staging/rtl8723bs/core/rtw_ieee80211.c    | 30 +++----------------
>>   .../staging/rtl8723bs/os_dep/ioctl_linux.c    |  3 --
>>   2 files changed, 4 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
>> index 8af4a89e632f..8e0025e1ff14 100644
>> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
>> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
>> @@ -1137,25 +1137,6 @@ ParseRes rtw_ieee802_11_parse_elems(u8 *start, uint len,
>>
>>   }
>>
>> -static u8 key_char2num(u8 ch);
>> -static u8 key_char2num(u8 ch)
>> -{
>> -               if ((ch >= '0') && (ch <= '9'))
>> -                       return ch - '0';
>> -               else if ((ch >= 'a') && (ch <= 'f'))
>> -                       return ch - 'a' + 10;
>> -               else if ((ch >= 'A') && (ch <= 'F'))
>> -                       return ch - 'A' + 10;
>> -               else
>> -                       return 0xff;
>> -}
>> -
>> -u8 key_2char2num(u8 hch, u8 lch);
>> -u8 key_2char2num(u8 hch, u8 lch)
>> -{
>> -               return ((key_char2num(hch) << 4) | key_char2num(lch));
>> -}
>> -
>>   void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
>>   {
>>          u8 mac[ETH_ALEN];
>> @@ -1166,14 +1147,11 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
>>          if (!mac_addr)
>>                  return;
>>
>> -       if (rtw_initmac) {      /*      Users specify the mac address */
>> -               int jj, kk;
>> -
>> -               for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
>> -                       mac[jj] = key_2char2num(rtw_initmac[kk], rtw_initmac[kk + 1]);
>> -               }
>> +       if (rtw_initmac && mac_pton(rtw_initmac, mac)) {
>> +               /* Users specify the mac address */
>>                  ether_addr_copy(mac_addr, mac);
>> -       } else{ /*      Use the mac address stored in the Efuse */
>> +       } else {
>> +               /* Use the mac address stored in the Efuse */
>>                  ether_addr_copy(mac, mac_addr);
>>          }
>>
>> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
>> index 39502156f652..4eb51f45b387 100644
>> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
>> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
>> @@ -32,9 +32,6 @@
>>   #define WEXT_CSCAN_HOME_DWELL_SECTION  'H'
>>   #define WEXT_CSCAN_TYPE_SECTION                'T'
>>
>> -
>> -extern u8 key_2char2num(u8 hch, u8 lch);
>> -
>>   static u32 rtw_rates[] = {1000000, 2000000, 5500000, 11000000,
>>          6000000, 9000000, 12000000, 18000000, 24000000, 36000000, 48000000, 54000000};
>>
>> --
>> 2.18.0
>>
> 
> 
> 

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Joe Perches]

On Tue, 2018-06-26 at 22:32 +0200, Michael Straube wrote:
> On 06/26/18 22:17, Joe Perches wrote:
> > On Tue, 2018-06-26 at 21:44 +0200, Michael Straube wrote:
> > > On 06/26/18 19:32, Andy Shevchenko wrote:
> > > > On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
> > > > <straube.linux@gmail.com> wrote:
> > > > > Use ether_addr_copy() instead of memcpy() to copy the mac address.
[]
> > > > Btw, ensure that the source and destination buffers are aligned to u16
> > > > as required by API.
> > > To be honest I'm not sure how to do that excactly.
> > > Use __align(2) in the array declarations? e.g.:
> > >           u8 mac[ETH_ALEN] __align(2);
> > All initial function automatics are naturally aligned.
> So there is nothing to change? Now I'm confused.

There's nothing to change.  You could add __aligned(2)
if you wanted to, but it's already aligned.

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Dan Carpenter]

On Tue, Jun 26, 2018 at 10:32:09PM +0200, Michael Straube wrote:
> 
> On 06/26/18 22:17, Joe Perches wrote:
> > On Tue, 2018-06-26 at 21:44 +0200, Michael Straube wrote:
> > > On 06/26/18 19:32, Andy Shevchenko wrote:
> > > > On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
> > > > <straube.linux@gmail.com> wrote:
> > > > > Use ether_addr_copy() instead of memcpy() to copy the mac address.
> > > > > 
> > > > 
> > > > Suggested-by ?
> > > > 
> > > 
> > > I'll add it. Sorry, I was not aware of the Suggested-by tag.
> > > 
> > > > Btw, ensure that the source and destination buffers are aligned to u16
> > > > as required by API.
> > > 
> > > To be honest I'm not sure how to do that excactly.
> > > 
> > > Use __align(2) in the array declarations? e.g.:
> > > 
> > >           u8 mac[ETH_ALEN] __align(2);
> > 
> > All initial function automatics are naturally aligned.
> > > 
> So there is nothing to change? Now I'm confused.

Do not add the __align(2), as Joe says, it's not required.  You just
need to C alignment rules (it's expected/required for this sort of
patch).

Like if you have a struct:

struct foo {
	char a;
	int b;
};

There is going to be a 3 byte gap between a and b because ints are
normally __align(4).  The exception is when the struct is __packed.  So
sizeof(struct foo) in this case is going to be 8.  kmalloc() returns
pointers which are 8 at least byte aligned normally.  See
ARCH_KMALLOC_MINALIGN.  There is one arch where it's 4 byte aligned?

So when you would get things which aren't __align(2) is when you have:

struct bar {
	char a[3];
	u8 mac[ETH_ALEN];
};

Here the struct member before the mac[] is an odd number of char.  Or
when the struct is packed.

regards,
dan carpenter

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Michael Straube]

Am 27.06.2018 um 10:33 schrieb Dan Carpenter:
> On Tue, Jun 26, 2018 at 10:32:09PM +0200, Michael Straube wrote:
>>
>> On 06/26/18 22:17, Joe Perches wrote:
>>> On Tue, 2018-06-26 at 21:44 +0200, Michael Straube wrote:
>>>> On 06/26/18 19:32, Andy Shevchenko wrote:
>>>>> On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
>>>>> <straube.linux@gmail.com> wrote:
>>>>>> Use ether_addr_copy() instead of memcpy() to copy the mac address.
>>>>>>
>>>>>
>>>>> Suggested-by ?
>>>>>
>>>>
>>>> I'll add it. Sorry, I was not aware of the Suggested-by tag.
>>>>
>>>>> Btw, ensure that the source and destination buffers are aligned to u16
>>>>> as required by API.
>>>>
>>>> To be honest I'm not sure how to do that excactly.
>>>>
>>>> Use __align(2) in the array declarations? e.g.:
>>>>
>>>>            u8 mac[ETH_ALEN] __align(2);
>>>
>>> All initial function automatics are naturally aligned.
>>>>
>> So there is nothing to change? Now I'm confused.
> 
> Do not add the __align(2), as Joe says, it's not required.  You just
> need to C alignment rules (it's expected/required for this sort of
> patch).
> 
> Like if you have a struct:
> 
> struct foo {
> 	char a;
> 	int b;
> };
> 
> There is going to be a 3 byte gap between a and b because ints are
> normally __align(4).  The exception is when the struct is __packed.  So
> sizeof(struct foo) in this case is going to be 8.  kmalloc() returns
> pointers which are 8 at least byte aligned normally.  See
> ARCH_KMALLOC_MINALIGN.  There is one arch where it's 4 byte aligned?
> 
> So when you would get things which aren't __align(2) is when you have:
> 
> struct bar {
> 	char a[3];
> 	u8 mac[ETH_ALEN];
> };
> 
> Here the struct member before the mac[] is an odd number of char.  Or
> when the struct is packed.
> 
> regards,
> dan carpenter
> 

Thank you for your explanation. I will not add __aligned(2).
The other buffer is u32 aligned:

struct eeprom_priv
{
         u8 bautoload_fail_flag;
         u8 bloadfile_fail_flag;
         u8 bloadmac_fail_flag;
         u8 EepromOrEfuse;

         u8 mac_addr[6]; /* PermanentAddress */

	...

         u32     ocr;

	...
};

Should I add a thanks line to the commit message:

     Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.

Or would that be considered as too much?

Regards
Michael

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Dan Carpenter]

On Wed, Jun 27, 2018 at 02:56:00PM +0200, Michael Straube wrote:
> Should I add a thanks line to the commit message:
> 
>     Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.
> 
> Or would that be considered as too much?

You can write whatever the heck you want...  :P  No one cares.

When it comes to credit, I do appreciate Reported-by tags because LWN
and employers do count those sometimes.  I also think there should be a
tag for when you find a bug in a patch before the patch gets merged.
You wouldn't get the tag for complaining about white space and style
issue because that's easier and less important and, in a way,
complaining about style is its own reward.  But so far we haven't
invented a tag for that.

regards,
dan carpenter

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Andy Shevchenko]

On Wed, Jun 27, 2018 at 4:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Jun 27, 2018 at 02:56:00PM +0200, Michael Straube wrote:
>> Should I add a thanks line to the commit message:
>>
>>     Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.
>>
>> Or would that be considered as too much?
>
> You can write whatever the heck you want...  :P  No one cares.
>
> When it comes to credit, I do appreciate Reported-by tags because LWN
> and employers do count those sometimes.

In some cases Suggested-by fits better.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Joe Perches]

On Wed, 2018-06-27 at 19:14 +0300, Andy Shevchenko wrote:
> On Wed, Jun 27, 2018 at 4:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Wed, Jun 27, 2018 at 02:56:00PM +0200, Michael Straube wrote:
> > > Should I add a thanks line to the commit message:
> > > 
> > >     Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.
> > > 
> > > Or would that be considered as too much?
> > 
> > You can write whatever the heck you want...  :P  No one cares.

I hope that's true.

> > When it comes to credit, I do appreciate Reported-by tags because LWN
> > and employers do count those sometimes.

Which is likely as useful as judging engineer
productivity by LOC counts.

> In some cases Suggested-by fits better.

True, but meh.

I think all the Suggested-by, Reported-by,
Co-developed-by and such are mostly unnecessary.

Who really cares for anything other than
signed-off-by, acked-by, tested-by, and
maybe reviewed-by ?

Maybe the kernel should add a signature for
"Co-authored-by:" instead of "Co-developed-by:"

Perhaps another option would be to extend
the git commit "Author:" attribute to allow
multiple names and addresses.

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Dan Carpenter]

On Wed, Jun 27, 2018 at 09:42:47AM -0700, Joe Perches wrote:
> On Wed, 2018-06-27 at 19:14 +0300, Andy Shevchenko wrote:
> > On Wed, Jun 27, 2018 at 4:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > On Wed, Jun 27, 2018 at 02:56:00PM +0200, Michael Straube wrote:
> > > > Should I add a thanks line to the commit message:
> > > > 
> > > >     Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.
> > > > 
> > > > Or would that be considered as too much?
> > > 
> > > You can write whatever the heck you want...  :P  No one cares.
> 
> I hope that's true.
> 
> > > When it comes to credit, I do appreciate Reported-by tags because LWN
> > > and employers do count those sometimes.
> 
> Which is likely as useful as judging engineer
> productivity by LOC counts.
> 

What are you talking about??  Reported-by count absolutely means you're
a Rock Star.

regards,
dan carpenter