[PATCH 0/6] wl1251: Fix MAC address for Nokia N900

[PATCH 0/6] wl1251: Fix MAC address for Nokia N900

[Pali Rohár]

This patch series fix processing MAC address for wl1251 chip found in Nokia N900.

Pali Rohár (6):
  firmware: Add request_firmware_prefer_user() function
  wl1251: Use request_firmware_prefer_user() for loading NVS
    calibration data
  wl1251: Update wl->nvs_len after wl->nvs is valid
  wl1251: Generate random MAC address only if driver does not have
    valid
  wl1251: Parse and use MAC address from supplied NVS data
  wl1251: Set generated MAC address back to NVS data

 drivers/base/firmware_class.c          |   45 +++++++++++++++-
 drivers/net/wireless/ti/wl1251/Kconfig |    1 +
 drivers/net/wireless/ti/wl1251/main.c  |   91 +++++++++++++++++++++++++-------
 include/linux/firmware.h               |    9 ++++
 4 files changed, 125 insertions(+), 21 deletions(-)

-- 
1.7.9.5

[PATCH 1/6] firmware: Add request_firmware_prefer_user() function

[Pali Rohár]

This function works pretty much like request_firmware(), but it prefer
usermode helper. If usermode helper fails then it fallback to direct
access. Useful for dynamic or model specific firmware data.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/base/firmware_class.c |   45 +++++++++++++++++++++++++++++++++++++++--
 include/linux/firmware.h      |    9 +++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760..6a8c261 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -119,6 +119,11 @@ static inline long firmware_loading_timeout(void)
 #endif
 #define FW_OPT_NO_WARN	(1U << 3)
 #define FW_OPT_NOCACHE	(1U << 4)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_PREFER_USER	(1U << 5)
+#else
+#define FW_OPT_PREFER_USER	0
+#endif
 
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
@@ -1169,13 +1174,26 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 		}
 	}
 
-	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (opt_flags & FW_OPT_PREFER_USER) {
+		ret = fw_load_from_user_helper(fw, name, device, opt_flags, timeout);
+		if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
+			dev_warn(device,
+				 "User helper firmware load for %s failed with error %d\n",
+				 name, ret);
+			dev_warn(device, "Falling back to direct firmware load\n");
+		}
+	} else {
+		ret = -EINVAL;
+	}
+
+	if (ret)
+		ret = fw_get_filesystem_firmware(device, fw->priv);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
 				 "Direct firmware load for %s failed with error %d\n",
 				 name, ret);
-		if (opt_flags & FW_OPT_USERHELPER) {
+		if ((opt_flags & FW_OPT_USERHELPER) && !(opt_flags & FW_OPT_PREFER_USER)) {
 			dev_warn(device, "Falling back to user helper\n");
 			ret = fw_load_from_user_helper(fw, name, device,
 						       opt_flags, timeout);
@@ -1287,6 +1305,29 @@ int request_firmware_direct(const struct firmware **firmware_p,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
+ * request_firmware_prefer_user: - prefer usermode helper for loading firmware
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but it prefer
+ * usermode helper. If usermode helper fails then it fallback to direct access.
+ * Useful for dynamic or model specific firmware data.
+ **/
+int request_firmware_prefer_user(const struct firmware **firmware_p,
+			    const char *name, struct device *device)
+{
+	int ret;
+
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, NULL, 0,
+				FW_OPT_UEVENT | FW_OPT_PREFER_USER);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
+
+/**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0c..01f7a85 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,6 +47,8 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
+int request_firmware_prefer_user(const struct firmware **fw, const char *name,
+				 struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
 	const char *name, struct device *device, void *buf, size_t size);
 
@@ -77,6 +79,13 @@ static inline int request_firmware_direct(const struct firmware **fw,
 	return -EINVAL;
 }
 
+static inline int request_firmware_prefer_user(const struct firmware **fw,
+					       const char *name,
+					       struct device *device)
+{
+	return -EINVAL;
+}
+
 static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 	const char *name, struct device *device, void *buf, size_t size)
 {
-- 
1.7.9.5

[PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

NVS calibration data for wl1251 are model specific. Every one device with
wl1251 chip has different and calibrated in factory.

Not all wl1251 chips have own EEPROM where are calibration data stored. And
in that case there is no "standard" place. Every device has stored them on
different place (some in rootfs file, some in dedicated nand partition,
some in another proprietary structure).

Kernel wl1251 driver cannot support every one different storage decided by
device manufacture so it will use request_firmware_prefer_user() call for
loading NVS calibration data and userspace helper will be responsible to
prepare correct data.

In case userspace helper fails request_firmware_prefer_user() still try to
load data file directly from VFS as fallback mechanism.

On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
in CAL nand partition. CAL is proprietary Nokia key/value format for nand
devices.

With this patch it is finally possible to load correct model specific NVS
calibration data for Nokia N900.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/Kconfig |    1 +
 drivers/net/wireless/ti/wl1251/main.c  |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
index 7142ccf..affe154 100644
--- a/drivers/net/wireless/ti/wl1251/Kconfig
+++ b/drivers/net/wireless/ti/wl1251/Kconfig
@@ -2,6 +2,7 @@ config WL1251
 	tristate "TI wl1251 driver support"
 	depends on MAC80211
 	select FW_LOADER
+	select FW_LOADER_USER_HELPER
 	select CRC7
 	---help---
 	  This will enable TI wl1251 driver support. The drivers make
diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 208f062..24f8866 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
 	struct device *dev = wiphy_dev(wl->hw->wiphy);
 	int ret;
 
-	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
+	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
 
 	if (ret < 0) {
 		wl1251_error("could not get nvs file: %d", ret);
-- 
1.7.9.5

[PATCH 3/6] wl1251: Update wl->nvs_len after wl->nvs is valid

[Pali Rohár]

In case kmemdup fails thne wl->nvs_len will contains invalid non-zero size.
This patch fixes it.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/main.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 24f8866..8971b64 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -124,8 +124,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
 		goto out;
 	}
 
-	wl->nvs_len = fw->size;
-	wl->nvs = kmemdup(fw->data, wl->nvs_len, GFP_KERNEL);
+	wl->nvs = kmemdup(fw->data, fw->size, GFP_KERNEL);
 
 	if (!wl->nvs) {
 		wl1251_error("could not allocate memory for the nvs file");
@@ -133,6 +132,8 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
 		goto out;
 	}
 
+	wl->nvs_len = fw->size;
+
 	ret = 0;
 
 out:
-- 
1.7.9.5

[PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid

[Pali Rohár]

Before this patch driver generated random MAC address every time when was
doing initialization. And after that random MAC address could be
overwritten with fixed one if provided.

This patch changes order. First it tries to read fixed MAC address and if
it fails then driver generates random MAC address.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/main.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 8971b64..c3fa0b6 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1582,7 +1582,24 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
 	wl->hw->queues = 4;
 
 	if (wl->use_eeprom)
-		wl1251_read_eeprom_mac(wl);
+		ret = wl1251_read_eeprom_mac(wl);
+	else
+		ret = -EINVAL;
+
+	if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
+		ret = -EINVAL;
+
+	if (ret < 0) {
+		/*
+		 * In case our MAC address is not correctly set,
+		 * we use a random but Nokia MAC.
+		 */
+		static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
+		memcpy(wl->mac_addr, nokia_oui, 3);
+		get_random_bytes(wl->mac_addr + 3, 3);
+		wl1251_warning("MAC address in eeprom or nvs data is not valid");
+		wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
+	}
 
 	ret = wl1251_register_hw(wl);
 	if (ret)
@@ -1623,7 +1640,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
 	struct ieee80211_hw *hw;
 	struct wl1251 *wl;
 	int i;
-	static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
 
 	hw = ieee80211_alloc_hw(sizeof(*wl), &wl1251_ops);
 	if (!hw) {
@@ -1674,13 +1690,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
 	INIT_WORK(&wl->irq_work, wl1251_irq_work);
 	INIT_WORK(&wl->tx_work, wl1251_tx_work);
 
-	/*
-	 * In case our MAC address is not correctly set,
-	 * we use a random but Nokia MAC.
-	 */
-	memcpy(wl->mac_addr, nokia_oui, 3);
-	get_random_bytes(wl->mac_addr + 3, 3);
-
 	wl->state = WL1251_STATE_OFF;
 	mutex_init(&wl->mutex);
 
-- 
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 <pali.rohar@gmail.com>
---
 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 6/6] wl1251: Set generated MAC address back to NVS data

[Pali Rohár]

In case there is no valid MAC address kernel generates random one. This
patch propagate this generated MAC address back to NVS data which will be
uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
uses.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/main.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 1454ba2..895ae05 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1555,6 +1555,24 @@ static int wl1251_read_nvs_mac(struct wl1251 *wl)
 	return 0;
 }
 
+static int wl1251_write_nvs_mac(struct wl1251 *wl)
+{
+	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++)
+		wl->nvs[0x1c + i] = wl->mac_addr[ETH_ALEN - i - 1];
+
+	return 0;
+}
+
 static int wl1251_register_hw(struct wl1251 *wl)
 {
 	int ret;
@@ -1620,6 +1638,8 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
 		static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
 		memcpy(wl->mac_addr, nokia_oui, 3);
 		get_random_bytes(wl->mac_addr + 3, 3);
+		if (!wl->use_eeprom)
+			wl1251_write_nvs_mac(wl);
 		wl1251_warning("MAC address in eeprom or nvs data is not valid");
 		wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
 	}
-- 
1.7.9.5

Re: [PATCH 3/6] wl1251: Update wl->nvs_len after wl->nvs is valid

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

On Sat 2016-12-24 17:52:58, Pali Rohár wrote:
> In case kmemdup fails thne wl->nvs_len will contains invalid non-zero size.
> This patch fixes it.

If kmemdup fails, then wl->nvs_len will contain invalid non-zero size.

?

This probably should go as first in series, as it is bugfix?

									Pavel
Acked-by: Pavel Machek <pavel@ucw.cz>


> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/net/wireless/ti/wl1251/main.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
> index 24f8866..8971b64 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -124,8 +124,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>  		goto out;
>  	}
>  
> -	wl->nvs_len = fw->size;
> -	wl->nvs = kmemdup(fw->data, wl->nvs_len, GFP_KERNEL);
> +	wl->nvs = kmemdup(fw->data, fw->size, GFP_KERNEL);
>  
>  	if (!wl->nvs) {
>  		wl1251_error("could not allocate memory for the nvs file");
> @@ -133,6 +132,8 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>  		goto out;
>  	}
>  
> +	wl->nvs_len = fw->size;
> +
>  	ret = 0;
>  
>  out:

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Sat 2016-12-24 17:52:59, Pali Rohár wrote:

> Before this patch driver generated random MAC address every time when was
> doing initialization. And after that random MAC address could be
> overwritten with fixed one if provided.

Before this patch, driver generated random MAC address every time it
was initialized. After that random MAC address could be overwritten
with fixed one, if provided.

> This patch changes order. First it tries to read fixed MAC address and if
> it fails then driver generates random MAC address.

I don't quite get where the advantage is supposed to be. Is it that
"use_eeprom" is set, but reading fails?

The only case where this helps is if wl1251_read_eeprom_mac() succeeds
but reads invalid address.

> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

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 <pali.rohar@gmail.com>
> ---
>  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&paste 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

Hi!

> In case there is no valid MAC address kernel generates random one. This
> patch propagate this generated MAC address back to NVS data which will be
> uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
> uses.

>  	return 0;
>  }
>  
> +static int wl1251_write_nvs_mac(struct wl1251 *wl)
> +{

The name is quite confusing, this sounds like writing into
non-volatile storage.

> +	int i;
> +
> +	if (wl->nvs_len < 0x24)
> +		return -ENODATA;
> +
> +	/* length is 2 and data address is 0x546c (mask is 0xfffe) */

You don't actually check for the mask.

> +	if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 0x54)
> +		return -EINVAL;

You have two copies of these. Does it make sense to move it to helper
function?

Thanks,
									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1251 bytes --]

On Saturday 24 December 2016 19:08:54 Pavel Machek wrote:
> On Sat 2016-12-24 17:52:59, Pali Rohár wrote:
> > Before this patch driver generated random MAC address every time
> > when was doing initialization. And after that random MAC address
> > could be overwritten with fixed one if provided.
> 
> Before this patch, driver generated random MAC address every time it
> was initialized. After that random MAC address could be overwritten
> with fixed one, if provided.
> 
> > This patch changes order. First it tries to read fixed MAC address
> > and if it fails then driver generates random MAC address.
> 
> I don't quite get where the advantage is supposed to be. Is it that
> "use_eeprom" is set, but reading fails?

Random bytes are read from kernel only if random MAC address is needed. 
And in wl->mac_addr is always either invalid address or permanenent mac 
address which will be used. Without patch in wl->mac_addr can be random 
temporary address for some time...

> The only case where this helps is if wl1251_read_eeprom_mac()
> succeeds but reads invalid address.
> 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 663 bytes --]

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&paste 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.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1277 bytes --]

On Saturday 24 December 2016 19:17:30 Pavel Machek wrote:
> Hi!
> 
> > In case there is no valid MAC address kernel generates random one.
> > This patch propagate this generated MAC address back to NVS data
> > which will be uploaded to wl1251 chip. So HW would have same MAC
> > address as linux kernel uses.
> > 
> >  	return 0;
> >  
> >  }
> > 
> > +static int wl1251_write_nvs_mac(struct wl1251 *wl)
> > +{
> 
> The name is quite confusing, this sounds like writing into
> non-volatile storage.
> 
> > +	int i;
> > +
> > +	if (wl->nvs_len < 0x24)
> > +		return -ENODATA;
> > +
> > +	/* length is 2 and data address is 0x546c (mask is 0xfffe) */
> 
> You don't actually check for the mask.

It is quite complicated. { 0x6d, 0x54 } (= 0x546d) in data represent 
address 0x546c and content are data. You need to apply mask 0xfffe for 
0x546d and you get address where data will be written (so 0x546c).

> > +	if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b]
> > != 0x54) +		return -EINVAL;
> 
> You have two copies of these. Does it make sense to move it to helper
> function?

I'm thinking if checks is really needed. But probably moving it to 
separate function is good idea.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 24-12-2016 17:52, Pali Rohár wrote:
> NVS calibration data for wl1251 are model specific. Every one device with
> wl1251 chip has different and calibrated in factory.
> 
> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> in that case there is no "standard" place. Every device has stored them on
> different place (some in rootfs file, some in dedicated nand partition,
> some in another proprietary structure).
> 
> Kernel wl1251 driver cannot support every one different storage decided by
> device manufacture so it will use request_firmware_prefer_user() call for
> loading NVS calibration data and userspace helper will be responsible to
> prepare correct data.

Responding to this patch as it provides a lot of context to discuss. As
you might have gathered from earlier discussions I am not a fan of using
user-space helper. I can agree that the kernel driver, wl1251 in this
case, should be agnostic to platform specific details regarding storage
solutions and the firmware api should hide that. However, it seems your
only solution is adding user-space to the mix and changing the api
towards that. Can we solve it without user-space help?

The firmware_class already supports a number of path prefixes it
traverses looking for the requested firmware. So I was thinking about
adding a hashtable in which a platform driver can add firmware which are
stored in the hashtable using the hashed firmware name. Upon a firmware
request from the driver we could check the hashtable before traversing
the path prefixes on VFS. The obvious problem is that the request may
come before the firmware is added to the hashtable. Just wanted to pitch
the idea first and hear what others think about it and maybe someone has
a nice solution for this problem. Fingers crossed :-p

> In case userspace helper fails request_firmware_prefer_user() still try to
> load data file directly from VFS as fallback mechanism.
> 
> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> devices.

With the firmware hashtable api on N900 a platform driver could
interpret the CAL data in the nand partition and provide it through the
firmware_class.

> With this patch it is finally possible to load correct model specific NVS
> calibration data for Nokia N900.

But on other devices that use wl1251, but for instance have no userspace
helper the request to userspace will fail (after 60 sec?) and try VFS
after that. Maybe not so nice. You should consider other device
configurations. Not just N900.

Regards,
Arend

> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
> index 7142ccf..affe154 100644
> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> @@ -2,6 +2,7 @@ config WL1251
>  	tristate "TI wl1251 driver support"
>  	depends on MAC80211
>  	select FW_LOADER
> +	select FW_LOADER_USER_HELPER
>  	select CRC7
>  	---help---
>  	  This will enable TI wl1251 driver support. The drivers make
> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
> index 208f062..24f8866 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
>  	int ret;
>  
> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
>  
>  	if (ret < 0) {
>  		wl1251_error("could not get nvs file: %d", ret);
> 

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 5551 bytes --]

On Sunday 25 December 2016 21:15:40 Arend Van Spriel wrote:
> On 24-12-2016 17:52, Pali Rohár wrote:
> > NVS calibration data for wl1251 are model specific. Every one
> > device with wl1251 chip has different and calibrated in factory.
> > 
> > Not all wl1251 chips have own EEPROM where are calibration data
> > stored. And in that case there is no "standard" place. Every
> > device has stored them on different place (some in rootfs file,
> > some in dedicated nand partition, some in another proprietary
> > structure).
> > 
> > Kernel wl1251 driver cannot support every one different storage
> > decided by device manufacture so it will use
> > request_firmware_prefer_user() call for loading NVS calibration
> > data and userspace helper will be responsible to prepare correct
> > data.
> 
> Responding to this patch as it provides a lot of context to discuss.
> As you might have gathered from earlier discussions I am not a fan
> of using user-space helper. I can agree that the kernel driver,
> wl1251 in this case, should be agnostic to platform specific details
> regarding storage solutions and the firmware api should hide that.
> However, it seems your only solution is adding user-space to the mix
> and changing the api towards that. Can we solve it without
> user-space help?

Without userspace helper it means that userspace helper code must be 
integrated into kernel.

So what is userspace helper doing?

1) Read MAC address from CAL
2) Read NVS data from CAL
3) Modify MAC address in memory NVS data (new for this patch series)
4) Modify in memory NVS data if we in FCC country

Checking for country is done via dbus call to either Maemo cellular 
daemon or alternatively via REGDOMAIN in /etc/default/crda. I have plan 
to use ofono (instead Maemo cellular daemon) too...

Currently we are using closed Nokia proprietary CAL library.

Steps 1) and 2) needs closed library, step 4) needs dbus call.

In current state I do not see way to integrate it into kernel. And 
because wl1251 currently uses request_firmware() to load those nvs data 
I think it is still the best way how to handle it...

And IIRC there was already discussion about Nokia CAL parser in kernel 
and it was declined.

> The firmware_class already supports a number of path prefixes it
> traverses looking for the requested firmware. So I was thinking about
> adding a hashtable in which a platform driver can add firmware which
> are stored in the hashtable using the hashed firmware name. Upon a
> firmware request from the driver we could check the hashtable before
> traversing the path prefixes on VFS. The obvious problem is that the
> request may come before the firmware is added to the hashtable. Just
> wanted to pitch the idea first and hear what others think about it
> and maybe someone has a nice solution for this problem. Fingers
> crossed :-p
> 
> > In case userspace helper fails request_firmware_prefer_user() still
> > try to load data file directly from VFS as fallback mechanism.
> > 
> > On Nokia N900 device which has wl1251 chip, NVS calibration data
> > are stored in CAL nand partition. CAL is proprietary Nokia
> > key/value format for nand devices.
> 
> With the firmware hashtable api on N900 a platform driver could
> interpret the CAL data in the nand partition and provide it through
> the firmware_class.
> 
> > With this patch it is finally possible to load correct model
> > specific NVS calibration data for Nokia N900.
> 
> But on other devices that use wl1251, but for instance have no
> userspace helper the request to userspace will fail (after 60 sec?)
> and try VFS after that. Maybe not so nice.

Currently support for those devices is broken (like for N900) as without 
proper NVS data they do not work correctly...

> You should consider other device configurations. Not just N900.

I do not have any other wl1251 devices. I know that pandora has wl1251 
too, but it has wl1251 with eeprom where is stored NVS. And in this case 
request_firmware() is not used there.

> Regards,
> Arend
> 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
> >  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/ti/wl1251/Kconfig
> > b/drivers/net/wireless/ti/wl1251/Kconfig index 7142ccf..affe154
> > 100644
> > --- a/drivers/net/wireless/ti/wl1251/Kconfig
> > +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> > @@ -2,6 +2,7 @@ config WL1251
> > 
> >  	tristate "TI wl1251 driver support"
> >  	depends on MAC80211
> >  	select FW_LOADER
> > 
> > +	select FW_LOADER_USER_HELPER
> > 
> >  	select CRC7
> >  	---help---
> >  	
> >  	  This will enable TI wl1251 driver support. The drivers make
> > 
> > diff --git a/drivers/net/wireless/ti/wl1251/main.c
> > b/drivers/net/wireless/ti/wl1251/main.c index 208f062..24f8866
> > 100644
> > --- a/drivers/net/wireless/ti/wl1251/main.c
> > +++ b/drivers/net/wireless/ti/wl1251/main.c
> > @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
> > 
> >  	struct device *dev = wiphy_dev(wl->hw->wiphy);
> >  	int ret;
> > 
> > -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> > +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
> > 
> >  	if (ret < 0) {
> >  	
> >  		wl1251_error("could not get nvs file: %d", ret);

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

Hi!

> > > NVS calibration data for wl1251 are model specific. Every one
> > > device with wl1251 chip has different and calibrated in factory.
> > > 
> > > Not all wl1251 chips have own EEPROM where are calibration data
> > > stored. And in that case there is no "standard" place. Every
> > > device has stored them on different place (some in rootfs file,
> > > some in dedicated nand partition, some in another proprietary
> > > structure).
> > > 
> > > Kernel wl1251 driver cannot support every one different storage
> > > decided by device manufacture so it will use
> > > request_firmware_prefer_user() call for loading NVS calibration
> > > data and userspace helper will be responsible to prepare correct
> > > data.
> > 
> > Responding to this patch as it provides a lot of context to discuss.
> > As you might have gathered from earlier discussions I am not a fan
> > of using user-space helper. I can agree that the kernel driver,
> > wl1251 in this case, should be agnostic to platform specific details
> > regarding storage solutions and the firmware api should hide that.
> > However, it seems your only solution is adding user-space to the mix
> > and changing the api towards that. Can we solve it without
> > user-space help?
> 
> Without userspace helper it means that userspace helper code must be 
> integrated into kernel.
> 
> So what is userspace helper doing?
> 
> 1) Read MAC address from CAL
> 2) Read NVS data from CAL
> 3) Modify MAC address in memory NVS data (new for this patch series)
> 4) Modify in memory NVS data if we in FCC country
> 
> Checking for country is done via dbus call to either Maemo cellular 
> daemon or alternatively via REGDOMAIN in /etc/default/crda. I have plan 
> to use ofono (instead Maemo cellular daemon) too...
> 
> Currently we are using closed Nokia proprietary CAL library.
> 
> Steps 1) and 2) needs closed library, step 4) needs dbus call.

I guess pointer to the source code implementing this would be welcome.

> > But on other devices that use wl1251, but for instance have no
> > userspace helper the request to userspace will fail (after 60 sec?)
> > and try VFS after that. Maybe not so nice.
> 
> Currently support for those devices is broken (like for N900) as without 
> proper NVS data they do not work correctly...

Is it expected to work at all, perhaps with degraded performance /
range? Because it seems to work for me.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2959 bytes --]

On Monday 26 December 2016 16:43:53 Pavel Machek wrote:
> Hi!
> 
> > > > NVS calibration data for wl1251 are model specific. Every one
> > > > device with wl1251 chip has different and calibrated in
> > > > factory.
> > > > 
> > > > Not all wl1251 chips have own EEPROM where are calibration data
> > > > stored. And in that case there is no "standard" place. Every
> > > > device has stored them on different place (some in rootfs file,
> > > > some in dedicated nand partition, some in another proprietary
> > > > structure).
> > > > 
> > > > Kernel wl1251 driver cannot support every one different storage
> > > > decided by device manufacture so it will use
> > > > request_firmware_prefer_user() call for loading NVS calibration
> > > > data and userspace helper will be responsible to prepare
> > > > correct data.
> > > 
> > > Responding to this patch as it provides a lot of context to
> > > discuss. As you might have gathered from earlier discussions I
> > > am not a fan of using user-space helper. I can agree that the
> > > kernel driver, wl1251 in this case, should be agnostic to
> > > platform specific details regarding storage solutions and the
> > > firmware api should hide that. However, it seems your only
> > > solution is adding user-space to the mix and changing the api
> > > towards that. Can we solve it without user-space help?
> > 
> > Without userspace helper it means that userspace helper code must
> > be integrated into kernel.
> > 
> > So what is userspace helper doing?
> > 
> > 1) Read MAC address from CAL
> > 2) Read NVS data from CAL
> > 3) Modify MAC address in memory NVS data (new for this patch
> > series) 4) Modify in memory NVS data if we in FCC country
> > 
> > Checking for country is done via dbus call to either Maemo cellular
> > daemon or alternatively via REGDOMAIN in /etc/default/crda. I have
> > plan to use ofono (instead Maemo cellular daemon) too...
> > 
> > Currently we are using closed Nokia proprietary CAL library.
> > 
> > Steps 1) and 2) needs closed library, step 4) needs dbus call.
> 
> I guess pointer to the source code implementing this would be
> welcome.

Here is current code: https://github.com/community-ssu/wl1251-cal

(there is implemented also Maemo netlink interface)

> > > But on other devices that use wl1251, but for instance have no
> > > userspace helper the request to userspace will fail (after 60
> > > sec?) and try VFS after that. Maybe not so nice.
> > 
> > Currently support for those devices is broken (like for N900) as
> > without proper NVS data they do not work correctly...
> 
> Is it expected to work at all, perhaps with degraded performance /
> range? Because it seems to work for me.

Yes, some degraded performance or problems with connecting is expected. 
And random MAC address at every boot. Plus some regulatory problems in 
FCC countries.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

On Sun 2016-12-25 21:15:40, Arend Van Spriel wrote:
> On 24-12-2016 17:52, Pali Rohár wrote:
> > NVS calibration data for wl1251 are model specific. Every one device with
> > wl1251 chip has different and calibrated in factory.
> > 
> > Not all wl1251 chips have own EEPROM where are calibration data stored. And
> > in that case there is no "standard" place. Every device has stored them on
> > different place (some in rootfs file, some in dedicated nand partition,
> > some in another proprietary structure).
> > 
> > Kernel wl1251 driver cannot support every one different storage decided by
> > device manufacture so it will use request_firmware_prefer_user() call for
> > loading NVS calibration data and userspace helper will be responsible to
> > prepare correct data.
> 
> Responding to this patch as it provides a lot of context to discuss. As
> you might have gathered from earlier discussions I am not a fan of using
> user-space helper. I can agree that the kernel driver, wl1251 in this
> case, should be agnostic to platform specific details regarding storage
> solutions and the firmware api should hide that. However, it seems your
> only solution is adding user-space to the mix and changing the api
> towards that. Can we solve it without user-space help?

Answer is no, due to licensing. But that's wrong question to ask.

Right question is "should we solve it without user-space help"?

Answer is no, too. Way too complex. Yes, it would be nice if hardware
was designed in such a way that getting calibration data from kernel
is easy, and if you design hardware, please design it like that. But
N900 is not designed like that and getting the calibration through
userspace looks like only reasonable solution.

Now... how exactly to do that is other question. (But this is looks
very reasonable. Maybe I'd add request_firmware_with_flags(, ...int
flags), but.. that's a tiny detail.). But userspace needs to be
involved.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
> 
> Right question is "should we solve it without user-space help"?
> 
> Answer is no, too. Way too complex. Yes, it would be nice if hardware
> was designed in such a way that getting calibration data from kernel
> is easy, and if you design hardware, please design it like that. But
> N900 is not designed like that and getting the calibration through
> userspace looks like only reasonable solution.

Arend seems to have a better alternative in mind possible for other
devices which *can* probably pull of doing this easily and nicely,
given the nasty history of the usermode helper crap we should not
in any way discourage such efforts.

Arend -- please look at the firmware cache, it not a hash but a hash
table for an O(1) lookups would be a welcomed change, then it could
be repurposed for what you describe, I think the only difference is
you'd perhaps want a custom driver hook to fetch the calibration data
so the driver does whatever it needs.

> Now... how exactly to do that is other question. (But this is looks
> very reasonable. Maybe I'd add request_firmware_with_flags(, ...int
> flags), but.. that's a tiny detail.). But userspace needs to be
> involved.

No, no, we keep adding yet-another-exported symbol for requesting firmware,
instead of just adding a set of parameters possible and easily extending
functionality. Please review the patches posted on my last set which adds
a flexible API with only 2 calls, sync and async, and lets us customize
our requests using a parameter:

https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20161216-drvdata-v3-try3

This also documents the "usermode helper" properly and explains some of
the issues and limitations you will need to consider if you use it, its
one reason I'd highly encourage to consider an alternative as what Arend
is considering. *Iff* you insist on using the (now using the proper term,
as per the documentation update I am providing) "custom fallback mechanism"
I welcome such a change but I ask we *really* think this through well so
we avoid the stupid issues which have historically made the custom fallback
mechanism more of a nuisance for Linux distributions, users and developers.
To this end -- I ask you check out Daniel Wagner and Tom Gundersen's firmwared
work [0] which I referred you to in December. Although the drvdata API does
not yet use a custom fallback mechanism, after and its merged the goal here
would be to *only* support a clean custom fallback mechanism which aligns
itself *well* with firmwared or solutions like it. Your patch set then could
just become a patch set to add the custom fallback mechaism support to drvdata
API with the new options/prefernce you are looking for to be specified in
the new parameters, not a new exported symbol.

One of the cruxes we should consider addressing before the drvdata API gets a
custom fallback mechanism support added is that the usermode helper lock should
be replaced with a generic solution for the races it was intended to address:
use of the API on suspend/resume and implicitly later avoid a race on init. To
this end we should consider the same race for *other* real kernel "user mode
helpers", I've documented this on a wiki [1] which documents the *real*
kernel usermode helpers users, one of which was the kobject uevent which is
one of the fallback mechanisms.

I should also note that the idea of fallback mechanism using kobject uevents
should really suffice, in review with Johannes Berg at least, he seemed
convinced just letting either the upstream firmwared, a custom firmwared or
a custom userspace solution should be able to just monitor for uevents for
drvdata and do the right thing, this whole "custom fallback mechanism"
in retrospect seems not really needed as far as I can tell.

[0] https://github.com/teg/firmwared
[1] https://kernelnewbies.org/KernelProjects/usermode-helper-enhancements

  Luis

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> writes:

> NVS calibration data for wl1251 are model specific. Every one device with
> wl1251 chip has different and calibrated in factory.
>
> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> in that case there is no "standard" place. Every device has stored them on
> different place (some in rootfs file, some in dedicated nand partition,
> some in another proprietary structure).
>
> Kernel wl1251 driver cannot support every one different storage decided by
> device manufacture so it will use request_firmware_prefer_user() call for
> loading NVS calibration data and userspace helper will be responsible to
> prepare correct data.
>
> In case userspace helper fails request_firmware_prefer_user() still try to
> load data file directly from VFS as fallback mechanism.
>
> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> devices.
>
> With this patch it is finally possible to load correct model specific NVS
> calibration data for Nokia N900.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
> index 7142ccf..affe154 100644
> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> @@ -2,6 +2,7 @@ config WL1251
>  	tristate "TI wl1251 driver support"
>  	depends on MAC80211
>  	select FW_LOADER
> +	select FW_LOADER_USER_HELPER
>  	select CRC7
>  	---help---
>  	  This will enable TI wl1251 driver support. The drivers make
> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
> index 208f062..24f8866 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
>  	int ret;
>  
> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);

I don't see the need for this. Just remove the default nvs file from
filesystem and the fallback user helper will be always used, right?

Like we discussed earlier, the default nvs file should not be used by
normal users.

-- 
Kalle Valo

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

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> 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 <pali.rohar@gmail.com>

[...]

> +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 6/6] wl1251: Set generated MAC address back to NVS data

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> writes:

> In case there is no valid MAC address kernel generates random one. This
> patch propagate this generated MAC address back to NVS data which will be
> uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
> uses.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

Why? What issue does this fix?

-- 
Kalle Valo

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 8:33, Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
>> NVS calibration data for wl1251 are model specific. Every one device with
>> wl1251 chip has different and calibrated in factory.
>>
>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
>> in that case there is no "standard" place. Every device has stored them on
>> different place (some in rootfs file, some in dedicated nand partition,
>> some in another proprietary structure).
>>
>> Kernel wl1251 driver cannot support every one different storage decided by
>> device manufacture so it will use request_firmware_prefer_user() call for
>> loading NVS calibration data and userspace helper will be responsible to
>> prepare correct data.
>>
>> In case userspace helper fails request_firmware_prefer_user() still try to
>> load data file directly from VFS as fallback mechanism.
>>
>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
>> devices.
>>
>> With this patch it is finally possible to load correct model specific NVS
>> calibration data for Nokia N900.
>>
>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>> ---
>>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
>> index 7142ccf..affe154 100644
>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
>> @@ -2,6 +2,7 @@ config WL1251
>>  	tristate "TI wl1251 driver support"
>>  	depends on MAC80211
>>  	select FW_LOADER
>> +	select FW_LOADER_USER_HELPER
>>  	select CRC7
>>  	---help---
>>  	  This will enable TI wl1251 driver support. The drivers make
>> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
>> index 208f062..24f8866 100644
>> --- a/drivers/net/wireless/ti/wl1251/main.c
>> +++ b/drivers/net/wireless/ti/wl1251/main.c
>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
>>  	int ret;
>>  
>> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
>> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
> 
> I don't see the need for this. Just remove the default nvs file from
> filesystem and the fallback user helper will be always used, right?

Indeed. The only remaining issue would be that an error message is
logged. Also note the fallback is only used if selected in Kconfig.

> Like we discussed earlier, the default nvs file should not be used by
> normal users.

Yup.

Regards,
Arend

Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Pali Rohár]

On Friday 27 January 2017 09:56:09 Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > In case there is no valid MAC address kernel generates random one. This
> > patch propagate this generated MAC address back to NVS data which will be
> > uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
> > uses.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Why? What issue does this fix?

Send permanent MAC address to wl1251 chip, same what is doing wl12xx
driver.

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

Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> writes:

> On Friday 27 January 2017 09:56:09 Kalle Valo wrote:
>> Pali Rohár <pali.rohar@gmail.com> writes:
>> 
>> > In case there is no valid MAC address kernel generates random one. This
>> > patch propagate this generated MAC address back to NVS data which will be
>> > uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
>> > uses.
>> >
>> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>> 
>> Why? What issue does this fix?
>
> Send permanent MAC address to wl1251 chip, same what is doing wl12xx
> driver.

Ok, so this doesn't change functionality in any way and you are adding
it only because wl12xx does the same? You should document that in the
the commit log.

If there's no change I don't really see the point of this. But if
there's harm, hopefully, I guess it's ok.

-- 
Kalle Valo

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > NVS calibration data for wl1251 are model specific. Every one device with
> > wl1251 chip has different and calibrated in factory.
> >
> > Not all wl1251 chips have own EEPROM where are calibration data stored. And
> > in that case there is no "standard" place. Every device has stored them on
> > different place (some in rootfs file, some in dedicated nand partition,
> > some in another proprietary structure).
> >
> > Kernel wl1251 driver cannot support every one different storage decided by
> > device manufacture so it will use request_firmware_prefer_user() call for
> > loading NVS calibration data and userspace helper will be responsible to
> > prepare correct data.
> >
> > In case userspace helper fails request_firmware_prefer_user() still try to
> > load data file directly from VFS as fallback mechanism.
> >
> > On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> > in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> > devices.
> >
> > With this patch it is finally possible to load correct model specific NVS
> > calibration data for Nokia N900.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
> >  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
> > index 7142ccf..affe154 100644
> > --- a/drivers/net/wireless/ti/wl1251/Kconfig
> > +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> > @@ -2,6 +2,7 @@ config WL1251
> >  	tristate "TI wl1251 driver support"
> >  	depends on MAC80211
> >  	select FW_LOADER
> > +	select FW_LOADER_USER_HELPER
> >  	select CRC7
> >  	---help---
> >  	  This will enable TI wl1251 driver support. The drivers make
> > diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
> > index 208f062..24f8866 100644
> > --- a/drivers/net/wireless/ti/wl1251/main.c
> > +++ b/drivers/net/wireless/ti/wl1251/main.c
> > @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
> >  	struct device *dev = wiphy_dev(wl->hw->wiphy);
> >  	int ret;
> >  
> > -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> > +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
> 
> I don't see the need for this. Just remove the default nvs file from
> filesystem and the fallback user helper will be always used, right?

It is part of linux-firmware repository. And already part of all
previous versions of linux-firmware packages in lot of linux
distributions. So removing it is not possible...

> Like we discussed earlier, the default nvs file should not be used by
> normal users.

But already is and we need to deal with this fact.

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 10:43, Pali Rohár wrote:
> On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
>> Pali Rohár <pali.rohar@gmail.com> writes:
>>
>>> NVS calibration data for wl1251 are model specific. Every one device with
>>> wl1251 chip has different and calibrated in factory.
>>>
>>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
>>> in that case there is no "standard" place. Every device has stored them on
>>> different place (some in rootfs file, some in dedicated nand partition,
>>> some in another proprietary structure).
>>>
>>> Kernel wl1251 driver cannot support every one different storage decided by
>>> device manufacture so it will use request_firmware_prefer_user() call for
>>> loading NVS calibration data and userspace helper will be responsible to
>>> prepare correct data.
>>>
>>> In case userspace helper fails request_firmware_prefer_user() still try to
>>> load data file directly from VFS as fallback mechanism.
>>>
>>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
>>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
>>> devices.
>>>
>>> With this patch it is finally possible to load correct model specific NVS
>>> calibration data for Nokia N900.
>>>
>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>> ---
>>>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>>>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
>>> index 7142ccf..affe154 100644
>>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
>>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
>>> @@ -2,6 +2,7 @@ config WL1251
>>>  	tristate "TI wl1251 driver support"
>>>  	depends on MAC80211
>>>  	select FW_LOADER
>>> +	select FW_LOADER_USER_HELPER
>>>  	select CRC7
>>>  	---help---
>>>  	  This will enable TI wl1251 driver support. The drivers make
>>> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
>>> index 208f062..24f8866 100644
>>> --- a/drivers/net/wireless/ti/wl1251/main.c
>>> +++ b/drivers/net/wireless/ti/wl1251/main.c
>>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>>>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
>>>  	int ret;
>>>  
>>> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
>>> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
>>
>> I don't see the need for this. Just remove the default nvs file from
>> filesystem and the fallback user helper will be always used, right?
> 
> It is part of linux-firmware repository. And already part of all
> previous versions of linux-firmware packages in lot of linux
> distributions. So removing it is not possible...

You are probably saying that on your platform you can not remove
anything from /lib/firmware, right? I don't see how you come from "it is
part of firmware package" to "removing is not possible". Trying to
understand this and it makes no sense.

>> Like we discussed earlier, the default nvs file should not be used by
>> normal users.
> 
> But already is and we need to deal with this fact.

Why? Are there other platforms that use the default nvs file and have a
working wifi. So your "removing is not possible" would be about
regression for those?

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Friday 27 January 2017 11:05:32 Arend Van Spriel wrote:
> On 27-1-2017 10:43, Pali Rohár wrote:
> > On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
> >> Pali Rohár <pali.rohar@gmail.com> writes:
> >>
> >>> NVS calibration data for wl1251 are model specific. Every one device with
> >>> wl1251 chip has different and calibrated in factory.
> >>>
> >>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> >>> in that case there is no "standard" place. Every device has stored them on
> >>> different place (some in rootfs file, some in dedicated nand partition,
> >>> some in another proprietary structure).
> >>>
> >>> Kernel wl1251 driver cannot support every one different storage decided by
> >>> device manufacture so it will use request_firmware_prefer_user() call for
> >>> loading NVS calibration data and userspace helper will be responsible to
> >>> prepare correct data.
> >>>
> >>> In case userspace helper fails request_firmware_prefer_user() still try to
> >>> load data file directly from VFS as fallback mechanism.
> >>>
> >>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> >>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> >>> devices.
> >>>
> >>> With this patch it is finally possible to load correct model specific NVS
> >>> calibration data for Nokia N900.
> >>>
> >>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >>> ---
> >>>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
> >>>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
> >>>  2 files changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
> >>> index 7142ccf..affe154 100644
> >>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> >>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> >>> @@ -2,6 +2,7 @@ config WL1251
> >>>  	tristate "TI wl1251 driver support"
> >>>  	depends on MAC80211
> >>>  	select FW_LOADER
> >>> +	select FW_LOADER_USER_HELPER
> >>>  	select CRC7
> >>>  	---help---
> >>>  	  This will enable TI wl1251 driver support. The drivers make
> >>> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
> >>> index 208f062..24f8866 100644
> >>> --- a/drivers/net/wireless/ti/wl1251/main.c
> >>> +++ b/drivers/net/wireless/ti/wl1251/main.c
> >>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
> >>>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
> >>>  	int ret;
> >>>  
> >>> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> >>> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
> >>
> >> I don't see the need for this. Just remove the default nvs file from
> >> filesystem and the fallback user helper will be always used, right?
> > 
> > It is part of linux-firmware repository. And already part of all
> > previous versions of linux-firmware packages in lot of linux
> > distributions. So removing it is not possible...
> 
> You are probably saying that on your platform you can not remove
> anything from /lib/firmware, right? I don't see how you come from "it is
> part of firmware package" to "removing is not possible". Trying to
> understand this and it makes no sense.

It is already in linux distribution packages. If I remove that file from
file system it will be placed there again by package management or it it
will throw error message about system integrity (missing file, etc...).

Also that file is already in linux-firmware git and so is propagated to
/lib/firmware by anybody who is using linux-firmware.

> >> Like we discussed earlier, the default nvs file should not be used by
> >> normal users.
> > 
> > But already is and we need to deal with this fact.
> 
> Why?

Because everybody has already installed it.

> Are there other platforms that use the default nvs file and have a
> working wifi.

I do not know.

> So your "removing is not possible" would be about
> regression for those?

Yes, that is possible.

Also you can use wifi on Nokia N900 with this default file. Yes it is
not recommended and probably has performance problems... but more people
use it for SSH and it is working. Pavel could confirm this.

So yes, if you remove that file *now* there is regression for Nokia N900
when you are using SSH over wifi.

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 11:10, Pali Rohár wrote:
> On Friday 27 January 2017 11:05:32 Arend Van Spriel wrote:
>> On 27-1-2017 10:43, Pali Rohár wrote:
>>> On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
>>>> Pali Rohár <pali.rohar@gmail.com> writes:
>>>>
>>>>> NVS calibration data for wl1251 are model specific. Every one device with
>>>>> wl1251 chip has different and calibrated in factory.
>>>>>
>>>>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
>>>>> in that case there is no "standard" place. Every device has stored them on
>>>>> different place (some in rootfs file, some in dedicated nand partition,
>>>>> some in another proprietary structure).
>>>>>
>>>>> Kernel wl1251 driver cannot support every one different storage decided by
>>>>> device manufacture so it will use request_firmware_prefer_user() call for
>>>>> loading NVS calibration data and userspace helper will be responsible to
>>>>> prepare correct data.
>>>>>
>>>>> In case userspace helper fails request_firmware_prefer_user() still try to
>>>>> load data file directly from VFS as fallback mechanism.
>>>>>
>>>>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
>>>>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
>>>>> devices.
>>>>>
>>>>> With this patch it is finally possible to load correct model specific NVS
>>>>> calibration data for Nokia N900.
>>>>>
>>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>>>> ---
>>>>>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>>>>>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
>>>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> index 7142ccf..affe154 100644
>>>>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
>>>>> @@ -2,6 +2,7 @@ config WL1251
>>>>>  	tristate "TI wl1251 driver support"
>>>>>  	depends on MAC80211
>>>>>  	select FW_LOADER
>>>>> +	select FW_LOADER_USER_HELPER
>>>>>  	select CRC7
>>>>>  	---help---
>>>>>  	  This will enable TI wl1251 driver support. The drivers make
>>>>> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
>>>>> index 208f062..24f8866 100644
>>>>> --- a/drivers/net/wireless/ti/wl1251/main.c
>>>>> +++ b/drivers/net/wireless/ti/wl1251/main.c
>>>>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>>>>>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
>>>>>  	int ret;
>>>>>  
>>>>> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
>>>>> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
>>>>
>>>> I don't see the need for this. Just remove the default nvs file from
>>>> filesystem and the fallback user helper will be always used, right?
>>>
>>> It is part of linux-firmware repository. And already part of all
>>> previous versions of linux-firmware packages in lot of linux
>>> distributions. So removing it is not possible...
>>
>> You are probably saying that on your platform you can not remove
>> anything from /lib/firmware, right? I don't see how you come from "it is
>> part of firmware package" to "removing is not possible". Trying to
>> understand this and it makes no sense.
> 
> It is already in linux distribution packages. If I remove that file from
> file system it will be placed there again by package management or it it
> will throw error message about system integrity (missing file, etc...).
> 
> Also that file is already in linux-firmware git and so is propagated to
> /lib/firmware by anybody who is using linux-firmware.
> 
>>>> Like we discussed earlier, the default nvs file should not be used by
>>>> normal users.
>>>
>>> But already is and we need to deal with this fact.
>>
>> Why?
> 
> Because everybody has already installed it.
> 
>> Are there other platforms that use the default nvs file and have a
>> working wifi.
> 
> I do not know.
> 
>> So your "removing is not possible" would be about
>> regression for those?
> 
> Yes, that is possible.
> 
> Also you can use wifi on Nokia N900 with this default file. Yes it is
> not recommended and probably has performance problems... but more people
> use it for SSH and it is working. Pavel could confirm this.
> 
> So yes, if you remove that file *now* there is regression for Nokia N900
> when you are using SSH over wifi.

So you are changing the behavior for all platforms using wl1251, but the
user-helper preference is (probably) only applicable for N900, right? So
for those other platforms there will be a delay waiting for user-mode
helper to fail, before trying to get nvs file from /lib/firmware.

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Friday 27 January 2017 11:19:25 Arend Van Spriel wrote:
> On 27-1-2017 11:10, Pali Rohár wrote:
> > On Friday 27 January 2017 11:05:32 Arend Van Spriel wrote:
> >> On 27-1-2017 10:43, Pali Rohár wrote:
> >>> On Friday 27 January 2017 09:33:40 Kalle Valo wrote:
> >>>> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>>
> >>>>> NVS calibration data for wl1251 are model specific. Every one device with
> >>>>> wl1251 chip has different and calibrated in factory.
> >>>>>
> >>>>> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> >>>>> in that case there is no "standard" place. Every device has stored them on
> >>>>> different place (some in rootfs file, some in dedicated nand partition,
> >>>>> some in another proprietary structure).
> >>>>>
> >>>>> Kernel wl1251 driver cannot support every one different storage decided by
> >>>>> device manufacture so it will use request_firmware_prefer_user() call for
> >>>>> loading NVS calibration data and userspace helper will be responsible to
> >>>>> prepare correct data.
> >>>>>
> >>>>> In case userspace helper fails request_firmware_prefer_user() still try to
> >>>>> load data file directly from VFS as fallback mechanism.
> >>>>>
> >>>>> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> >>>>> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> >>>>> devices.
> >>>>>
> >>>>> With this patch it is finally possible to load correct model specific NVS
> >>>>> calibration data for Nokia N900.
> >>>>>
> >>>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >>>>> ---
> >>>>>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
> >>>>>  drivers/net/wireless/ti/wl1251/main.c  |    2 +-
> >>>>>  2 files changed, 2 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
> >>>>> index 7142ccf..affe154 100644
> >>>>> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> >>>>> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> >>>>> @@ -2,6 +2,7 @@ config WL1251
> >>>>>  	tristate "TI wl1251 driver support"
> >>>>>  	depends on MAC80211
> >>>>>  	select FW_LOADER
> >>>>> +	select FW_LOADER_USER_HELPER
> >>>>>  	select CRC7
> >>>>>  	---help---
> >>>>>  	  This will enable TI wl1251 driver support. The drivers make
> >>>>> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
> >>>>> index 208f062..24f8866 100644
> >>>>> --- a/drivers/net/wireless/ti/wl1251/main.c
> >>>>> +++ b/drivers/net/wireless/ti/wl1251/main.c
> >>>>> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
> >>>>>  	struct device *dev = wiphy_dev(wl->hw->wiphy);
> >>>>>  	int ret;
> >>>>>  
> >>>>> -	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> >>>>> +	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
> >>>>
> >>>> I don't see the need for this. Just remove the default nvs file from
> >>>> filesystem and the fallback user helper will be always used, right?
> >>>
> >>> It is part of linux-firmware repository. And already part of all
> >>> previous versions of linux-firmware packages in lot of linux
> >>> distributions. So removing it is not possible...
> >>
> >> You are probably saying that on your platform you can not remove
> >> anything from /lib/firmware, right? I don't see how you come from "it is
> >> part of firmware package" to "removing is not possible". Trying to
> >> understand this and it makes no sense.
> > 
> > It is already in linux distribution packages. If I remove that file from
> > file system it will be placed there again by package management or it it
> > will throw error message about system integrity (missing file, etc...).
> > 
> > Also that file is already in linux-firmware git and so is propagated to
> > /lib/firmware by anybody who is using linux-firmware.
> > 
> >>>> Like we discussed earlier, the default nvs file should not be used by
> >>>> normal users.
> >>>
> >>> But already is and we need to deal with this fact.
> >>
> >> Why?
> > 
> > Because everybody has already installed it.
> > 
> >> Are there other platforms that use the default nvs file and have a
> >> working wifi.
> > 
> > I do not know.
> > 
> >> So your "removing is not possible" would be about
> >> regression for those?
> > 
> > Yes, that is possible.
> > 
> > Also you can use wifi on Nokia N900 with this default file. Yes it is
> > not recommended and probably has performance problems... but more people
> > use it for SSH and it is working. Pavel could confirm this.
> > 
> > So yes, if you remove that file *now* there is regression for Nokia N900
> > when you are using SSH over wifi.
> 
> So you are changing the behavior for all platforms using wl1251, but the
> user-helper preference is (probably) only applicable for N900, right?

No. Some wl1251 chips have internal EEPROM where is stored MAC address
and NVS data. And kernel driver already can read it. So this change is
only for platforms without internal EEPROM.

And all platforms without internal EEPROM should use userspace helper to
provide correct NVS data (and ideally also MAC address).

Except Nokia N900 I know just Pandora who has also wl1251 chip. But
Pandora has EEPROM.

Grepping linux source code... and I see only defines for Nokia N900 and
Pandora. There can be also another user with external DTS file, but what
is reality? Is there still really any other user of wl1251 chip with
upstream kernel? If yes, we can prepare userspace helper if he does not
have NVS stored in EEPROM...

> So
> for those other platforms there will be a delay waiting for user-mode
> helper to fail, before trying to get nvs file from /lib/firmware.

Yes, there will be. But there is no easy way to fix this problem that
kernel is trying to use default/example NVS data...

When helper is not available this patch just adds delay, but
functionality is still there and same. With helper support will be
finally fixed.

And I have no idea if those default NVS data are somehow usable on other
platforms...

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> writes:

>> So
>> for those other platforms there will be a delay waiting for user-mode
>> helper to fail, before trying to get nvs file from /lib/firmware.
>
> Yes, there will be. But there is no easy way to fix this problem that
> kernel is trying to use default/example NVS data...

Kernel is doing correctly and requesting NVS data as expected, the
problem here is that linux-firmware claims that the example NVS data is
real calibration data (which it is not). Distros should not use that,
only developers for testing purposes. We should not courage users using
example calibration data.

The simple fix is to rename the NVS file in linux-firmware to something
like wl1251-nvs.bin.example, no need to workaround this in kernel. If
you send a patch to linux-firmware I'm happy to ack that.

-- 
Kalle Valo

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Friday 27 January 2017 13:49:03 Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> >> So
> >> for those other platforms there will be a delay waiting for user-mode
> >> helper to fail, before trying to get nvs file from /lib/firmware.
> >
> > Yes, there will be. But there is no easy way to fix this problem that
> > kernel is trying to use default/example NVS data...
> 
> Kernel is doing correctly and requesting NVS data as expected, the
> problem here is that linux-firmware claims that the example NVS data is
> real calibration data (which it is not). Distros should not use that,
> only developers for testing purposes. We should not courage users using
> example calibration data.
> 
> The simple fix is to rename the NVS file in linux-firmware to something
> like wl1251-nvs.bin.example, no need to workaround this in kernel. If
> you send a patch to linux-firmware I'm happy to ack that.

I agree with rename and fact that default/example data should not be
used.

But...

1) Kernel should not read device/model specific data from VFS where
are stored not-device-specific files preinstalled by linux
distributions.

And linux distributions are already putting files into VFS and kernel
cannot enforce userspace to not do that (as they are already doing it).

2) It was already tested that example NVS data can be used for N900 e.g.
for SSH connection. If real correct data are not available it is better
to use at least those example (and probably log warning message) so user
can connect via SSH and start investigating where is problem.

3) If we do rename *now* we will totally break wifi support on Nokia
N900.

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

Hi!

> > You are probably saying that on your platform you can not remove
> > anything from /lib/firmware, right? I don't see how you come from "it is
> > part of firmware package" to "removing is not possible". Trying to
> > understand this and it makes no sense.
> 
> It is already in linux distribution packages. If I remove that file from
> file system it will be placed there again by package management or it it
> will throw error message about system integrity (missing file, etc...).
> 
> Also that file is already in linux-firmware git and so is propagated to
> /lib/firmware by anybody who is using linux-firmware.
> 
> > >> Like we discussed earlier, the default nvs file should not be used by
> > >> normal users.
> > > 
> > > But already is and we need to deal with this fact.
> > 
> > Why?
> 
> Because everybody has already installed it.
> 
> > Are there other platforms that use the default nvs file and have a
> > working wifi.
> 
> I do not know.
> 
> > So your "removing is not possible" would be about
> > regression for those?
> 
> Yes, that is possible.
> 
> Also you can use wifi on Nokia N900 with this default file. Yes it is
> not recommended and probably has performance problems... but more people
> use it for SSH and it is working. Pavel could confirm this.

Yes, wifi somehow works on N900. .. depending on userspace and kernel
versions.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> writes:

> On Friday 27 January 2017 13:49:03 Kalle Valo wrote:
>> Pali Rohár <pali.rohar@gmail.com> writes:
>> 
>> >> So
>> >> for those other platforms there will be a delay waiting for user-mode
>> >> helper to fail, before trying to get nvs file from /lib/firmware.
>> >
>> > Yes, there will be. But there is no easy way to fix this problem that
>> > kernel is trying to use default/example NVS data...
>> 
>> Kernel is doing correctly and requesting NVS data as expected, the
>> problem here is that linux-firmware claims that the example NVS data is
>> real calibration data (which it is not). Distros should not use that,
>> only developers for testing purposes. We should not courage users using
>> example calibration data.
>> 
>> The simple fix is to rename the NVS file in linux-firmware to something
>> like wl1251-nvs.bin.example, no need to workaround this in kernel. If
>> you send a patch to linux-firmware I'm happy to ack that.
>
> I agree with rename and fact that default/example data should not be
> used.
>
> But...
>
> 1) Kernel should not read device/model specific data from VFS where
> are stored not-device-specific files preinstalled by linux
> distributions.
>
> And linux distributions are already putting files into VFS and kernel
> cannot enforce userspace to not do that (as they are already doing it).

I'm having problems to understand what you are saying here.

> 2) It was already tested that example NVS data can be used for N900 e.g.
> for SSH connection. If real correct data are not available it is better
> to use at least those example (and probably log warning message) so user
> can connect via SSH and start investigating where is problem.

I disagree. Allowing default calibration data to be used can be
unnoticed by user and left her wondering why wifi works so badly.

> 3) If we do rename *now* we will totally break wifi support on Nokia
> N900.

Then the distro should fix that when updating the linux-firmware
packages. Can you provide details about the setup, what distro etc?

-- 
Kalle Valo

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 27-1-2017 13:26, Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
>> On Friday 27 January 2017 13:49:03 Kalle Valo wrote:
>>> Pali Rohár <pali.rohar@gmail.com> writes:
>>>
>>>>> So
>>>>> for those other platforms there will be a delay waiting for user-mode
>>>>> helper to fail, before trying to get nvs file from /lib/firmware.
>>>>
>>>> Yes, there will be. But there is no easy way to fix this problem that
>>>> kernel is trying to use default/example NVS data...
>>>
>>> Kernel is doing correctly and requesting NVS data as expected, the
>>> problem here is that linux-firmware claims that the example NVS data is
>>> real calibration data (which it is not). Distros should not use that,
>>> only developers for testing purposes. We should not courage users using
>>> example calibration data.
>>>
>>> The simple fix is to rename the NVS file in linux-firmware to something
>>> like wl1251-nvs.bin.example, no need to workaround this in kernel. If
>>> you send a patch to linux-firmware I'm happy to ack that.
>>
>> I agree with rename and fact that default/example data should not be
>> used.
>>
>> But...
>>
>> 1) Kernel should not read device/model specific data from VFS where
>> are stored not-device-specific files preinstalled by linux
>> distributions.
>>
>> And linux distributions are already putting files into VFS and kernel
>> cannot enforce userspace to not do that (as they are already doing it).
> 
> I'm having problems to understand what you are saying here.

This is a personal opinion. I read it as: /lib/firmware can only contain
files for from linux-firmware.

At least the device-specific vs. non-device-specific does not seem to
hold. The firmware files that we have in the linux-firmware repository
are very device-specific. Unless you mean the 'platform' when talking
about 'device'.

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > On Friday 27 January 2017 13:49:03 Kalle Valo wrote:
> >> Pali Rohár <pali.rohar@gmail.com> writes:
> >> 
> >> >> So
> >> >> for those other platforms there will be a delay waiting for user-mode
> >> >> helper to fail, before trying to get nvs file from /lib/firmware.
> >> >
> >> > Yes, there will be. But there is no easy way to fix this problem that
> >> > kernel is trying to use default/example NVS data...
> >> 
> >> Kernel is doing correctly and requesting NVS data as expected, the
> >> problem here is that linux-firmware claims that the example NVS data is
> >> real calibration data (which it is not). Distros should not use that,
> >> only developers for testing purposes. We should not courage users using
> >> example calibration data.
> >> 
> >> The simple fix is to rename the NVS file in linux-firmware to something
> >> like wl1251-nvs.bin.example, no need to workaround this in kernel. If
> >> you send a patch to linux-firmware I'm happy to ack that.
> >
> > I agree with rename and fact that default/example data should not be
> > used.
> >
> > But...
> >
> > 1) Kernel should not read device/model specific data from VFS where
> > are stored not-device-specific files preinstalled by linux
> > distributions.
> >
> > And linux distributions are already putting files into VFS and kernel
> > cannot enforce userspace to not do that (as they are already doing it).
> 
> I'm having problems to understand what you are saying here.

I'm saying that linux distributions are putting files to /lib/firmware
which comes from some sources already released. You cannot force linux
distributions to stop putting particular file to /lib/firmware *now*
after it was already released and recommended.

> > 2) It was already tested that example NVS data can be used for N900 e.g.
> > for SSH connection. If real correct data are not available it is better
> > to use at least those example (and probably log warning message) so user
> > can connect via SSH and start investigating where is problem.
> 
> I disagree. Allowing default calibration data to be used can be
> unnoticed by user and left her wondering why wifi works so badly.

So there are only two options:

1) Disallow it and so these users will have non-working wifi.

2) Allow those data to be used as fallback mechanism.

And personally I'm against 1) because it will break wifi support for
*all* Nokia N900 devices right now.

> > 3) If we do rename *now* we will totally break wifi support on Nokia
> > N900.
> 
> Then the distro should fix that when updating the linux-firmware
> packages. Can you provide details about the setup, what distro etc?

Debian stable, Ubuntu LTSs 14.04, 16.04. And I think that other
LTS distributions contains that example nvs file too (I'm not going to
verify others, but list will be probably long). Upgrading linux-firmware
is against policy of those distributions. So no this is not an solution.

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Friday 27 January 2017 13:53:28 Arend Van Spriel wrote:
> On 27-1-2017 13:26, Kalle Valo wrote:
> > Pali Rohár <pali.rohar@gmail.com> writes:
> > 
> >> On Friday 27 January 2017 13:49:03 Kalle Valo wrote:
> >>> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>
> >>>>> So
> >>>>> for those other platforms there will be a delay waiting for user-mode
> >>>>> helper to fail, before trying to get nvs file from /lib/firmware.
> >>>>
> >>>> Yes, there will be. But there is no easy way to fix this problem that
> >>>> kernel is trying to use default/example NVS data...
> >>>
> >>> Kernel is doing correctly and requesting NVS data as expected, the
> >>> problem here is that linux-firmware claims that the example NVS data is
> >>> real calibration data (which it is not). Distros should not use that,
> >>> only developers for testing purposes. We should not courage users using
> >>> example calibration data.
> >>>
> >>> The simple fix is to rename the NVS file in linux-firmware to something
> >>> like wl1251-nvs.bin.example, no need to workaround this in kernel. If
> >>> you send a patch to linux-firmware I'm happy to ack that.
> >>
> >> I agree with rename and fact that default/example data should not be
> >> used.
> >>
> >> But...
> >>
> >> 1) Kernel should not read device/model specific data from VFS where
> >> are stored not-device-specific files preinstalled by linux
> >> distributions.
> >>
> >> And linux distributions are already putting files into VFS and kernel
> >> cannot enforce userspace to not do that (as they are already doing it).
> > 
> > I'm having problems to understand what you are saying here.
> 
> This is a personal opinion. I read it as: /lib/firmware can only contain
> files for from linux-firmware.
> 
> At least the device-specific vs. non-device-specific does not seem to
> hold. The firmware files that we have in the linux-firmware repository
> are very device-specific. Unless you mean the 'platform' when talking
> about 'device'.

Here I'm talking about files which are specific per unit. Every one N900
has different NVS file (stored in CAL) and so every one N900 device
needs its own NVS file. And we cannot store thousands of NVS files into
linux-firmware repository for each N900 which was ever produced in
factory.

Firmware files in linux-firmware repository are "device" specific, but
"filename" of that file describe exactly for which "device" it is
specific.

But there are thousands of different NVS files for one filename
"wl1251-nvs.bin" and we cannot use one particular for another device. In
linux-firmware is stored "wl1251-nvs.bin" file with example data.

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> writes:

> On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
>> Pali Rohár <pali.rohar@gmail.com> writes:
>> 
>> > 2) It was already tested that example NVS data can be used for N900 e.g.
>> > for SSH connection. If real correct data are not available it is better
>> > to use at least those example (and probably log warning message) so user
>> > can connect via SSH and start investigating where is problem.
>> 
>> I disagree. Allowing default calibration data to be used can be
>> unnoticed by user and left her wondering why wifi works so badly.
>
> So there are only two options:
>
> 1) Disallow it and so these users will have non-working wifi.
>
> 2) Allow those data to be used as fallback mechanism.
>
> And personally I'm against 1) because it will break wifi support for
> *all* Nokia N900 devices right now.

All two of them? :)

But not working is exactly my point, if correct calibration data is not
available wifi should not work. And it's not only about functionality
problems, there's also the regulatory aspect.

>> > 3) If we do rename *now* we will totally break wifi support on Nokia
>> > N900.
>> 
>> Then the distro should fix that when updating the linux-firmware
>> packages. Can you provide details about the setup, what distro etc?
>
> Debian stable, Ubuntu LTSs 14.04, 16.04. 

You can run these out of box on N900?

> And I think that other LTS distributions contains that example nvs
> file too (I'm not going to verify others, but list will be probably
> long). Upgrading linux-firmware is against policy of those
> distributions. So no this is not an solution.

So instead we should workaround distro policies in kernel? Come on.

Seriously, just rename the file in linux-firmware and file a bug (with a
patch) to distros. If they don't fix the bug you just have to do a
custom hack for N900. But such is life.

-- 
Kalle Valo

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Friday 27 January 2017 17:23:07 Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> >> Pali Rohár <pali.rohar@gmail.com> writes:
> >> 
> >> > 2) It was already tested that example NVS data can be used for N900 e.g.
> >> > for SSH connection. If real correct data are not available it is better
> >> > to use at least those example (and probably log warning message) so user
> >> > can connect via SSH and start investigating where is problem.
> >> 
> >> I disagree. Allowing default calibration data to be used can be
> >> unnoticed by user and left her wondering why wifi works so badly.
> >
> > So there are only two options:
> >
> > 1) Disallow it and so these users will have non-working wifi.
> >
> > 2) Allow those data to be used as fallback mechanism.
> >
> > And personally I'm against 1) because it will break wifi support for
> > *all* Nokia N900 devices right now.
> 
> All two of them? :)

Ehm...

> But not working is exactly my point, if correct calibration data is not
> available wifi should not work. And it's not only about functionality
> problems, there's also the regulatory aspect.

About functionality, Pavel confirmed too that SSH is somehow working...

Regulatory aspect is different, but via iw can be manually configured
some settings.

> >> > 3) If we do rename *now* we will totally break wifi support on Nokia
> >> > N900.
> >> 
> >> Then the distro should fix that when updating the linux-firmware
> >> packages. Can you provide details about the setup, what distro etc?
> >
> > Debian stable, Ubuntu LTSs 14.04, 16.04. 
> 
> You can run these out of box on N900?

Out-of-box I can run Kubuntu 12.04 (which is LTS too). They had prepared
special image for N900 and I still have it on uSD card.

I guess that new versions of Ubuntu could somehow work (maybe not
out-of-box but with some changes) and Pavel has working Debian.

Also basic support needed for wifi and SSH server is probably working
with any distribution targeting armv7-a or omap3. So yes, I can say it
is out-of-box. We will not have GSM calls or camera support, but wifi
breakage is there.

> > And I think that other LTS distributions contains that example nvs
> > file too (I'm not going to verify others, but list will be probably
> > long). Upgrading linux-firmware is against policy of those
> > distributions. So no this is not an solution.
> 
> So instead we should workaround distro policies in kernel? Come on.
> 
> Seriously, just rename the file in linux-firmware and file a bug (with a
> patch) to distros. If they don't fix the bug you just have to do a
> custom hack for N900. But such is life.

I do not see point what will be changed. I rename that file and after
system update (or integrity check) it will be there again.

And if I do that, what prevents kernel to stop using NVS file from
/lib/firmware/? Nothing, original problem (which is going solved by this
patch series) still remains.

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]

On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> >> Pali Rohár <pali.rohar@gmail.com> writes:
> >> 
> >> > 2) It was already tested that example NVS data can be used for N900 e.g.
> >> > for SSH connection. If real correct data are not available it is better
> >> > to use at least those example (and probably log warning message) so user
> >> > can connect via SSH and start investigating where is problem.
> >> 
> >> I disagree. Allowing default calibration data to be used can be
> >> unnoticed by user and left her wondering why wifi works so badly.
> >
> > So there are only two options:
> >
> > 1) Disallow it and so these users will have non-working wifi.
> >
> > 2) Allow those data to be used as fallback mechanism.
> >
> > And personally I'm against 1) because it will break wifi support for
> > *all* Nokia N900 devices right now.
> 
> All two of them? :)

Umm. You clearly want a flock of angry penguins at your doorsteps :-).

> But not working is exactly my point, if correct calibration data is not
> available wifi should not work. And it's not only about functionality
> problems, there's also the regulatory aspect.

If you break existing configuration that's called "regression".

> >> > 3) If we do rename *now* we will totally break wifi support on Nokia
> >> > N900.
> >> 
> >> Then the distro should fix that when updating the linux-firmware
> >> packages. Can you provide details about the setup, what distro etc?
> >
> > Debian stable, Ubuntu LTSs 14.04, 16.04. 
> 
> You can run these out of box on N900?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Fri, Jan 27, 2017 at 02:11:46PM +0100, Pali Rohár wrote:
> So there are only two options:
> 
> 1) Disallow it and so these users will have non-working wifi.
> 
> 2) Allow those data to be used as fallback mechanism.

There is one "custom fallback" user in kernel which we recently
determined was a total mistake. A sysfs interface should have
been defined to enable custom LED settings. Can't a series of
sysfs interfaces be used to enable override ? So is that not a
third option worth consideration?

  Luis

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [170127 11:41]:
> On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
> > Pali Rohár <pali.rohar@gmail.com> writes:
> > 
> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> > >> Pali Rohár <pali.rohar@gmail.com> writes:
> > >> 
> > >> > 2) It was already tested that example NVS data can be used for N900 e.g.
> > >> > for SSH connection. If real correct data are not available it is better
> > >> > to use at least those example (and probably log warning message) so user
> > >> > can connect via SSH and start investigating where is problem.
> > >> 
> > >> I disagree. Allowing default calibration data to be used can be
> > >> unnoticed by user and left her wondering why wifi works so badly.
> > >
> > > So there are only two options:
> > >
> > > 1) Disallow it and so these users will have non-working wifi.
> > >
> > > 2) Allow those data to be used as fallback mechanism.
> > >
> > > And personally I'm against 1) because it will break wifi support for
> > > *all* Nokia N900 devices right now.
> > 
> > All two of them? :)
> 
> Umm. You clearly want a flock of angry penguins at your doorsteps :-).

Well this silly issue of symlinking and renaming nvs files in a standard
Linux distro was also hitting me on various devices with wl12xx/wl18xx
trying to use the same rootfs.

Why don't we just set a custom compatible property for n900 that then
picks up some other nvs file instead of the default?

Regards,

Tony

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1925 bytes --]

On Monday 30 January 2017 18:53:09 Tony Lindgren wrote:
> * Pavel Machek <pavel@ucw.cz> [170127 11:41]:
> > On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
> > > Pali Rohár <pali.rohar@gmail.com> writes:
> > > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> > > >> Pali Rohár <pali.rohar@gmail.com> writes:
> > > >> > 2) It was already tested that example NVS data can be used
> > > >> > for N900 e.g. for SSH connection. If real correct data are
> > > >> > not available it is better to use at least those example
> > > >> > (and probably log warning message) so user can connect via
> > > >> > SSH and start investigating where is problem.
> > > >> 
> > > >> I disagree. Allowing default calibration data to be used can
> > > >> be unnoticed by user and left her wondering why wifi works so
> > > >> badly.
> > > > 
> > > > So there are only two options:
> > > > 
> > > > 1) Disallow it and so these users will have non-working wifi.
> > > > 
> > > > 2) Allow those data to be used as fallback mechanism.
> > > > 
> > > > And personally I'm against 1) because it will break wifi
> > > > support for *all* Nokia N900 devices right now.
> > > 
> > > All two of them? :)
> > 
> > Umm. You clearly want a flock of angry penguins at your doorsteps
> > :-).
> 
> Well this silly issue of symlinking and renaming nvs files in a
> standard Linux distro was also hitting me on various devices with
> wl12xx/wl18xx trying to use the same rootfs.

wl12xx/wl18xx have probably exactly same problem as wl1251.

> Why don't we just set a custom compatible property for n900 that then
> picks up some other nvs file instead of the default?

But that still does not solve this problem correctly. Every n900 device 
have different NVS file. If we allow to load firmware directly from VFS 
without userspace helper we would see again same problem.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Kalle Valo]

Tony Lindgren <tony@atomide.com> writes:

> * Pavel Machek <pavel@ucw.cz> [170127 11:41]:
>> On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
>> > Pali Rohár <pali.rohar@gmail.com> writes:
>> > 
>> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
>> > >> Pali Rohár <pali.rohar@gmail.com> writes:
>> > >> 
>> > >> > 2) It was already tested that example NVS data can be used for N900 e.g.
>> > >> > for SSH connection. If real correct data are not available it is better
>> > >> > to use at least those example (and probably log warning message) so user
>> > >> > can connect via SSH and start investigating where is problem.
>> > >> 
>> > >> I disagree. Allowing default calibration data to be used can be
>> > >> unnoticed by user and left her wondering why wifi works so badly.
>> > >
>> > > So there are only two options:
>> > >
>> > > 1) Disallow it and so these users will have non-working wifi.
>> > >
>> > > 2) Allow those data to be used as fallback mechanism.
>> > >
>> > > And personally I'm against 1) because it will break wifi support for
>> > > *all* Nokia N900 devices right now.
>> > 
>> > All two of them? :)
>> 
>> Umm. You clearly want a flock of angry penguins at your doorsteps :-).
>
> Well this silly issue of symlinking and renaming nvs files in a standard
> Linux distro was also hitting me on various devices with wl12xx/wl18xx
> trying to use the same rootfs.
>
> Why don't we just set a custom compatible property for n900 that then
> picks up some other nvs file instead of the default?

Please don't. An ugly kernel workaround in kernel because of user space
problems is a bad idea. wl1251 should just ask for NVS file from user
space, it shouldn't care if it's a "default" file or something else.
That's a user space policy decision.

Why can't you do something like this:

* rename the NVS file linux-firmware to wl1251-nvs.bin.example

* before distro updates linux-firmware create yours own deb/rpm/whatever
  package "wl1251-firmware" which installs your flavor of nvs file (or
  the user fallback helper if more dynamic functionality is preferred)

-- 
Kalle Valo

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Tony Lindgren]

* Kalle Valo <kvalo@codeaurora.org> [170130 22:36]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > * Pavel Machek <pavel@ucw.cz> [170127 11:41]:
> >> On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
> >> > Pali Rohár <pali.rohar@gmail.com> writes:
> >> > 
> >> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> >> > >> Pali Rohár <pali.rohar@gmail.com> writes:
> >> > >> 
> >> > >> > 2) It was already tested that example NVS data can be used for N900 e.g.
> >> > >> > for SSH connection. If real correct data are not available it is better
> >> > >> > to use at least those example (and probably log warning message) so user
> >> > >> > can connect via SSH and start investigating where is problem.
> >> > >> 
> >> > >> I disagree. Allowing default calibration data to be used can be
> >> > >> unnoticed by user and left her wondering why wifi works so badly.
> >> > >
> >> > > So there are only two options:
> >> > >
> >> > > 1) Disallow it and so these users will have non-working wifi.
> >> > >
> >> > > 2) Allow those data to be used as fallback mechanism.
> >> > >
> >> > > And personally I'm against 1) because it will break wifi support for
> >> > > *all* Nokia N900 devices right now.
> >> > 
> >> > All two of them? :)
> >> 
> >> Umm. You clearly want a flock of angry penguins at your doorsteps :-).
> >
> > Well this silly issue of symlinking and renaming nvs files in a standard
> > Linux distro was also hitting me on various devices with wl12xx/wl18xx
> > trying to use the same rootfs.
> >
> > Why don't we just set a custom compatible property for n900 that then
> > picks up some other nvs file instead of the default?
> 
> Please don't. An ugly kernel workaround in kernel because of user space
> problems is a bad idea. wl1251 should just ask for NVS file from user
> space, it shouldn't care if it's a "default" file or something else.
> That's a user space policy decision.

Grr I keep forgetting it needs to be for each device manufactured so
yeah that won't work.

The names of standard distro files are hardcoded into the kernel
driver so it's also a kernel problem though :p

How about a custom devices tree property saying "needs-custom-firmware"?

Something that would prevent anything being loaded until user space
loads the firmware. It could also be set in the driver automatically
based on the compatible flag if we always want it enabled. And we could
have some cmdline option to ignore it. Or the other way around whatever
makes sense.

> Why can't you do something like this:
> 
> * rename the NVS file linux-firmware to wl1251-nvs.bin.example

As that name is hardcoded in the kernel and that file is provided by
all standard distros, let's assume we just have to deal with that ABI
forever.

> * before distro updates linux-firmware create yours own deb/rpm/whatever
>   package "wl1251-firmware" which installs your flavor of nvs file (or
>   the user fallback helper if more dynamic functionality is preferred)

And that won't work when using the same file system on other machines.

Think NFSroot for example. At least I'm using the same NFSroot across
about 15 different machines including one n900 macro board with smc91x
Ethernet.

Regards,

Tony

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

On Tuesday 31 January 2017 07:59:18 Tony Lindgren wrote:
> * Kalle Valo <kvalo@codeaurora.org> [170130 22:36]:
> > Tony Lindgren <tony@atomide.com> writes:
> > 
> > > * Pavel Machek <pavel@ucw.cz> [170127 11:41]:
> > >> On Fri 2017-01-27 17:23:07, Kalle Valo wrote:
> > >> > Pali Rohár <pali.rohar@gmail.com> writes:
> > >> > 
> > >> > > On Friday 27 January 2017 14:26:22 Kalle Valo wrote:
> > >> > >> Pali Rohár <pali.rohar@gmail.com> writes:
> > >> > >> 
> > >> > >> > 2) It was already tested that example NVS data can be used for N900 e.g.
> > >> > >> > for SSH connection. If real correct data are not available it is better
> > >> > >> > to use at least those example (and probably log warning message) so user
> > >> > >> > can connect via SSH and start investigating where is problem.
> > >> > >> 
> > >> > >> I disagree. Allowing default calibration data to be used can be
> > >> > >> unnoticed by user and left her wondering why wifi works so badly.
> > >> > >
> > >> > > So there are only two options:
> > >> > >
> > >> > > 1) Disallow it and so these users will have non-working wifi.
> > >> > >
> > >> > > 2) Allow those data to be used as fallback mechanism.
> > >> > >
> > >> > > And personally I'm against 1) because it will break wifi support for
> > >> > > *all* Nokia N900 devices right now.
> > >> > 
> > >> > All two of them? :)
> > >> 
> > >> Umm. You clearly want a flock of angry penguins at your doorsteps :-).
> > >
> > > Well this silly issue of symlinking and renaming nvs files in a standard
> > > Linux distro was also hitting me on various devices with wl12xx/wl18xx
> > > trying to use the same rootfs.
> > >
> > > Why don't we just set a custom compatible property for n900 that then
> > > picks up some other nvs file instead of the default?
> > 
> > Please don't. An ugly kernel workaround in kernel because of user space
> > problems is a bad idea. wl1251 should just ask for NVS file from user
> > space, it shouldn't care if it's a "default" file or something else.
> > That's a user space policy decision.
> 
> Grr I keep forgetting it needs to be for each device manufactured so
> yeah that won't work.
> 
> The names of standard distro files are hardcoded into the kernel
> driver so it's also a kernel problem though :p
> 
> How about a custom devices tree property saying "needs-custom-firmware"?

How does it help request_firmware() call which automatically loads
firmware file from VFS (if is available)?

> Something that would prevent anything being loaded until user space
> loads the firmware. It could also be set in the driver automatically
> based on the compatible flag if we always want it enabled. And we could
> have some cmdline option to ignore it. Or the other way around whatever
> makes sense.

So you just want to kernel automatically prevent loading firmware file
(based on flag which driver can set). That is similar approach as mine.

> > Why can't you do something like this:
> > 
> > * rename the NVS file linux-firmware to wl1251-nvs.bin.example
> 
> As that name is hardcoded in the kernel and that file is provided by
> all standard distros, let's assume we just have to deal with that ABI
> forever.

Yes.

> > * before distro updates linux-firmware create yours own deb/rpm/whatever
> >   package "wl1251-firmware" which installs your flavor of nvs file (or
> >   the user fallback helper if more dynamic functionality is preferred)
> 
> And that won't work when using the same file system on other machines.
> 
> Think NFSroot for example. At least I'm using the same NFSroot across
> about 15 different machines including one n900 macro board with smc91x
> Ethernet.

Exactly problem which we already discussed in previous emails. You
cannot serve one file (loaded by direct request_firmware) when your
rootfs is readonly, e.g. comes via NFS shared for more devices...

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

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Michal Kazior]

On 1 February 2017 at 09:33, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 31 January 2017 07:59:18 Tony Lindgren wrote:
>> * Kalle Valo <kvalo@codeaurora.org> [170130 22:36]:
[...]
>> > * before distro updates linux-firmware create yours own deb/rpm/whatever
>> >   package "wl1251-firmware" which installs your flavor of nvs file (or
>> >   the user fallback helper if more dynamic functionality is preferred)
>>
>> And that won't work when using the same file system on other machines.
>>
>> Think NFSroot for example. At least I'm using the same NFSroot across
>> about 15 different machines including one n900 macro board with smc91x
>> Ethernet.
>
> Exactly problem which we already discussed in previous emails. You
> cannot serve one file (loaded by direct request_firmware) when your
> rootfs is readonly, e.g. comes via NFS shared for more devices...

You can extract the nvs blob, put it in tmpfs and bind-mount (or
symlink) it to /lib/firmware/ via modprobe install hook (or init
scripts).


Michał

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 3-1-2017 18:59, Luis R. Rodriguez wrote:
> On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
>>
>> Right question is "should we solve it without user-space help"?
>>
>> Answer is no, too. Way too complex. Yes, it would be nice if hardware
>> was designed in such a way that getting calibration data from kernel
>> is easy, and if you design hardware, please design it like that. But
>> N900 is not designed like that and getting the calibration through
>> userspace looks like only reasonable solution.
> 
> Arend seems to have a better alternative in mind possible for other
> devices which *can* probably pull of doing this easily and nicely,
> given the nasty history of the usermode helper crap we should not
> in any way discourage such efforts.
> 
> Arend -- please look at the firmware cache, it not a hash but a hash
> table for an O(1) lookups would be a welcomed change, then it could
> be repurposed for what you describe, I think the only difference is
> you'd perhaps want a custom driver hook to fetch the calibration data
> so the driver does whatever it needs.

Hi Luis,

I let my idea catch dust on the shelf for a while. Actually had a couple
of patches ready, but did get around testing them. So I wanted to rebase
them on your linux-next tree. I bumped into the umh lock thing and was
wondering why direct filesystem access was under that lock as well. In
your tree I noticed a fix for that. The more reason to base my work on
top of your firmware_class changes. Now my question is what is the best
branch to choose, because you have a "few" in that repo to choose from ;-)

Regards,
Arend

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Wed, May 03, 2017 at 09:02:20PM +0200, Arend Van Spriel wrote:
> On 3-1-2017 18:59, Luis R. Rodriguez wrote:
> > On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
> >>
> >> Right question is "should we solve it without user-space help"?
> >>
> >> Answer is no, too. Way too complex. Yes, it would be nice if hardware
> >> was designed in such a way that getting calibration data from kernel
> >> is easy, and if you design hardware, please design it like that. But
> >> N900 is not designed like that and getting the calibration through
> >> userspace looks like only reasonable solution.
> > 
> > Arend seems to have a better alternative in mind possible for other
> > devices which *can* probably pull of doing this easily and nicely,
> > given the nasty history of the usermode helper crap we should not
> > in any way discourage such efforts.
> > 
> > Arend -- please look at the firmware cache, it not a hash but a hash
> > table for an O(1) lookups would be a welcomed change, then it could
> > be repurposed for what you describe, I think the only difference is
> > you'd perhaps want a custom driver hook to fetch the calibration data
> > so the driver does whatever it needs.
> 
> Hi Luis,
> 
> I let my idea catch dust on the shelf for a while. 

:) BTW did you get to check out Daniel Wagner and Tom Gundersen's firmware work
[0] ? This can provide a proper clear fallback mechanism which *also* helps
address the race between "critical mount points ready" problem we had discussed
long ago. IIRC it did this by having two modes of operation a best effort-mode
and a final-mode. The final-mode would only be used once all the real rootfs is
ready. Userspace decides when to kick / signal firmwared to switch to final-mode
as only userspace will know for sure when that is ready. The best-effort mode
would be used in initramfs. There is also no need for a "custom fallback", the
uevent fallback mechanism can be relied upon alone. Custom tools can just fork
off and do something similar to firmward then in terms of architecture. This is
a form of fallback mechanism I think I'd be happy to see enabled on the new
driver data API. But of course, I recall also liking what you had in mind as well
so would be happy to see more alternatives! The cleaner the better !

[0] https://github.com/teg/firmwared

> Actually had a couple
> of patches ready, but did get around testing them. So I wanted to rebase
> them on your linux-next tree. I bumped into the umh lock thing and was
> wondering why direct filesystem access was under that lock as well.

Indeed, it was an odd thing.

> In your tree I noticed a fix for that. 

Yup!

It took a lot of git archeology to reach a sound approach forward which makes
me feel comfortable without regressing the kernel, this should not regress
the kernel.

> The more reason to base my work on
> top of your firmware_class changes. Now my question is what is the best
> branch to choose, because you have a "few" in that repo to choose from ;-)

I have a series of topical changes, and I rebase onto the latest linux-next
regularly before posting patches, if 0-day finds issues, I dish out a next
try2 or tryX iteration until all issues are fixed. So in this case you'd
just go for the latest driver-data-$(next_date) and if there is a try
postfix use the latest tryX.

In this case 20170501-driver-data-try2, this is based on linux-next tag
next-20170501. If you have issues booting on that next tag though you
could instead try the v4.11-rc8-driver-data-try3 branch based on v4.11-rc8.
But naturally patches ultimately should be based on the latest linux-next
tag for actual submission.

PS. after my changes the fallback mechanism can easily be shoved into its
own file, not sure if that helps with how clean of a solution your work
will be.

  Luis

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Arend Van Spriel]

On 4-5-2017 4:28, Luis R. Rodriguez wrote:
> On Wed, May 03, 2017 at 09:02:20PM +0200, Arend Van Spriel wrote:
>> On 3-1-2017 18:59, Luis R. Rodriguez wrote:
>>> On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
>>>>
>>>> Right question is "should we solve it without user-space help"?
>>>>
>>>> Answer is no, too. Way too complex. Yes, it would be nice if hardware
>>>> was designed in such a way that getting calibration data from kernel
>>>> is easy, and if you design hardware, please design it like that. But
>>>> N900 is not designed like that and getting the calibration through
>>>> userspace looks like only reasonable solution.
>>>
>>> Arend seems to have a better alternative in mind possible for other
>>> devices which *can* probably pull of doing this easily and nicely,
>>> given the nasty history of the usermode helper crap we should not
>>> in any way discourage such efforts.
>>>
>>> Arend -- please look at the firmware cache, it not a hash but a hash
>>> table for an O(1) lookups would be a welcomed change, then it could
>>> be repurposed for what you describe, I think the only difference is
>>> you'd perhaps want a custom driver hook to fetch the calibration data
>>> so the driver does whatever it needs.
>>
>> Hi Luis,
>>
>> I let my idea catch dust on the shelf for a while. 
> 
> :) BTW did you get to check out Daniel Wagner and Tom Gundersen's firmware work
> [0] ? This can provide a proper clear fallback mechanism which *also* helps
> address the race between "critical mount points ready" problem we had discussed
> long ago. IIRC it did this by having two modes of operation a best effort-mode
> and a final-mode. The final-mode would only be used once all the real rootfs is
> ready. Userspace decides when to kick / signal firmwared to switch to final-mode
> as only userspace will know for sure when that is ready. The best-effort mode
> would be used in initramfs. There is also no need for a "custom fallback", the
> uevent fallback mechanism can be relied upon alone. Custom tools can just fork
> off and do something similar to firmward then in terms of architecture. This is
> a form of fallback mechanism I think I'd be happy to see enabled on the new
> driver data API. But of course, I recall also liking what you had in mind as well
> so would be happy to see more alternatives! The cleaner the better !
> 
> [0] https://github.com/teg/firmwared
> 
>> Actually had a couple
>> of patches ready, but did get around testing them. So I wanted to rebase
>> them on your linux-next tree. I bumped into the umh lock thing and was
>> wondering why direct filesystem access was under that lock as well.
> 
> Indeed, it was an odd thing.
> 
>> In your tree I noticed a fix for that. 
> 
> Yup!
> 
> It took a lot of git archeology to reach a sound approach forward which makes
> me feel comfortable without regressing the kernel, this should not regress
> the kernel.
> 
>> The more reason to base my work on
>> top of your firmware_class changes. Now my question is what is the best
>> branch to choose, because you have a "few" in that repo to choose from ;-)
> 
> I have a series of topical changes, and I rebase onto the latest linux-next
> regularly before posting patches, if 0-day finds issues, I dish out a next
> try2 or tryX iteration until all issues are fixed. So in this case you'd
> just go for the latest driver-data-$(next_date) and if there is a try
> postfix use the latest tryX.
> 
> In this case 20170501-driver-data-try2, this is based on linux-next tag
> next-20170501. If you have issues booting on that next tag though you
> could instead try the v4.11-rc8-driver-data-try3 branch based on v4.11-rc8.
> But naturally patches ultimately should be based on the latest linux-next
> tag for actual submission.
> 
> PS. after my changes the fallback mechanism can easily be shoved into its
> own file, not sure if that helps with how clean of a solution your work
> will be.

Let me explain the idea to refresh your memory (and mine). It started
when we were working on adding driver support for OpenWrt in brcmfmac.
The driver requests for firmware calibration data, but on routers it is
stored in flash. So after failing on the firmware request we now call a
platform specific API. That was my itch, but it was not bad enough to go
and scratch. Now for N900 case there is a similar scenario alhtough it
has additional requirement to go to user-space due to need to use a
proprietary library to obtain the NVS calibration data. My thought: Why
should firmware_class care? So the idea is that firmware_class provides
a registry for modules that can produce a certain firmware "file". Those
modules can do whatever is needed. If they need to use umh so be it.
They would only register themselves with firmware_class on platforms
that need them. It would basically be replacing the fallback mechanism
and only be effective on certain platforms.

Let me know if this idea is still of interest and I will rebase what I
have for an RFC round.

Regards,
Arend

[PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900

[Pali Rohár]

This patch series fix processing MAC address for wl1251 chip found in Nokia N900.

Changes since v1:
* Added Acked-by for Pavel Machek
* Fixed grammar
* Magic numbers for NVS offsets are replaced by defines
* Check for validity of mac address NVS data is moved into function
* Changed order of patches as Pavel requested

Pali Rohár (6):
  wl1251: Update wl->nvs_len after wl->nvs is valid
  wl1251: Generate random MAC address only if driver does not have
    valid
  wl1251: Parse and use MAC address from supplied NVS data
  wl1251: Set generated MAC address back to NVS data
  firmware: Add request_firmware_prefer_user() function
  wl1251: Use request_firmware_prefer_user() for loading NVS
    calibration data

 drivers/base/firmware_class.c          |   45 +++++++++++++-
 drivers/net/wireless/ti/wl1251/Kconfig |    1 +
 drivers/net/wireless/ti/wl1251/main.c  |  104 ++++++++++++++++++++++++++------
 include/linux/firmware.h               |    9 +++
 4 files changed, 138 insertions(+), 21 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/6] wl1251: Update wl->nvs_len after wl->nvs is valid

[Pali Rohár]

If kmemdup fails, then wl->nvs_len will contain invalid non-zero size.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/net/wireless/ti/wl1251/main.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 9915d83..8929bb3 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -122,8 +122,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
 		goto out;
 	}
 
-	wl->nvs_len = fw->size;
-	wl->nvs = kmemdup(fw->data, wl->nvs_len, GFP_KERNEL);
+	wl->nvs = kmemdup(fw->data, fw->size, GFP_KERNEL);
 
 	if (!wl->nvs) {
 		wl1251_error("could not allocate memory for the nvs file");
@@ -131,6 +130,8 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
 		goto out;
 	}
 
+	wl->nvs_len = fw->size;
+
 	ret = 0;
 
 out:
-- 
1.7.9.5

[PATCH v2 2/6] wl1251: Generate random MAC address only if driver does not have valid

[Pali Rohár]

Before this patch, driver generated random MAC address every time it was
initialized. After that random MAC address could be overwritten with fixed
one, if provided.

This patch changes order. First it tries to read fixed MAC address and if
it fails then driver generates random MAC address.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/net/wireless/ti/wl1251/main.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 8929bb3..9106c20 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1492,7 +1492,24 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
 	wl->hw->queues = 4;
 
 	if (wl->use_eeprom)
-		wl1251_read_eeprom_mac(wl);
+		ret = wl1251_read_eeprom_mac(wl);
+	else
+		ret = -EINVAL;
+
+	if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
+		ret = -EINVAL;
+
+	if (ret < 0) {
+		/*
+		 * In case our MAC address is not correctly set,
+		 * we use a random but Nokia MAC.
+		 */
+		static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
+		memcpy(wl->mac_addr, nokia_oui, 3);
+		get_random_bytes(wl->mac_addr + 3, 3);
+		wl1251_warning("MAC address in eeprom or nvs data is not valid");
+		wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
+	}
 
 	ret = wl1251_register_hw(wl);
 	if (ret)
@@ -1513,7 +1530,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
 	struct ieee80211_hw *hw;
 	struct wl1251 *wl;
 	int i;
-	static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
 
 	hw = ieee80211_alloc_hw(sizeof(*wl), &wl1251_ops);
 	if (!hw) {
@@ -1563,13 +1579,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
 	INIT_WORK(&wl->irq_work, wl1251_irq_work);
 	INIT_WORK(&wl->tx_work, wl1251_tx_work);
 
-	/*
-	 * In case our MAC address is not correctly set,
-	 * we use a random but Nokia MAC.
-	 */
-	memcpy(wl->mac_addr, nokia_oui, 3);
-	get_random_bytes(wl->mac_addr + 3, 3);
-
 	wl->state = WL1251_STATE_OFF;
 	mutex_init(&wl->mutex);
 	spin_lock_init(&wl->wl_lock);
-- 
1.7.9.5

[PATCH v2 3/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 of randomly generated one.

This patch also moves 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 device specific. Every device with
wl1251 chip should have been calibrated in factory and needs to provide own
calibration data.

Default example file wl1251-nvs.bin, found in linux-firmware repository,
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 <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/main.c |   55 ++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 9106c20..d497ba5 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -203,13 +203,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;
 }
@@ -1448,6 +1441,46 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
 	return 0;
 }
 
+#define NVS_OFF_MAC_LEN 0x19
+#define NVS_OFF_MAC_ADDR_LO 0x1a
+#define NVS_OFF_MAC_ADDR_HI 0x1b
+#define NVS_OFF_MAC_DATA 0x1c
+
+static int wl1251_check_nvs_mac(struct wl1251 *wl)
+{
+	if (wl->nvs_len < 0x24)
+		return -ENODATA;
+
+	/* length is 2 and data address is 0x546c (ANDed with 0xfffe) */
+	if (wl->nvs[NVS_OFF_MAC_LEN] != 2 ||
+	    wl->nvs[NVS_OFF_MAC_ADDR_LO] != 0x6d ||
+	    wl->nvs[NVS_OFF_MAC_ADDR_HI] != 0x54)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int wl1251_read_nvs_mac(struct wl1251 *wl)
+{
+	u8 mac[ETH_ALEN];
+	int i, ret;
+
+	ret = wl1251_check_nvs_mac(wl);
+	if (ret)
+		return ret;
+
+	/* MAC is stored in reverse order */
+	for (i = 0; i < ETH_ALEN; i++)
+		mac[i] = wl->nvs[NVS_OFF_MAC_DATA + ETH_ALEN - i - 1];
+
+	/* 00:00:20:07:03:09 is in example file 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;
@@ -1491,10 +1524,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 v2 4/6] wl1251: Set generated MAC address back to NVS data

[Pali Rohár]

In case there is no valid MAC address kernel generates random one. This
patch propagate this generated MAC address back to NVS data which will be
uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
uses.

This should not change any functionality, but it is better to tell wl1251
correct mac address since beginning of chip usage.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/main.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index d497ba5..1f423be 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1481,6 +1481,21 @@ static int wl1251_read_nvs_mac(struct wl1251 *wl)
 	return 0;
 }
 
+static int wl1251_write_nvs_mac(struct wl1251 *wl)
+{
+	int i, ret;
+
+	ret = wl1251_check_nvs_mac(wl);
+	if (ret)
+		return ret;
+
+	/* MAC is stored in reverse order */
+	for (i = 0; i < ETH_ALEN; i++)
+		wl->nvs[NVS_OFF_MAC_DATA + i] = wl->mac_addr[ETH_ALEN - i - 1];
+
+	return 0;
+}
+
 static int wl1251_register_hw(struct wl1251 *wl)
 {
 	int ret;
@@ -1546,6 +1561,8 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
 		static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
 		memcpy(wl->mac_addr, nokia_oui, 3);
 		get_random_bytes(wl->mac_addr + 3, 3);
+		if (!wl->use_eeprom)
+			wl1251_write_nvs_mac(wl);
 		wl1251_warning("MAC address in eeprom or nvs data is not valid");
 		wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
 	}
-- 
1.7.9.5

[PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Pali Rohár]

This function works pretty much like request_firmware(), but it prefer
usermode helper. If usermode helper fails then it fallback to direct
access. Useful for dynamic or model specific firmware data.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/base/firmware_class.c |   45 +++++++++++++++++++++++++++++++++++++++--
 include/linux/firmware.h      |    9 +++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b57cf5..c3a9fe5 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 #endif
 #define FW_OPT_NO_WARN	(1U << 3)
 #define FW_OPT_NOCACHE	(1U << 4)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_PREFER_USER	(1U << 5)
+#else
+#define FW_OPT_PREFER_USER	0
+#endif
 
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
@@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
-	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (opt_flags & FW_OPT_PREFER_USER) {
+		ret = fw_load_from_user_helper(fw, name, device, opt_flags, timeout);
+		if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
+			dev_warn(device,
+				 "User helper firmware load for %s failed with error %d\n",
+				 name, ret);
+			dev_warn(device, "Falling back to direct firmware load\n");
+		}
+	} else {
+		ret = -EINVAL;
+	}
+
+	if (ret)
+		ret = fw_get_filesystem_firmware(device, fw->priv);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
 				 "Direct firmware load for %s failed with error %d\n",
 				 name, ret);
-		if (opt_flags & FW_OPT_USERHELPER) {
+		if ((opt_flags & FW_OPT_USERHELPER) && !(opt_flags & FW_OPT_PREFER_USER)) {
 			dev_warn(device, "Falling back to user helper\n");
 			ret = fw_load_from_user_helper(fw, name, device,
 						       opt_flags);
@@ -1329,6 +1347,29 @@ int request_firmware_direct(const struct firmware **firmware_p,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
+ * request_firmware_prefer_user: - prefer usermode helper for loading firmware
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but it prefer
+ * usermode helper. If usermode helper fails then it fallback to direct access.
+ * Useful for dynamic or model specific firmware data.
+ **/
+int request_firmware_prefer_user(const struct firmware **firmware_p,
+			    const char *name, struct device *device)
+{
+	int ret;
+
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, NULL, 0,
+				FW_OPT_UEVENT | FW_OPT_PREFER_USER);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
+
+/**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d450808..8584528 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -48,6 +48,8 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
+int request_firmware_prefer_user(const struct firmware **fw, const char *name,
+				 struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
 	const char *name, struct device *device, void *buf, size_t size);
 
@@ -78,6 +80,13 @@ static inline int request_firmware_direct(const struct firmware **fw,
 	return -EINVAL;
 }
 
+static inline int request_firmware_prefer_user(const struct firmware **fw,
+					       const char *name,
+					       struct device *device)
+{
+	return -EINVAL;
+}
+
 static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 	const char *name, struct device *device, void *buf, size_t size)
 {
-- 
1.7.9.5

[PATCH v2 6/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

NVS calibration data for wl1251 are model specific. Every one device with
wl1251 chip has different and calibrated in factory.

Not all wl1251 chips have own EEPROM where are calibration data stored. And
in that case there is no "standard" place. Every device has stored them on
different place (some in rootfs file, some in dedicated nand partition,
some in another proprietary structure).

Kernel wl1251 driver cannot support every one different storage decided by
device manufacture so it will use request_firmware_prefer_user() call for
loading NVS calibration data and userspace helper will be responsible to
prepare correct data.

In case userspace helper fails request_firmware_prefer_user() still try to
load data file directly from VFS as fallback mechanism.

On Nokia N900 device, which has wl1251 chip, NVS calibration data are
stored in CAL nand partition. CAL is proprietary Nokia key/value format for
nand devices.

With this patch it is finally possible to load correct model specific NVS
calibration data for Nokia N900.

Userspace tool for reading NVS calibration data on Nokia N900 is available
in git repository at: https://github.com/community-ssu/wl1251-cal

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/net/wireless/ti/wl1251/Kconfig |    1 +
 drivers/net/wireless/ti/wl1251/main.c  |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
index 7142ccf..affe154 100644
--- a/drivers/net/wireless/ti/wl1251/Kconfig
+++ b/drivers/net/wireless/ti/wl1251/Kconfig
@@ -2,6 +2,7 @@ config WL1251
 	tristate "TI wl1251 driver support"
 	depends on MAC80211
 	select FW_LOADER
+	select FW_LOADER_USER_HELPER
 	select CRC7
 	---help---
 	  This will enable TI wl1251 driver support. The drivers make
diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
index 1f423be..e9d232c 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -108,7 +108,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
 	struct device *dev = wiphy_dev(wl->hw->wiphy);
 	int ret;
 
-	ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
+	ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
 
 	if (ret < 0) {
 		wl1251_error("could not get nvs file: %d", ret);
-- 
1.7.9.5

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> This function works pretty much like request_firmware(), but it prefer
> usermode helper. If usermode helper fails then it fallback to direct
> access. Useful for dynamic or model specific firmware data.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/base/firmware_class.c |   45 +++++++++++++++++++++++++++++++++++++++--
>  include/linux/firmware.h      |    9 +++++++++
>  2 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 4b57cf5..c3a9fe5 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
>  #endif
>  #define FW_OPT_NO_WARN	(1U << 3)
>  #define FW_OPT_NOCACHE	(1U << 4)
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +#define FW_OPT_PREFER_USER	(1U << 5)
> +#else
> +#define FW_OPT_PREFER_USER	0
> +#endif

I've been cleaning these up these flags [0], which I'll shortly respin based
on feedback, so this sort of stuff should be avoided at all costs.

Regardless of this even if you *leave* the flag in place and a driver required
this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
calling fw_load_from_user_helper would just already return -ENOENT, as such it
would in turn fallback to direct fs loading so the #ifef'ery seems to be not
needed.

[0] https://lkml.kernel.org/r/20170914225422.31034-1-mcgrof@kernel.org

>  struct firmware_cache {
>  	/* firmware_buf instance will be added into the below list */
> @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
>  	if (ret <= 0) /* error or already assigned */
>  		goto out;
>  
> -	ret = fw_get_filesystem_firmware(device, fw->priv);
> +	if (opt_flags & FW_OPT_PREFER_USER) {
> +		ret = fw_load_from_user_helper(fw, name, device, opt_flags, timeout);
> +		if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> +			dev_warn(device,
> +				 "User helper firmware load for %s failed with error %d\n",
> +				 name, ret);
> +			dev_warn(device, "Falling back to direct firmware load\n");

As I had noted before, the usermode helper was really not well designed,
as such extending further use of it is something we should shy away unless we
determine its completely necessary.

So what's wrong with this driver failing at direct access, which should be fast,
and relying on a uevent to then work using the current fallback mechanisms?

The commit log in no way documents any of the justifications for further
extending use of the usermode helper.

  Luis

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 12:26 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> even if you *leave* the flag in place and a driver required
> this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> calling fw_load_from_user_helper would just already return -ENOENT, as such it
> would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> needed.

> The commit log in no way documents any of the justifications for further
> extending use of the usermode helper.

Also, any new functionality introduced should have a respective test
case in tools/testing/selftests/firmware/ and lib/test_firmware.c, as
well as extending existing documentation on
Documentation/driver-api/firmware/.

  Luis

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Pali Rohár]

On Friday 10 November 2017 21:26:01 Luis R. Rodriguez wrote:
> On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> > This function works pretty much like request_firmware(), but it prefer
> > usermode helper. If usermode helper fails then it fallback to direct
> > access. Useful for dynamic or model specific firmware data.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >  drivers/base/firmware_class.c |   45 +++++++++++++++++++++++++++++++++++++++--
> >  include/linux/firmware.h      |    9 +++++++++
> >  2 files changed, 52 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 4b57cf5..c3a9fe5 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
> >  #endif
> >  #define FW_OPT_NO_WARN	(1U << 3)
> >  #define FW_OPT_NOCACHE	(1U << 4)
> > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> > +#define FW_OPT_PREFER_USER	(1U << 5)
> > +#else
> > +#define FW_OPT_PREFER_USER	0
> > +#endif
> 
> I've been cleaning these up these flags [0], which I'll shortly respin based
> on feedback, so this sort of stuff should be avoided at all costs.
> 
> Regardless of this even if you *leave* the flag in place and a driver required
> this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> calling fw_load_from_user_helper would just already return -ENOENT, as such it
> would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> needed.
> 
> [0] https://lkml.kernel.org/r/20170914225422.31034-1-mcgrof@kernel.org
> 
> >  struct firmware_cache {
> >  	/* firmware_buf instance will be added into the below list */
> > @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
> >  	if (ret <= 0) /* error or already assigned */
> >  		goto out;
> >  
> > -	ret = fw_get_filesystem_firmware(device, fw->priv);
> > +	if (opt_flags & FW_OPT_PREFER_USER) {
> > +		ret = fw_load_from_user_helper(fw, name, device, opt_flags, timeout);
> > +		if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> > +			dev_warn(device,
> > +				 "User helper firmware load for %s failed with error %d\n",
> > +				 name, ret);
> > +			dev_warn(device, "Falling back to direct firmware load\n");
> 
> As I had noted before, the usermode helper was really not well designed,
> as such extending further use of it is something we should shy away unless we
> determine its completely necessary.
> 
> So what's wrong with this driver failing at direct access, which should be fast,
> and relying on a uevent to then work using the current fallback mechanisms?
> 
> The commit log in no way documents any of the justifications for further
> extending use of the usermode helper.

Hi! See patch 6/6. It is needed to avoid direct access and wl1251 on
Nokia N900 needs to use userspace helper which prepares firmware data.

Direct access is just fallback when userspace helper is not available.
Without userspace helper on devices where wl1251 do not have own eeprom
memory, wl1251 cannot work.

I know that usermode helper is not well designed, but it is the best
option what we can do for wl1251.

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

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 10:08:19PM +0100, Pali Rohár wrote:
> On Friday 10 November 2017 21:26:01 Luis R. Rodriguez wrote:
> > On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> > > This function works pretty much like request_firmware(), but it prefer
> > > usermode helper. If usermode helper fails then it fallback to direct
> > > access. Useful for dynamic or model specific firmware data.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > >  drivers/base/firmware_class.c |   45 +++++++++++++++++++++++++++++++++++++++--
> > >  include/linux/firmware.h      |    9 +++++++++
> > >  2 files changed, 52 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 4b57cf5..c3a9fe5 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
> > >  #endif
> > >  #define FW_OPT_NO_WARN	(1U << 3)
> > >  #define FW_OPT_NOCACHE	(1U << 4)
> > > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> > > +#define FW_OPT_PREFER_USER	(1U << 5)
> > > +#else
> > > +#define FW_OPT_PREFER_USER	0
> > > +#endif
> > 
> > I've been cleaning these up these flags [0], which I'll shortly respin based
> > on feedback, so this sort of stuff should be avoided at all costs.
> > 
> > Regardless of this even if you *leave* the flag in place and a driver required
> > this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> > calling fw_load_from_user_helper would just already return -ENOENT, as such it
> > would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> > needed.
> > 
> > [0] https://lkml.kernel.org/r/20170914225422.31034-1-mcgrof@kernel.org
> > 
> > >  struct firmware_cache {
> > >  	/* firmware_buf instance will be added into the below list */
> > > @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
> > >  	if (ret <= 0) /* error or already assigned */
> > >  		goto out;
> > >  
> > > -	ret = fw_get_filesystem_firmware(device, fw->priv);
> > > +	if (opt_flags & FW_OPT_PREFER_USER) {
> > > +		ret = fw_load_from_user_helper(fw, name, device, opt_flags, timeout);
> > > +		if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> > > +			dev_warn(device,
> > > +				 "User helper firmware load for %s failed with error %d\n",
> > > +				 name, ret);
> > > +			dev_warn(device, "Falling back to direct firmware load\n");
> > 
> > As I had noted before, the usermode helper was really not well designed,
> > as such extending further use of it is something we should shy away unless we
> > determine its completely necessary.
> > 
> > So what's wrong with this driver failing at direct access, which should be fast,
> > and relying on a uevent to then work using the current fallback mechanisms?
> > 
> > The commit log in no way documents any of the justifications for further
> > extending use of the usermode helper.
> 
> Hi! See patch 6/6. It is needed to avoid direct access and wl1251 on
> Nokia N900 needs to use userspace helper which prepares firmware data.

My point is your commit log in no way describes the shortcomings of the
current affairs for device drivers which only can access the data it
needs using the firmware fallback mechanism.

In order for a change to go in, specially if its extending use of the
fallback mechanism through sysfs now as primary citizen, the justification
should be well documented on the commit log.

For instance you may want to highlight that what I documented here:

https://www.kernel.org/doc/html/latest/driver-api/firmware/fallback-mechanisms.html

"CONFIG_FW_LOADER_USER_HELPER: enables building the firmware fallback
mechanism. Most distributions enable this option today. If enabled but
CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled, only the custom fallback
mechanism is available and for the request_firmware_nowait() call."

Since most distros disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK, in practice
this means the fallback mechanism is never actually triggered, and the
only way to do that in practice this for most distros is to use the
custom fallback mechanism, and the only difference there is that the
custom fallback mechanism has an infinite timeout since we have no clear
way to know what it is or when it will complete, other than when it
actually does its work.

That begs the question, why cant you just use request_firmware_nowait()
with the custom fallback mechanism?

Your commit log should explain the shortcomings of the current API.

Also note that "usermode helper" refers to kernel/umh.c, and the only
code being used from that UMH API is the usermodehelper_read_lock_wait()
on async, or usermodehelper_read_trylock() on sync. Nothing else. The
rest of the firmware uploading is done via a sysfs file upload mechanism.
Its precisely why I documented the fallback mechanism instead as:

"Firmware sysfs loading facility"

Adding any other flag which reflects "UMH" only will confuse people further. If
you are going to add a new flag, perhaps "FW_OPT_PREFER_SYSFS_LOAD" is much
more suitable.

> Direct access is just fallback when userspace helper is not available.
> Without userspace helper on devices where wl1251 do not have own eeprom
> memory, wl1251 cannot work.

That is not an explanation as to why request_firmware_nowait() could not
be used with the custom fallback interface.

> I know that usermode helper is not well designed, but it is the best
> option what we can do for wl1251.

It does not seem like you have tested the custom fallback mechanism, nor have
you documented properly the justification for extending and making use of the
sysfs loading facility a first option for loading firmware.

  Luis

Re: [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900

[Pali Rohár]

On Friday 10 November 2017 00:38:22 Pali Rohár wrote:
> This patch series fix processing MAC address for wl1251 chip found in Nokia N900.
> 
> Changes since v1:
> * Added Acked-by for Pavel Machek
> * Fixed grammar
> * Magic numbers for NVS offsets are replaced by defines
> * Check for validity of mac address NVS data is moved into function
> * Changed order of patches as Pavel requested
> 
> Pali Rohár (6):
>   wl1251: Update wl->nvs_len after wl->nvs is valid
>   wl1251: Generate random MAC address only if driver does not have
>     valid
>   wl1251: Parse and use MAC address from supplied NVS data
>   wl1251: Set generated MAC address back to NVS data
>   firmware: Add request_firmware_prefer_user() function
>   wl1251: Use request_firmware_prefer_user() for loading NVS
>     calibration data
> 
>  drivers/base/firmware_class.c          |   45 +++++++++++++-
>  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>  drivers/net/wireless/ti/wl1251/main.c  |  104 ++++++++++++++++++++++++++------
>  include/linux/firmware.h               |    9 +++
>  4 files changed, 138 insertions(+), 21 deletions(-)

Hi! Are there any comments for first 4 patches? If not, could they be
accepted and merged?

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

Re: [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900

[Luis R. Rodriguez]

On Tue, Jan 02, 2018 at 08:23:45PM +0100, Pali Rohár wrote:
> On Friday 10 November 2017 00:38:22 Pali Rohár wrote:
> > This patch series fix processing MAC address for wl1251 chip found in Nokia N900.
> > 
> > Changes since v1:
> > * Added Acked-by for Pavel Machek
> > * Fixed grammar
> > * Magic numbers for NVS offsets are replaced by defines
> > * Check for validity of mac address NVS data is moved into function
> > * Changed order of patches as Pavel requested
> > 
> > Pali Rohár (6):
> >   wl1251: Update wl->nvs_len after wl->nvs is valid
> >   wl1251: Generate random MAC address only if driver does not have
> >     valid
> >   wl1251: Parse and use MAC address from supplied NVS data
> >   wl1251: Set generated MAC address back to NVS data
> >   firmware: Add request_firmware_prefer_user() function
> >   wl1251: Use request_firmware_prefer_user() for loading NVS
> >     calibration data
> > 
> >  drivers/base/firmware_class.c          |   45 +++++++++++++-
> >  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
> >  drivers/net/wireless/ti/wl1251/main.c  |  104 ++++++++++++++++++++++++++------
> >  include/linux/firmware.h               |    9 +++
> >  4 files changed, 138 insertions(+), 21 deletions(-)
> 
> Hi! Are there any comments for first 4 patches? If not, could they be
> accepted and merged?

Since the first 4 patches do not touch the firmware API they seem fine to me so
long as the maintainer accepts them. Maybe resend and clarify you have dropped
the other ones and amend with the new tags.

  Luis

Re: [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]

On Friday 05 January 2018 02:45:10 Luis R. Rodriguez wrote:
> On Tue, Jan 02, 2018 at 08:23:45PM +0100, Pali Rohár wrote:
> > On Friday 10 November 2017 00:38:22 Pali Rohár wrote:
> > > This patch series fix processing MAC address for wl1251 chip found in Nokia N900.
> > > 
> > > Changes since v1:
> > > * Added Acked-by for Pavel Machek
> > > * Fixed grammar
> > > * Magic numbers for NVS offsets are replaced by defines
> > > * Check for validity of mac address NVS data is moved into function
> > > * Changed order of patches as Pavel requested
> > > 
> > > Pali Rohár (6):
> > >   wl1251: Update wl->nvs_len after wl->nvs is valid
> > >   wl1251: Generate random MAC address only if driver does not have
> > >     valid
> > >   wl1251: Parse and use MAC address from supplied NVS data
> > >   wl1251: Set generated MAC address back to NVS data
> > >   firmware: Add request_firmware_prefer_user() function
> > >   wl1251: Use request_firmware_prefer_user() for loading NVS
> > >     calibration data
> > > 
> > >  drivers/base/firmware_class.c          |   45 +++++++++++++-
> > >  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
> > >  drivers/net/wireless/ti/wl1251/main.c  |  104 ++++++++++++++++++++++++++------
> > >  include/linux/firmware.h               |    9 +++
> > >  4 files changed, 138 insertions(+), 21 deletions(-)
> > 
> > Hi! Are there any comments for first 4 patches? If not, could they be
> > accepted and merged?
> 
> Since the first 4 patches do not touch the firmware API they seem fine to me so
> long as the maintainer accepts them. Maybe resend and clarify you have dropped
> the other ones and amend with the new tags.

According to get_maintainer.pl, Kalle Valo is maintainer.

Kalle Valo, if you do not have any other comments, can you accept first
4 patches? Or do you really need to resent first 4 patches again?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900

[Kalle Valo]

Pali Rohár <pali.rohar@gmail.com> writes:

> On Friday 05 January 2018 02:45:10 Luis R. Rodriguez wrote:
>> On Tue, Jan 02, 2018 at 08:23:45PM +0100, Pali Rohár wrote:
>> > On Friday 10 November 2017 00:38:22 Pali Rohár wrote:
>> > > This patch series fix processing MAC address for wl1251 chip found in Nokia N900.
>> > > 
>> > > Changes since v1:
>> > > * Added Acked-by for Pavel Machek
>> > > * Fixed grammar
>> > > * Magic numbers for NVS offsets are replaced by defines
>> > > * Check for validity of mac address NVS data is moved into function
>> > > * Changed order of patches as Pavel requested
>> > > 
>> > > Pali Rohár (6):
>> > >   wl1251: Update wl->nvs_len after wl->nvs is valid
>> > >   wl1251: Generate random MAC address only if driver does not have
>> > >     valid
>> > >   wl1251: Parse and use MAC address from supplied NVS data
>> > >   wl1251: Set generated MAC address back to NVS data
>> > >   firmware: Add request_firmware_prefer_user() function
>> > >   wl1251: Use request_firmware_prefer_user() for loading NVS
>> > >     calibration data
>> > > 
>> > >  drivers/base/firmware_class.c          |   45 +++++++++++++-
>> > >  drivers/net/wireless/ti/wl1251/Kconfig |    1 +
>> > >  drivers/net/wireless/ti/wl1251/main.c  |  104 ++++++++++++++++++++++++++------
>> > >  include/linux/firmware.h               |    9 +++
>> > >  4 files changed, 138 insertions(+), 21 deletions(-)
>> > 
>> > Hi! Are there any comments for first 4 patches? If not, could they be
>> > accepted and merged?
>> 
>> Since the first 4 patches do not touch the firmware API they seem fine to me so
>> long as the maintainer accepts them. Maybe resend and clarify you have dropped
>> the other ones and amend with the new tags.
>
> According to get_maintainer.pl, Kalle Valo is maintainer.
>
> Kalle Valo, if you do not have any other comments, can you accept first
> 4 patches? Or do you really need to resent first 4 patches again?

The first 4 patches are now in my queue, I should get to them in the
next few days.

-- 
Kalle Valo

Re: [v2,1/6] wl1251: Update wl->nvs_len after wl->nvs is valid

[Kalle Valo]

Pali Rohár wrote:

> If kmemdup fails, then wl->nvs_len will contain invalid non-zero size.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>

4 patches applied to wireless-drivers-next.git, thanks.

f63b4c971f5f wl1251: Update wl->nvs_len after wl->nvs is valid
562da3a39cb7 wl1251: Generate random MAC address only if driver does not have valid
4f507d588d08 wl1251: Parse and use MAC address from supplied NVS data
3142467fc15b wl1251: Set generated MAC address back to NVS data

-- 
https://patchwork.kernel.org/patch/10052101/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [v2,1/6] wl1251: Update wl->nvs_len after wl->nvs is valid

[Kalle Valo]

Pali Rohár wrote:

> If kmemdup fails, then wl->nvs_len will contain invalid non-zero size.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>

4 patches applied to wireless-drivers-next.git, thanks.

f63b4c971f5f wl1251: Update wl->nvs_len after wl->nvs is valid
562da3a39cb7 wl1251: Generate random MAC address only if driver does not have valid
4f507d588d08 wl1251: Parse and use MAC address from supplied NVS data
3142467fc15b wl1251: Set generated MAC address back to NVS data

-- 
https://patchwork.kernel.org/patch/10052101/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches