Skip to contentSkip to conversation

media: tuner xc5000 firmware load fixes and improvements

PATCH In review Shuah Khan 3 participants 5 messages Aug 14, 2014 – Sep 22, 2014 Media (V4L/DVB)

lore
Share & tools
lore

Shuah Khan

author
#0 lore raw

cover media: tuner xc5000 firmware load fixes and improvements

This patch fixes firmware load issues seen during suspend and resume and speeds up firmware force load path. Releasing firmware from xc5000_release() instead of right after load helps avoid requesting firmware when it needs to be force loaded and fixes slowpath warnings during resume after suspend is done before firmware is loaded.

Shuah Khan (2):
media: tuner xc5000 - release firmwware from xc5000_release()
media: tuner xc5000 - try to avoid firmware load in resume path

 drivers/media/tuners/xc5000.c |   50 ++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 15 deletions(-)
Signature · 1 line
1.7.10.4
author
#1 lore raw

PATCH 1/2 media: tuner xc5000 - release firmwware from xc5000_release()

xc5000 releases firmware right after loading it. Change it to save the firmware and release it from xc5000_release(). This helps avoid fecthing firmware when forced firmware load requests come in to change analog tv frequence and when firmware needs to be reloaded after suspend and resume.

---
 drivers/media/tuners/xc5000.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
drivers/media/tuners/xc5000.c +20 -14
index 512fe50..31b1dec 100644
@@ -70,6 +70,8 @@ struct xc5000_priv {
 
 	struct dvb_frontend *fe;
 	struct delayed_work timer_sleep;
+
+	const struct firmware   *firmware;
 };
 
 /* Misc Defines */
@@ -1136,20 +1138,23 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
 	if (!force && xc5000_is_firmware_loaded(fe) == 0)
 		return 0;
 
-	ret = request_firmware(&fw, desired_fw->name,
-			       priv->i2c_props.adap->dev.parent);
-	if (ret) {
-		printk(KERN_ERR "xc5000: Upload failed. (file not found?)\n");
-		return ret;
-	}
-
-	dprintk(1, "firmware read %Zu bytes.\n", fw->size);
+	if (!priv->firmware) {
+		ret = request_firmware(&fw, desired_fw->name,
+					priv->i2c_props.adap->dev.parent);
+		if (ret) {
+			pr_err("xc5000: Upload failed. rc %d\n", ret);
+			return ret;
+		}
+		dprintk(1, "firmware read %Zu bytes.\n", fw->size);
 
-	if (fw->size != desired_fw->size) {
-		printk(KERN_ERR "xc5000: Firmware file with incorrect size\n");
-		ret = -EINVAL;
-		goto err;
-	}
+		if (fw->size != desired_fw->size) {
+			pr_err("xc5000: Firmware file with incorrect size\n");
+			ret = -EINVAL;
+			goto err;
+		}
+		priv->firmware = fw;
+	} else
+		fw = priv->firmware;
 
 	/* Try up to 5 times to load firmware */
 	for (i = 0; i < 5; i++) {
@@ -1232,7 +1237,6 @@ err:
 	else
 		printk(KERN_CONT " - too many retries. Giving up\n");
 
-	release_firmware(fw);
 	return ret;
 }
 
@@ -1316,6 +1320,8 @@ static int xc5000_release(struct dvb_frontend *fe)
 	if (priv) {
 		cancel_delayed_work(&priv->timer_sleep);
 		hybrid_tuner_release_state(priv);
+		if (priv->firmware)
+			release_firmware(priv->firmware);
 	}
 
 	mutex_unlock(&xc5000_list_mutex);
Signature · 1 line
1.7.10.4
#2 lore raw

Re: [PATCH 1/2] media: tuner xc5000 - release firmwware from xc5000_release()

Em Wed, 13 Aug 2014 19:09:23 -0600
Shuah Khan <[email protected]> escreveu:

Quoted 44 lines

xc5000 releases firmware right after loading it. Change it to save the firmware and release it from xc5000_release(). This helps avoid fecthing firmware when forced firmware load requests come in to change analog tv frequence and when firmware needs to be reloaded after suspend and resume.

---
 drivers/media/tuners/xc5000.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
drivers/media/tuners/xc5000.c +10 -8
index 512fe50..31b1dec 100644
@@ -70,6 +70,8 @@ struct xc5000_priv {
 
 	struct dvb_frontend *fe;
 	struct delayed_work timer_sleep;
+
+	const struct firmware   *firmware;
 };
 
 /* Misc Defines */
@@ -1136,20 +1138,23 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
 	if (!force && xc5000_is_firmware_loaded(fe) == 0)
 		return 0;
 
-	ret = request_firmware(&fw, desired_fw->name,
-			       priv->i2c_props.adap->dev.parent);
-	if (ret) {
-		printk(KERN_ERR "xc5000: Upload failed. (file not found?)\n");
-		return ret;
-	}
-
-	dprintk(1, "firmware read %Zu bytes.\n", fw->size);
+	if (!priv->firmware) {
+		ret = request_firmware(&fw, desired_fw->name,
+					priv->i2c_props.adap->dev.parent);
+		if (ret) {
+			pr_err("xc5000: Upload failed. rc %d\n", ret);
+			return ret;
+		}
+		dprintk(1, "firmware read %Zu bytes.\n", fw->size);

drivers/media/tuners/xc5000.c static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) · line 1153

Quoted 10 lines
drivers/media/tuners/xc5000.c +4 -5
 
-	if (fw->size != desired_fw->size) {
-		printk(KERN_ERR "xc5000: Firmware file with incorrect size\n");
-		ret = -EINVAL;
-		goto err;
-	}
+		if (fw->size != desired_fw->size) {
+			pr_err("xc5000: Firmware file with incorrect size\n");
+			ret = -EINVAL;
+			goto err;

In this case, we should be releasing the firmware too. Btw, this code will also add a memory leak, as priv->firmware will be null, but the firmware was loaded.

The rest of the patch looks ok.

Regards,
Mauro

#3 lore raw

Re: [PATCH 1/2] media: tuner xc5000 - release firmwware from xc5000_release()

Quoted 48 lines · Mauro Carvalho Chehab

Em Wed, 13 Aug 2014 19:09:23 -0600
Shuah Khan <[email protected]> escreveu:

Quoted 45 lines

xc5000 releases firmware right after loading it. Change it to save the firmware and release it from xc5000_release(). This helps avoid fecthing firmware when forced firmware load requests come in to change analog tv frequence and when firmware needs to be reloaded after suspend and resume.

---
 drivers/media/tuners/xc5000.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
drivers/media/tuners/xc5000.c +10 -8
index 512fe50..31b1dec 100644
@@ -70,6 +70,8 @@ struct xc5000_priv {
 
 	struct dvb_frontend *fe;
 	struct delayed_work timer_sleep;
+
+	const struct firmware   *firmware;
 };
 
 /* Misc Defines */
@@ -1136,20 +1138,23 @@ static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force)
 	if (!force && xc5000_is_firmware_loaded(fe) == 0)
 		return 0;
 
-	ret = request_firmware(&fw, desired_fw->name,
-			       priv->i2c_props.adap->dev.parent);
-	if (ret) {
-		printk(KERN_ERR "xc5000: Upload failed. (file not found?)\n");
-		return ret;
-	}
-
-	dprintk(1, "firmware read %Zu bytes.\n", fw->size);
+	if (!priv->firmware) {
+		ret = request_firmware(&fw, desired_fw->name,
+					priv->i2c_props.adap->dev.parent);
+		if (ret) {
+			pr_err("xc5000: Upload failed. rc %d\n", ret);
+			return ret;
+		}
+		dprintk(1, "firmware read %Zu bytes.\n", fw->size);
 
Quoted 13 lines

drivers/media/tuners/xc5000.c static int xc_load_fw_and_init_tuner(struct dvb_frontend *fe, int force) · line 1153

Quoted 9 lines
drivers/media/tuners/xc5000.c +4 -5
-	if (fw->size != desired_fw->size) {
-		printk(KERN_ERR "xc5000: Firmware file with incorrect size\n");
-		ret = -EINVAL;
-		goto err;
-	}
+		if (fw->size != desired_fw->size) {
+			pr_err("xc5000: Firmware file with incorrect size\n");
+			ret = -EINVAL;
+			goto err;

In this case, we should be releasing the firmware too. Btw, this code will also add a memory leak, as priv->firmware will be null, but the firmware was loaded.

Yes you are right. I will revise the patch series and resend it.

-- Shuah

Signature · 4 lines
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
[email protected] | (970) 217-8978
author
#4 lore raw

PATCH 2/2 media: tuner xc5000 - try to avoid firmware load in resume path

In reply to Shuah Khan

xc5000 doesn't load firmware at attach time instead loads it when it needs to set and change configuration from its init, frequency, digital and analog mode set interffaces. As a result, when system is suspended before firmware is loaded, firmware load can be avoided during resume. Loading formware in this scenario results in slowpath warnings during resume as it won't be in the suspend firmware cache.

---
 drivers/media/tuners/xc5000.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/media/tuners/xc5000.c +15 -1
index 31b1dec..d853cb3 100644
@@ -1293,6 +1293,20 @@ static int xc5000_suspend(struct dvb_frontend *fe)
 	return 0;
 }
 
+static int xc5000_resume(struct dvb_frontend *fe)
+{
+	struct xc5000_priv *priv = fe->tuner_priv;
+
+	dprintk(1, "%s()\n", __func__);
+
+	/* suspended before firmware is loaded.
+	   Avoid firmware load in resume path. */
+	if (!priv->firmware)
+		return 0;
+
+	return xc5000_set_params(fe);
+}
+
 static int xc5000_init(struct dvb_frontend *fe)
 {
 	struct xc5000_priv *priv = fe->tuner_priv;
@@ -1360,7 +1374,7 @@ static const struct dvb_tuner_ops xc5000_tuner_ops = {
 	.init		   = xc5000_init,
 	.sleep		   = xc5000_sleep,
 	.suspend	   = xc5000_suspend,
-	.resume		   = xc5000_set_params,
+	.resume		   = xc5000_resume,
 
 	.set_config	   = xc5000_set_config,
 	.set_params	   = xc5000_set_digital_params,
Signature · 1 line
1.7.10.4