Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Kalle Valo]

Pali Rohár  writes:

> This patch implements parsing MAC address from NVS data which are sent to
> wl1251 chip. Calibration NVS data could contain valid MAC address and it
> will be used instead randomly generated.
>
> This patch also move code for requesting NVS data from userspace to driver
> initialization code to make sure that NVS data will be there at time when
> permanent MAC address is needed.
>
> Calibration NVS data for wl1251 are model specific. Every one device with
> wl1251 chip should have been calibrated in factory and needs to provide own
> calibration data.
>
> Default example wl1251-nvs.bin data found in linux-firmware repository and
> contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
> invalid as it is not real device specific address, just example one.
>
> Format of calibration NVS data can be found at:
> http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
>
> Signed-off-by: Pali Rohár 

[...]

> +static int wl1251_read_nvs_mac(struct wl1251 *wl)
> +{
> + u8 mac[ETH_ALEN];
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;
> +
> + /* MAC is stored in reverse order */
> + for (i = 0; i < ETH_ALEN; i++)
> + mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];

No magic numbers, please. Replace all nvs offsets with proper defines to
make the code more readable.

-- 
Kalle Valo

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Kalle Valo]

Pali Rohár  writes:

> This patch implements parsing MAC address from NVS data which are sent to
> wl1251 chip. Calibration NVS data could contain valid MAC address and it
> will be used instead randomly generated.
>
> This patch also move code for requesting NVS data from userspace to driver
> initialization code to make sure that NVS data will be there at time when
> permanent MAC address is needed.
>
> Calibration NVS data for wl1251 are model specific. Every one device with
> wl1251 chip should have been calibrated in factory and needs to provide own
> calibration data.
>
> Default example wl1251-nvs.bin data found in linux-firmware repository and
> contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
> invalid as it is not real device specific address, just example one.
>
> Format of calibration NVS data can be found at:
> http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
>
> Signed-off-by: Pali Rohár 

[...]

> +static int wl1251_read_nvs_mac(struct wl1251 *wl)
> +{
> + u8 mac[ETH_ALEN];
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;
> +
> + /* MAC is stored in reverse order */
> + for (i = 0; i < ETH_ALEN; i++)
> + mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];

No magic numbers, please. Replace all nvs offsets with proper defines to
make the code more readable.

-- 
Kalle Valo

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pali Rohár]

On Saturday 24 December 2016 19:14:21 Pavel Machek wrote:
> On Sat 2016-12-24 17:53:00, Pali Rohár wrote:
> > @@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251
> > *wl)
> > 
> > wl->hw->queues = 4;
> > 
> > +   if (wl->nvs == NULL && !wl->use_eeprom) {
> > +   ret = wl1251_fetch_nvs(wl);
> > +   if (ret < 0)
> > +   goto out;
> > +   }
> 
> Is goto out here good idea? IMNSHO it is copy bug, it should
> just proceed with generating random address.

No, goto is correct here. wl1251 cannot be initialized without NVS data. 
And when fetching (from userspace) fails it is fatal error.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pali Rohár]

On Saturday 24 December 2016 19:14:21 Pavel Machek wrote:
> On Sat 2016-12-24 17:53:00, Pali Rohár wrote:
> > @@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251
> > *wl)
> > 
> > wl->hw->queues = 4;
> > 
> > +   if (wl->nvs == NULL && !wl->use_eeprom) {
> > +   ret = wl1251_fetch_nvs(wl);
> > +   if (ret < 0)
> > +   goto out;
> > +   }
> 
> Is goto out here good idea? IMNSHO it is copy bug, it should
> just proceed with generating random address.

No, goto is correct here. wl1251 cannot be initialized without NVS data. 
And when fetching (from userspace) fails it is fatal error.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pavel Machek]

On Sat 2016-12-24 17:53:00, Pali Rohár wrote:
> This patch implements parsing MAC address from NVS data which are sent to
> wl1251 chip. Calibration NVS data could contain valid MAC address and it
> will be used instead randomly generated.

will be used instead of randomly generated one.

> This patch also move code for requesting NVS data from userspace to driver

"moves"

> initialization code to make sure that NVS data will be there at time when
> permanent MAC address is needed.

"at a time"

> Calibration NVS data for wl1251 are model specific. Every one device with

"device specific"? "Every device".

> wl1251 chip should have been calibrated in factory and needs to provide own
> calibration data.
> 
> Default example wl1251-nvs.bin data found in linux-firmware repository and

"are found"

> contains MAC address 00:00:20:07:03:09. So this MAC address is marked as

"contain"

> invalid as it is not real device specific address, just example one.
> 
> Format of calibration NVS data can be found at:
> http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
> 
> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/ti/wl1251/main.c |   39 
> ++---
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
> b/drivers/net/wireless/ti/wl1251/main.c
> index c3fa0b6..1454ba2 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -205,13 +205,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
>   goto out;
>   }
>  
> - if (wl->nvs == NULL && !wl->use_eeprom) {
> - /* No NVS from netlink, try to get it from the filesystem */
> - ret = wl1251_fetch_nvs(wl);
> - if (ret < 0)
> - goto out;
> - }
> -
>  out:
>   return ret;
>  }
> @@ -1538,6 +1531,30 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
>   return 0;
>  }
>  
> +static int wl1251_read_nvs_mac(struct wl1251 *wl)
> +{
> + u8 mac[ETH_ALEN];
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;
> +
> + /* MAC is stored in reverse order */
> + for (i = 0; i < ETH_ALEN; i++)
> + mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];
> +
> + /* 00:00:20:07:03:09 is in default example wl1251-nvs.bin, so invalid */

remove "default".

> + if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
> + return -EINVAL;
> +
> + memcpy(wl->mac_addr, mac, ETH_ALEN);
> + return 0;
> +}
> +
>  static int wl1251_register_hw(struct wl1251 *wl)
>  {
>   int ret;
> @@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
>  
>   wl->hw->queues = 4;
>  
> + if (wl->nvs == NULL && !wl->use_eeprom) {
> + ret = wl1251_fetch_nvs(wl);
> + if (ret < 0)
> + goto out;
> + }

Is goto out here good idea? IMNSHO it is copy bug, it should
just proceed with generating random address.

>   if (wl->use_eeprom)
>   ret = wl1251_read_eeprom_mac(wl);
>   else
> - ret = -EINVAL;
> + ret = wl1251_read_nvs_mac(wl);
>  
>   if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
>   ret = -EINVAL;

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pavel Machek]

On Sat 2016-12-24 17:53:00, Pali Rohár wrote:
> This patch implements parsing MAC address from NVS data which are sent to
> wl1251 chip. Calibration NVS data could contain valid MAC address and it
> will be used instead randomly generated.

will be used instead of randomly generated one.

> This patch also move code for requesting NVS data from userspace to driver

"moves"

> initialization code to make sure that NVS data will be there at time when
> permanent MAC address is needed.

"at a time"

> Calibration NVS data for wl1251 are model specific. Every one device with

"device specific"? "Every device".

> wl1251 chip should have been calibrated in factory and needs to provide own
> calibration data.
> 
> Default example wl1251-nvs.bin data found in linux-firmware repository and

"are found"

> contains MAC address 00:00:20:07:03:09. So this MAC address is marked as

"contain"

> invalid as it is not real device specific address, just example one.
> 
> Format of calibration NVS data can be found at:
> http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
> 
> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/ti/wl1251/main.c |   39 
> ++---
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
> b/drivers/net/wireless/ti/wl1251/main.c
> index c3fa0b6..1454ba2 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -205,13 +205,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
>   goto out;
>   }
>  
> - if (wl->nvs == NULL && !wl->use_eeprom) {
> - /* No NVS from netlink, try to get it from the filesystem */
> - ret = wl1251_fetch_nvs(wl);
> - if (ret < 0)
> - goto out;
> - }
> -
>  out:
>   return ret;
>  }
> @@ -1538,6 +1531,30 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
>   return 0;
>  }
>  
> +static int wl1251_read_nvs_mac(struct wl1251 *wl)
> +{
> + u8 mac[ETH_ALEN];
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;
> +
> + /* MAC is stored in reverse order */
> + for (i = 0; i < ETH_ALEN; i++)
> + mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];
> +
> + /* 00:00:20:07:03:09 is in default example wl1251-nvs.bin, so invalid */

remove "default".

> + if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
> + return -EINVAL;
> +
> + memcpy(wl->mac_addr, mac, ETH_ALEN);
> + return 0;
> +}
> +
>  static int wl1251_register_hw(struct wl1251 *wl)
>  {
>   int ret;
> @@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
>  
>   wl->hw->queues = 4;
>  
> + if (wl->nvs == NULL && !wl->use_eeprom) {
> + ret = wl1251_fetch_nvs(wl);
> + if (ret < 0)
> + goto out;
> + }

Is goto out here good idea? IMNSHO it is copy bug, it should
just proceed with generating random address.

>   if (wl->use_eeprom)
>   ret = wl1251_read_eeprom_mac(wl);
>   else
> - ret = -EINVAL;
> + ret = wl1251_read_nvs_mac(wl);
>  
>   if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
>   ret = -EINVAL;

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pali Rohár]

This patch implements parsing MAC address from NVS data which are sent to
wl1251 chip. Calibration NVS data could contain valid MAC address and it
will be used instead randomly generated.

This patch also move code for requesting NVS data from userspace to driver
initialization code to make sure that NVS data will be there at time when
permanent MAC address is needed.

Calibration NVS data for wl1251 are model specific. Every one device with
wl1251 chip should have been calibrated in factory and needs to provide own
calibration data.

Default example wl1251-nvs.bin data found in linux-firmware repository and
contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
invalid as it is not real device specific address, just example one.

Format of calibration NVS data can be found at:
http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ti/wl1251/main.c |   39 ++---
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index c3fa0b6..1454ba2 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -205,13 +205,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
goto out;
}
 
-   if (wl->nvs == NULL && !wl->use_eeprom) {
-   /* No NVS from netlink, try to get it from the filesystem */
-   ret = wl1251_fetch_nvs(wl);
-   if (ret < 0)
-   goto out;
-   }
-
 out:
return ret;
 }
@@ -1538,6 +1531,30 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
return 0;
 }
 
+static int wl1251_read_nvs_mac(struct wl1251 *wl)
+{
+   u8 mac[ETH_ALEN];
+   int i;
+
+   if (wl->nvs_len < 0x24)
+   return -ENODATA;
+
+   /* length is 2 and data address is 0x546c (mask is 0xfffe) */
+   if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
0x54)
+   return -EINVAL;
+
+   /* MAC is stored in reverse order */
+   for (i = 0; i < ETH_ALEN; i++)
+   mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];
+
+   /* 00:00:20:07:03:09 is in default example wl1251-nvs.bin, so invalid */
+   if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
+   return -EINVAL;
+
+   memcpy(wl->mac_addr, mac, ETH_ALEN);
+   return 0;
+}
+
 static int wl1251_register_hw(struct wl1251 *wl)
 {
int ret;
@@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
 
wl->hw->queues = 4;
 
+   if (wl->nvs == NULL && !wl->use_eeprom) {
+   ret = wl1251_fetch_nvs(wl);
+   if (ret < 0)
+   goto out;
+   }
+
if (wl->use_eeprom)
ret = wl1251_read_eeprom_mac(wl);
else
-   ret = -EINVAL;
+   ret = wl1251_read_nvs_mac(wl);
 
if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
ret = -EINVAL;
-- 
1.7.9.5

[PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pali Rohár]

This patch implements parsing MAC address from NVS data which are sent to
wl1251 chip. Calibration NVS data could contain valid MAC address and it
will be used instead randomly generated.

This patch also move code for requesting NVS data from userspace to driver
initialization code to make sure that NVS data will be there at time when
permanent MAC address is needed.

Calibration NVS data for wl1251 are model specific. Every one device with
wl1251 chip should have been calibrated in factory and needs to provide own
calibration data.

Default example wl1251-nvs.bin data found in linux-firmware repository and
contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
invalid as it is not real device specific address, just example one.

Format of calibration NVS data can be found at:
http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ti/wl1251/main.c |   39 ++---
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index c3fa0b6..1454ba2 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -205,13 +205,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
goto out;
}
 
-   if (wl->nvs == NULL && !wl->use_eeprom) {
-   /* No NVS from netlink, try to get it from the filesystem */
-   ret = wl1251_fetch_nvs(wl);
-   if (ret < 0)
-   goto out;
-   }
-
 out:
return ret;
 }
@@ -1538,6 +1531,30 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
return 0;
 }
 
+static int wl1251_read_nvs_mac(struct wl1251 *wl)
+{
+   u8 mac[ETH_ALEN];
+   int i;
+
+   if (wl->nvs_len < 0x24)
+   return -ENODATA;
+
+   /* length is 2 and data address is 0x546c (mask is 0xfffe) */
+   if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
0x54)
+   return -EINVAL;
+
+   /* MAC is stored in reverse order */
+   for (i = 0; i < ETH_ALEN; i++)
+   mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];
+
+   /* 00:00:20:07:03:09 is in default example wl1251-nvs.bin, so invalid */
+   if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
+   return -EINVAL;
+
+   memcpy(wl->mac_addr, mac, ETH_ALEN);
+   return 0;
+}
+
 static int wl1251_register_hw(struct wl1251 *wl)
 {
int ret;
@@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
 
wl->hw->queues = 4;
 
+   if (wl->nvs == NULL && !wl->use_eeprom) {
+   ret = wl1251_fetch_nvs(wl);
+   if (ret < 0)
+   goto out;
+   }
+
if (wl->use_eeprom)
ret = wl1251_read_eeprom_mac(wl);
else
-   ret = -EINVAL;
+   ret = wl1251_read_nvs_mac(wl);
 
if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
ret = -EINVAL;
-- 
1.7.9.5