diff mbox

hwrng: OMAP: Fix assumption that runtime_get_sync will always succeed

Message ID 1466783867-25743-1-git-send-email-nm@ti.com
State Superseded
Headers show

Commit Message

Nishanth Menon June 24, 2016, 3:57 p.m. UTC
pm_runtime_get_sync does return a error value that must be checked for
error conditions, else, due to various reasons, the device maynot be
enabled and the system will crash due to lack of clock to the hardware
module.

Before:
12.562784] [00000000] *pgd=fe193835
12.562792] Internal error: : 1406 [#1] SMP ARM
[...]
12.562864] CPU: 1 PID: 241 Comm: modprobe Not tainted 4.7.0-rc4-next-20160624 #2
12.562867] Hardware name: Generic DRA74X (Flattened Device Tree)
12.562872] task: ed51f140 ti: ed44c000 task.ti: ed44c000
12.562886] PC is at omap4_rng_init+0x20/0x84 [omap_rng]
12.562899] LR is at set_current_rng+0xc0/0x154 [rng_core]
[...]

After the proper checks:
[   94.366705] omap_rng 48090000.rng: _od_fail_runtime_resume: FIXME:
missing hwmod/omap_dev info
[   94.375767] omap_rng 48090000.rng: Failed to runtime_get device -19
[   94.382351] omap_rng 48090000.rng: initialization failed.

Fixes: 665d92fa85b5 ("hwrng: OMAP: convert to use runtime PM")
Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Nishanth Menon <nm@ti.com>

---

Patch based on: next-20160624
Issue seen with next-20160624
Full crash log: http://pastebin.ubuntu.com/17801376/

 drivers/char/hw_random/omap-rng.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

-- 
2.9.0

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

Comments

Suman Anna June 24, 2016, 4:35 p.m. UTC | #1
Hi Nishanth,

On 06/24/2016 10:57 AM, Menon, Nishanth wrote:
> pm_runtime_get_sync does return a error value that must be checked for

> error conditions, else, due to various reasons, the device maynot be

> enabled and the system will crash due to lack of clock to the hardware

> module.

> 

> Before:

> 12.562784] [00000000] *pgd=fe193835

> 12.562792] Internal error: : 1406 [#1] SMP ARM

> [...]

> 12.562864] CPU: 1 PID: 241 Comm: modprobe Not tainted 4.7.0-rc4-next-20160624 #2

> 12.562867] Hardware name: Generic DRA74X (Flattened Device Tree)

> 12.562872] task: ed51f140 ti: ed44c000 task.ti: ed44c000

> 12.562886] PC is at omap4_rng_init+0x20/0x84 [omap_rng]

> 12.562899] LR is at set_current_rng+0xc0/0x154 [rng_core]

> [...]

> 

> After the proper checks:

> [   94.366705] omap_rng 48090000.rng: _od_fail_runtime_resume: FIXME:

> missing hwmod/omap_dev info

> [   94.375767] omap_rng 48090000.rng: Failed to runtime_get device -19

> [   94.382351] omap_rng 48090000.rng: initialization failed.

> 

> Fixes: 665d92fa85b5 ("hwrng: OMAP: convert to use runtime PM")

> Cc: Paul Walmsley <paul@pwsan.com>

> Signed-off-by: Nishanth Menon <nm@ti.com>

> ---

> 

> Patch based on: next-20160624

> Issue seen with next-20160624

> Full crash log: http://pastebin.ubuntu.com/17801376/

> 

>  drivers/char/hw_random/omap-rng.c | 14 ++++++++++++--

>  1 file changed, 12 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c

> index 8a1432e8bb80..f30a1870cb64 100644

> --- a/drivers/char/hw_random/omap-rng.c

> +++ b/drivers/char/hw_random/omap-rng.c

> @@ -384,7 +384,11 @@ static int omap_rng_probe(struct platform_device *pdev)

>  	}

>  

>  	pm_runtime_enable(&pdev->dev);

> -	pm_runtime_get_sync(&pdev->dev);

> +	ret = pm_runtime_get_sync(&pdev->dev);

> +	if (ret) {

> +		dev_err(&pdev->dev, "Failed to runtime_get device: %d\n", ret);

> +		goto err_ioremap;


You need to also add a pm_runtime_put_noidle() on get_sync failures to
balance the device's power usage_count.

regards
Suman

> +	}

>  

>  	ret = (dev->of_node) ? of_get_omap_rng_device_details(priv, pdev) :

>  				get_omap_rng_device_details(priv);

> @@ -435,8 +439,14 @@ static int __maybe_unused omap_rng_suspend(struct device *dev)

>  static int __maybe_unused omap_rng_resume(struct device *dev)

>  {

>  	struct omap_rng_dev *priv = dev_get_drvdata(dev);

> +	int ret;

> +

> +	ret = pm_runtime_get_sync(dev);

> +	if (ret) {

> +		dev_err(dev, "Failed to runtime_get device: %d\n", ret);

> +		return ret;

> +	}

>  

> -	pm_runtime_get_sync(dev);

>  	priv->pdata->init(priv);

>  

>  	return 0;

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon June 24, 2016, 4:46 p.m. UTC | #2
On 06/24/2016 11:35 AM, Suman Anna wrote:
[...]
>> diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c

>> index 8a1432e8bb80..f30a1870cb64 100644

>> --- a/drivers/char/hw_random/omap-rng.c

>> +++ b/drivers/char/hw_random/omap-rng.c

>> @@ -384,7 +384,11 @@ static int omap_rng_probe(struct platform_device *pdev)

>>  	}

>>  

>>  	pm_runtime_enable(&pdev->dev);

>> -	pm_runtime_get_sync(&pdev->dev);

>> +	ret = pm_runtime_get_sync(&pdev->dev);

>> +	if (ret) {

>> +		dev_err(&pdev->dev, "Failed to runtime_get device: %d\n", ret);

>> +		goto err_ioremap;

> 

> You need to also add a pm_runtime_put_noidle() on get_sync failures to

> balance the device's power usage_count.

> 


Crap, yep, I should have known better. thanks for catching it.. v2
incoming..


-- 
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" 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/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c
index 8a1432e8bb80..f30a1870cb64 100644
--- a/drivers/char/hw_random/omap-rng.c
+++ b/drivers/char/hw_random/omap-rng.c
@@ -384,7 +384,11 @@  static int omap_rng_probe(struct platform_device *pdev)
 	}
 
 	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to runtime_get device: %d\n", ret);
+		goto err_ioremap;
+	}
 
 	ret = (dev->of_node) ? of_get_omap_rng_device_details(priv, pdev) :
 				get_omap_rng_device_details(priv);
@@ -435,8 +439,14 @@  static int __maybe_unused omap_rng_suspend(struct device *dev)
 static int __maybe_unused omap_rng_resume(struct device *dev)
 {
 	struct omap_rng_dev *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret) {
+		dev_err(dev, "Failed to runtime_get device: %d\n", ret);
+		return ret;
+	}
 
-	pm_runtime_get_sync(dev);
 	priv->pdata->init(priv);
 
 	return 0;