[PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[Ben Dooks]

The pm_rumtime work queue is causing the device to be suspended during
initialisation, thus the initialisation may not be able to access registers
properly. As the code is called from a work queue, it is possible that this
is not seen from certain configurations/builds due to the asynchronos
nature of the code.

Another issue has also been found where the network device registration
calls back into the driver thus causing further pm_runtime calls that
also caused issues with the MDIO bus code. This has now been checked
and is the only place the MDIO can be called without the device open.

Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the
pm system does not suspend it during the probe() call and remove the
now unnecessary pm_runtime_resume() call. Also add a call in the error
path to call pm_runtime_disable().

This fixes the external abort that can cause /sbin/init or other such
init processed to die.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

---
Cc: sergei.shtylyov@cogentembedded.com
Cc: laurent.pinchart+renesas@ideasonboard.com
cc: netdev@vger.kernel.org

Note, Laurent this should probably be applied before your
current series as it may require changes to the exit sequence.

Fixes from v1:
	- use pm_runtime_put() over pm_runtime_put_sync() as
	  we do not need to guaranteed the device has powered
          off after probe.

Fixed from v2:
	- call pm_runtime_put() in error path

Fixed from v3:
	- call pm_runtime_disable() in error path

Conflicts:
	drivers/net/ethernet/renesas/sh_eth.c
---
 drivers/net/ethernet/renesas/sh_eth.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index e4bff18..6a9509c 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2772,6 +2772,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	if (!ndev)
 		return -ENOMEM;
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	/* The sh Ether-specific entries in the device structure. */
 	ndev->base_addr = res->start;
 	devno = pdev->id;
@@ -2799,8 +2802,6 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 
 	spin_lock_init(&mdp->lock);
 	mdp->pdev = pdev;
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_resume(&pdev->dev);
 
 	if (pdev->dev.of_node)
 		pd = sh_eth_parse_dt(&pdev->dev);
@@ -2898,6 +2899,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n",
 		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
 
+	pm_runtime_put(&pdev->dev);
 	platform_set_drvdata(pdev, ndev);
 
 	return ret;
@@ -2911,6 +2913,8 @@ out_release:
 	if (ndev)
 		free_netdev(ndev);
 
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	return ret;
 }
 
-- 
1.9.0

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[Geert Uytterhoeven]

Hi Ben,

On Fri, Mar 21, 2014 at 12:09 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> The pm_rumtime work queue is causing the device to be suspended during
> initialisation, thus the initialisation may not be able to access registers
> properly. As the code is called from a work queue, it is possible that this
> is not seen from certain configurations/builds due to the asynchronos
> nature of the code.
>
> Another issue has also been found where the network device registration
> calls back into the driver thus causing further pm_runtime calls that
> also caused issues with the MDIO bus code. This has now been checked
> and is the only place the MDIO can be called without the device open.
>
> Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the
> pm system does not suspend it during the probe() call and remove the
> now unnecessary pm_runtime_resume() call. Also add a call in the error
> path to call pm_runtime_disable().
>
> This fixes the external abort that can cause /sbin/init or other such
> init processed to die.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks, still works!

> ---
> Cc: sergei.shtylyov@cogentembedded.com
> Cc: laurent.pinchart+renesas@ideasonboard.com
> cc: netdev@vger.kernel.org
>
> Note, Laurent this should probably be applied before your
> current series as it may require changes to the exit sequence.

I guess this should read:

" Fixed from v4:
          - rebased on top of netdev-next"

?

> Fixes from v1:
>         - use pm_runtime_put() over pm_runtime_put_sync() as
>           we do not need to guaranteed the device has powered
>           off after probe.
>
> Fixed from v2:
>         - call pm_runtime_put() in error path
>
> Fixed from v3:
>         - call pm_runtime_disable() in error path

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Friday 21 March 2014 12:09:14 Ben Dooks wrote:
> The pm_rumtime work queue is causing the device to be suspended during
> initialisation, thus the initialisation may not be able to access registers
> properly. As the code is called from a work queue, it is possible that this
> is not seen from certain configurations/builds due to the asynchronos
> nature of the code.
> 
> Another issue has also been found where the network device registration
> calls back into the driver thus causing further pm_runtime calls that
> also caused issues with the MDIO bus code. This has now been checked
> and is the only place the MDIO can be called without the device open.
> 
> Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the
> pm system does not suspend it during the probe() call and remove the
> now unnecessary pm_runtime_resume() call. Also add a call in the error
> path to call pm_runtime_disable().
> 
> This fixes the external abort that can cause /sbin/init or other such
> init processed to die.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> ---
> Cc: sergei.shtylyov@cogentembedded.com
> Cc: laurent.pinchart+renesas@ideasonboard.com
> cc: netdev@vger.kernel.org
> 
> Note, Laurent this should probably be applied before your
> current series as it may require changes to the exit sequence.
> 
> Fixes from v1:
> 	- use pm_runtime_put() over pm_runtime_put_sync() as
> 	  we do not need to guaranteed the device has powered
>           off after probe.
> 
> Fixed from v2:
> 	- call pm_runtime_put() in error path
> 
> Fixed from v3:
> 	- call pm_runtime_disable() in error path
> 
> Conflicts:
> 	drivers/net/ethernet/renesas/sh_eth.c
> ---
>  drivers/net/ethernet/renesas/sh_eth.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> b/drivers/net/ethernet/renesas/sh_eth.c index e4bff18..6a9509c 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -2772,6 +2772,9 @@ static int sh_eth_drv_probe(struct platform_device
> *pdev) if (!ndev)
>  		return -ENOMEM;
> 
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	/* The sh Ether-specific entries in the device structure. */
>  	ndev->base_addr = res->start;
>  	devno = pdev->id;
> @@ -2799,8 +2802,6 @@ static int sh_eth_drv_probe(struct platform_device
> *pdev)
> 
>  	spin_lock_init(&mdp->lock);
>  	mdp->pdev = pdev;
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_resume(&pdev->dev);
> 
>  	if (pdev->dev.of_node)
>  		pd = sh_eth_parse_dt(&pdev->dev);
> @@ -2898,6 +2899,7 @@ static int sh_eth_drv_probe(struct platform_device
> *pdev) netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n",
>  		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> 
> +	pm_runtime_put(&pdev->dev);
>  	platform_set_drvdata(pdev, ndev);
> 
>  	return ret;
> @@ -2911,6 +2913,8 @@ out_release:
>  	if (ndev)
>  		free_netdev(ndev);
> 
> +	pm_runtime_put(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[David Miller]

From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Fri, 21 Mar 2014 12:09:14 +0100

> The pm_rumtime work queue is causing the device to be suspended during
> initialisation, thus the initialisation may not be able to access registers
> properly. As the code is called from a work queue, it is possible that this
> is not seen from certain configurations/builds due to the asynchronos
> nature of the code.
> 
> Another issue has also been found where the network device registration
> calls back into the driver thus causing further pm_runtime calls that
> also caused issues with the MDIO bus code. This has now been checked
> and is the only place the MDIO can be called without the device open.
> 
> Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the
> pm system does not suspend it during the probe() call and remove the
> now unnecessary pm_runtime_resume() call. Also add a call in the error
> path to call pm_runtime_disable().
> 
> This fixes the external abort that can cause /sbin/init or other such
> init processed to die.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

What tree is this against?  It doesn't apply cleanly to 'net'.

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[Ben Dooks]

On 28/03/14 18:26, David Miller wrote:
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> Date: Fri, 21 Mar 2014 12:09:14 +0100
>
>> The pm_rumtime work queue is causing the device to be suspended during
>> initialisation, thus the initialisation may not be able to access registers
>> properly. As the code is called from a work queue, it is possible that this
>> is not seen from certain configurations/builds due to the asynchronos
>> nature of the code.
>>
>> Another issue has also been found where the network device registration
>> calls back into the driver thus causing further pm_runtime calls that
>> also caused issues with the MDIO bus code. This has now been checked
>> and is the only place the MDIO can be called without the device open.
>>
>> Use pm_runtime_get_sync() and pm_runtime_put() to ensure that the
>> pm system does not suspend it during the probe() call and remove the
>> now unnecessary pm_runtime_resume() call. Also add a call in the error
>> path to call pm_runtime_disable().
>>
>> This fixes the external abort that can cause /sbin/init or other such
>> init processed to die.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> What tree is this against?  It doesn't apply cleanly to 'net'.

I thought it was against the net next tree, given the number of patches
that are currently being applied to the sh_eth driver.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[David Miller]

From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Fri, 28 Mar 2014 19:25:51 +0000

> I thought it was against the net next tree, given the number of
> patches that are currently being applied to the sh_eth driver.

Fair enough, applied to net-next, thanks!

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[Sergei Shtylyov]

Hello.

On 03/28/2014 11:00 PM, David Miller wrote:

>> I thought it was against the net next tree, given the number of
>> patches that are currently being applied to the sh_eth driver.

> Fair enough, applied to net-next, thanks!

    It probably makes sense to queue this for the stable kernels as well.

WBR, Sergei

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sat, 29 Mar 2014 00:10:20 +0300

> Hello.
> 
> On 03/28/2014 11:00 PM, David Miller wrote:
> 
>>> I thought it was against the net next tree, given the number of
>>> patches that are currently being applied to the sh_eth driver.
> 
>> Fair enough, applied to net-next, thanks!
> 
>    It probably makes sense to queue this for the stable kernels as well.

Sorry, that's not how this works.

If it's good enough for -stable, meaning that users are activly hitting
the problem and it's a serious bug, then it's good enough for 'net'
and should have been submitted against 'net'.

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[Sergei Shtylyov]

Hello.

On 29-03-2014 1:18, David Miller wrote:

>>>> I thought it was against the net next tree, given the number of
>>>> patches that are currently being applied to the sh_eth driver.

>>> Fair enough, applied to net-next, thanks!

>>     It probably makes sense to queue this for the stable kernels as well.

> Sorry, that's not how this works.

> If it's good enough for -stable, meaning that users are activly hitting
> the problem and it's a serious bug, then it's good enough for 'net'
> and should have been submitted against 'net'.

    I thought that at this point only regression fixes are good for 'net'.
Although, this can be considered a regression too -- since addition of runtime 
PM support back in 2009.

WBR, Sergei

Re: [PATCH v5] sh_eth: ensure pm_runtime cannot suspend the device during init

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sat, 29 Mar 2014 17:52:07 +0400

> Hello.
> 
> On 29-03-2014 1:18, David Miller wrote:
> 
>>>>> I thought it was against the net next tree, given the number of
>>>>> patches that are currently being applied to the sh_eth driver.
> 
>>>> Fair enough, applied to net-next, thanks!
> 
>>>     It probably makes sense to queue this for the stable kernels as well.
> 
>> Sorry, that's not how this works.
> 
>> If it's good enough for -stable, meaning that users are activly
>> hitting
>> the problem and it's a serious bug, then it's good enough for 'net'
>> and should have been submitted against 'net'.
> 
>    I thought that at this point only regression fixes are good for 'net'.
> Although, this can be considered a regression too -- since addition of
> runtime PM support back in 2009.

-stable has more stringent requirements for inclusion than 'net',
therefore it is never valid for something to go -stable which is
not in 'net'.