[PATCH 0/2] media: tuner xc5000 firmware load fixes and improvements

[PATCH 0/2] media: tuner xc5000 firmware load fixes and improvements

[Shuah Khan]

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(-)

-- 
1.7.10.4

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

[Shuah Khan]

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.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/media/tuners/xc5000.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index 512fe50..31b1dec 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -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);
-- 
1.7.10.4

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

[Mauro Carvalho Chehab]

Em Wed, 13 Aug 2014 19:09:23 -0600
Shuah Khan <shuah.kh@samsung.com> escreveu:

> 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.
> 
> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> ---
>  drivers/media/tuners/xc5000.c |   34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
> index 512fe50..31b1dec 100644
> --- a/drivers/media/tuners/xc5000.c
> +++ b/drivers/media/tuners/xc5000.c
> @@ -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;

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

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

[Shuah Khan]

On 09/22/2014 02:18 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 13 Aug 2014 19:09:23 -0600
> Shuah Khan <shuah.kh@samsung.com> escreveu:
> 
>> 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.
>>
>> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
>> ---
>>  drivers/media/tuners/xc5000.c |   34 ++++++++++++++++++++--------------
>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
>> index 512fe50..31b1dec 100644
>> --- a/drivers/media/tuners/xc5000.c
>> +++ b/drivers/media/tuners/xc5000.c
>> @@ -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;
> 
> 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


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

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

[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.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/media/tuners/xc5000.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index 31b1dec..d853cb3 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -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,
-- 
1.7.10.4