diff mbox

wl18xx: fallback to default conf in case of invalid conf file

Message ID 1430900994-20509-1-git-send-email-eliad@wizery.com (mailing list archive)
State Accepted
Delegated to: Kalle Valo
Headers show

Commit Message

Eliad Peller May 6, 2015, 8:29 a.m. UTC 
If the wl18xx-conf.bin file is missing or invalid (e.g. due
to recent driver change), fallback to default configuration
instead of failing driver load.

Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/ti/wl18xx/main.c | 45 +++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 20 deletions(-)

Comments

Kalle Valo May 8, 2015, 1:12 p.m. UTC | #1 
Eliad Peller <eliad@wizery.com> writes:

> If the wl18xx-conf.bin file is missing or invalid (e.g. due
> to recent driver change), fallback to default configuration
> instead of failing driver load.
>
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Eliad Peller <eliad@wizery.com>

This tells me that wl18xx is doing something wrong. The driver should
still work normally even if user space is old (and vice versa). Should
this configuration file (whatever that even is) have a version number in
file name to handle incompatible changes in a backwards compatible
manner?

But I'm still planning to apply, just wanted to point out this.
Eliad Peller May 10, 2015, 7:20 a.m. UTC | #2 
On Fri, May 8, 2015 at 4:12 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Eliad Peller <eliad@wizery.com> writes:
>
> > If the wl18xx-conf.bin file is missing or invalid (e.g. due
> > to recent driver change), fallback to default configuration
> > instead of failing driver load.
> >
> > Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Eliad Peller <eliad@wizery.com>
>
> This tells me that wl18xx is doing something wrong. The driver should
> still work normally even if user space is old (and vice versa). Should
> this configuration file (whatever that even is) have a version number in
> file name to handle incompatible changes in a backwards compatible
> manner?
>
the configuration file has a version number only in the header.
it is not backward compatible.

this is certainly not the optimal way to go, but since the driver will
now fallback to the default configuration, i guess it should be good
enough, and won't (completely) break current users.
(most users will use the default configuration anyway, so the conf
file is not that important)

Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index 717c4f5..3d38f54 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -1351,9 +1351,10 @@  out:
 }
 
 #define WL18XX_CONF_FILE_NAME "ti-connectivity/wl18xx-conf.bin"
-static int wl18xx_conf_init(struct wl1271 *wl, struct device *dev)
+
+static int wl18xx_load_conf_file(struct device *dev, struct wlcore_conf *conf,
+				 struct wl18xx_priv_conf *priv_conf)
 {
-	struct wl18xx_priv *priv = wl->priv;
 	struct wlcore_conf_file *conf_file;
 	const struct firmware *fw;
 	int ret;
@@ -1362,14 +1363,14 @@  static int wl18xx_conf_init(struct wl1271 *wl, struct device *dev)
 	if (ret < 0) {
 		wl1271_error("could not get configuration binary %s: %d",
 			     WL18XX_CONF_FILE_NAME, ret);
-		goto out_fallback;
+		return ret;
 	}
 
 	if (fw->size != WL18XX_CONF_SIZE) {
 		wl1271_error("configuration binary file size is wrong, expected %zu got %zu",
 			     WL18XX_CONF_SIZE, fw->size);
 		ret = -EINVAL;
-		goto out;
+		goto out_release;
 	}
 
 	conf_file = (struct wlcore_conf_file *) fw->data;
@@ -1379,7 +1380,7 @@  static int wl18xx_conf_init(struct wl1271 *wl, struct device *dev)
 			     "expected 0x%0x got 0x%0x", WL18XX_CONF_MAGIC,
 			     conf_file->header.magic);
 		ret = -EINVAL;
-		goto out;
+		goto out_release;
 	}
 
 	if (conf_file->header.version != cpu_to_le32(WL18XX_CONF_VERSION)) {
@@ -1387,28 +1388,32 @@  static int wl18xx_conf_init(struct wl1271 *wl, struct device *dev)
 			     "expected 0x%08x got 0x%08x",
 			     WL18XX_CONF_VERSION, conf_file->header.version);
 		ret = -EINVAL;
-		goto out;
+		goto out_release;
 	}
 
-	memcpy(&wl->conf, &conf_file->core, sizeof(wl18xx_conf));
-	memcpy(&priv->conf, &conf_file->priv, sizeof(priv->conf));
+	memcpy(conf, &conf_file->core, sizeof(*conf));
+	memcpy(priv_conf, &conf_file->priv, sizeof(*priv_conf));
 
-	goto out;
+out_release:
+	release_firmware(fw);
+	return ret;
+}
 
-out_fallback:
-	wl1271_warning("falling back to default config");
+static int wl18xx_conf_init(struct wl1271 *wl, struct device *dev)
+{
+	struct wl18xx_priv *priv = wl->priv;
 
-	/* apply driver default configuration */
-	memcpy(&wl->conf, &wl18xx_conf, sizeof(wl18xx_conf));
-	/* apply default private configuration */
-	memcpy(&priv->conf, &wl18xx_default_priv_conf, sizeof(priv->conf));
+	if (wl18xx_load_conf_file(dev, &wl->conf, &priv->conf) < 0) {
+		wl1271_warning("falling back to default config");
 
-	/* For now we just fallback */
-	return 0;
+		/* apply driver default configuration */
+		memcpy(&wl->conf, &wl18xx_conf, sizeof(wl->conf));
+		/* apply default private configuration */
+		memcpy(&priv->conf, &wl18xx_default_priv_conf,
+		       sizeof(priv->conf));
+	}
 
-out:
-	release_firmware(fw);
-	return ret;
+	return 0;
 }
 
 static int wl18xx_plt_init(struct wl1271 *wl)