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

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

[Ben Dooks]

It seems 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. Use pm_runtime_get_sync() and pm_runtime_put_sync()
to ensure that the pm system does not suspend it during the probe() call.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 4f76b5e..88aac2c 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	mdp->pdev = pdev;
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
 
 	if (pdev->dev.of_node)
 		pd = sh_eth_parse_dt(&pdev->dev);
@@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
 		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
 
+	pm_runtime_put_sync(&pdev->dev);
 	platform_set_drvdata(pdev, ndev);
 
 	return ret;
-- 
1.9.0

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

[Ben Dooks]

On 14/03/14 19:00, Ben Dooks wrote:
> It seems 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. Use pm_runtime_get_sync() and pm_runtime_put_sync()
> to ensure that the pm system does not suspend it during the probe() call.

If this work's for Geert then I'll look into updating the header and
sending it to the netdev list.


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

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

[Sergei Shtylyov]

Hello.

On 03/14/2014 10:01 PM, Ben Dooks wrote:

>> It seems 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. Use pm_runtime_get_sync() and pm_runtime_put_sync()
>> to ensure that the pm system does not suspend it during the probe() call.

> If this work's for Geert then I'll look into updating the header and
> sending it to the netdev list.

    I keep wondering why I'm seeing no issues in my setup... I don't have 
drivers/sh/ patch and have CONFIG_PM_RUNTIME=y. Seeing no issues on 
multiplatform kernels on neither Lager nor Koelsch (however, other platforms 
are not enabled).

WBR, Sergei

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

[Ben Dooks]

On 14/03/14 20:06, Sergei Shtylyov wrote:
> Hello.
>
> On 03/14/2014 10:01 PM, Ben Dooks wrote:
>
>>> It seems 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. Use pm_runtime_get_sync() and pm_runtime_put_sync()
>>> to ensure that the pm system does not suspend it during the probe()
>>> call.
>
>> If this work's for Geert then I'll look into updating the header and
>> sending it to the netdev list.
>
>     I keep wondering why I'm seeing no issues in my setup... I don't
> have drivers/sh/ patch and have CONFIG_PM_RUNTIME=y. Seeing no issues on
> multiplatform kernels on neither Lager nor Koelsch (however, other
> platforms are not enabled).

Without drivers/sh there is no specification to the pm code to do
anything with the platform or OF devices. This means the clocks are
not being auto-managed, which in turn means the clock is probably
being left on from the boot-loader and thus the unit works correctly.

Unfortunately, without drivers/sh patched a number of other drivers
fail to work. This means we need it or something similar in the kernel.

I will spend some time over the weekend moving the code out of the
drivers/sh to somewhere else so that this does not occur again.


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

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

[Sergei Shtylyov]

Hello.

On 03/14/2014 10:19 PM, Ben Dooks wrote:

>>>> It seems 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. Use pm_runtime_get_sync() and pm_runtime_put_sync()
>>>> to ensure that the pm system does not suspend it during the probe()
>>>> call.

>>> If this work's for Geert then I'll look into updating the header and
>>> sending it to the netdev list.

>>     I keep wondering why I'm seeing no issues in my setup... I don't
>> have drivers/sh/ patch and have CONFIG_PM_RUNTIME=y. Seeing no issues on
>> multiplatform kernels on neither Lager nor Koelsch (however, other
>> platforms are not enabled).

> Without drivers/sh there is no specification to the pm code to do
> anything with the platform or OF devices. This means the clocks are
> not being auto-managed, which in turn means the clock is probably
> being left on from the boot-loader and thus the unit works correctly.

    Yeah, I TFTP-boot over Ether from U-Boot. I wonder how others' setup is 
different from mine.

> Unfortunately, without drivers/sh patched a number of other drivers
> fail to work. This means we need it or something similar in the kernel.

    I wonder how drivers/sh/ came to existence at all. :-) Although there's 
also drivers/parisc/...

> I will spend some time over the weekend moving the code out of the
> drivers/sh to somewhere else so that this does not occur again.

    Thanks for your work. I just wasn't aware of these issues when I ported 
Ether to DT -- it Just Worked (TM).

WBR, Sergei

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

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
> It seems the pm_rumtime work queue is causing the device to be suspended
> during initialisation,

Have you investigated through which call path(s) this happens ? If device can 
be runtime suspended during their probe function despite calling 
pm_runtime_resume() then I don't see the point of that function at all.

> thus the initialisation may not be able to access registers properly. Use
> pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the pm system
> does not suspend it during the probe() call.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/net/ethernet/renesas/sh_eth.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct platform_device
> *pdev) mdp->pdev = pdev;
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_resume(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);

I believe pm_runtime_resume() isn't needed anymore if we call 
pm_runtime_get_sync().

>  	if (pdev->dev.of_node)
>  		pd = sh_eth_parse_dt(&pdev->dev);
> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct platform_device
> *pdev) pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>  		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> 
> +	pm_runtime_put_sync(&pdev->dev);
>  	platform_set_drvdata(pdev, ndev);
> 
>  	return ret;

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Ben,

On Fri, Mar 14, 2014 at 8:00 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> It seems 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. Use pm_runtime_get_sync() and pm_runtime_put_sync()
> to ensure that the pm system does not suspend it during the probe() call.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Thanks, this fixes the "imprecise external abort" on Koelsch.

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

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] sh_eth: ensure pm_runtime cannot suspend the device during init

[Ben Dooks]

On 17/03/14 09:22, Geert Uytterhoeven wrote:
> Hi Ben,
>
> On Fri, Mar 14, 2014 at 8:00 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> It seems 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. Use pm_runtime_get_sync() and pm_runtime_put_sync()
>> to ensure that the pm system does not suspend it during the probe() call.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>
> Thanks, this fixes the "imprecise external abort" on Koelsch.
>
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks, will consider sending this to the netdev list later.


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

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

[Ben Dooks]

On 15/03/14 11:19, Laurent Pinchart wrote:
> Hi Ben,
>
> Thank you for the patch.
>
> On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
>> It seems the pm_rumtime work queue is causing the device to be suspended
>> during initialisation,
>
> Have you investigated through which call path(s) this happens ? If device can
> be runtime suspended during their probe function despite calling
> pm_runtime_resume() then I don't see the point of that function at all.
>
>> thus the initialisation may not be able to access registers properly. Use
>> pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the pm system
>> does not suspend it during the probe() call.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/net/ethernet/renesas/sh_eth.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct platform_device
>> *pdev) mdp->pdev = pdev;
>>   	pm_runtime_enable(&pdev->dev);
>>   	pm_runtime_resume(&pdev->dev);
>> +	pm_runtime_get_sync(&pdev->dev);
>
> I believe pm_runtime_resume() isn't needed anymore if we call
> pm_runtime_get_sync().
>
>>   	if (pdev->dev.of_node)
>>   		pd = sh_eth_parse_dt(&pdev->dev);
>> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct platform_device
>> *pdev) pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>>   		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>>
>> +	pm_runtime_put_sync(&pdev->dev);
>>   	platform_set_drvdata(pdev, ndev);
>>
>>   	return ret;

I will look at removing the pm_runtime_resume call as well in this
patch.

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

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

[Laurent Pinchart]

Hi Ben,

On Monday 17 March 2014 11:20:32 Ben Dooks wrote:
> On 15/03/14 11:19, Laurent Pinchart wrote:
> > On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
> >> It seems the pm_rumtime work queue is causing the device to be suspended
> >> during initialisation,
> > 
> > Have you investigated through which call path(s) this happens ? If device
> > can be runtime suspended during their probe function despite calling
> > pm_runtime_resume() then I don't see the point of that function at all.
> > 
> >> thus the initialisation may not be able to access registers properly. Use
> >> pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the pm
> >> system does not suspend it during the probe() call.
> >> 
> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >> ---
> >> 
> >>   drivers/net/ethernet/renesas/sh_eth.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> >> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
> >> --- a/drivers/net/ethernet/renesas/sh_eth.c
> >> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> >> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct platform_device
> >> *pdev)
> >>   	mdp->pdev = pdev;
> >>   	pm_runtime_enable(&pdev->dev);
> >>   	pm_runtime_resume(&pdev->dev);
> >> +	pm_runtime_get_sync(&pdev->dev);
> > 
> > I believe pm_runtime_resume() isn't needed anymore if we call
> > pm_runtime_get_sync().
> > 
> >>   	if (pdev->dev.of_node)
> >>   		pd = sh_eth_parse_dt(&pdev->dev);
> >> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct platform_device
> >> *pdev)
> >>   	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
> >>   		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> >> 
> >> +	pm_runtime_put_sync(&pdev->dev);
> >>   	platform_set_drvdata(pdev, ndev);
> >>   	
> >>   	return ret;
> 
> I will look at removing the pm_runtime_resume call as well in this patch.

Thank you. It would also be nice if you could find out what causes the PM 
runtime work queue to suspend the device. pm_runtime_resume() is documented as 
being the function to call at probe time. I'd like to know whether the problem 
comes from the PM runtime core itself, in which case the documentation should 
be updated or the PM runtime core should be fixed, or from operations 
performed by the sh-eth driver, in which case we might need a different fix in 
the driver.

-- 
Regards,

Laurent Pinchart

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

Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
pm system does not suspend it during the probe() call and remove the
now unnecessary pm_runtime_resume() call.

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>
---
 drivers/net/ethernet/renesas/sh_eth.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 4f76b5e..f1cfd64 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2869,7 +2869,7 @@ 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);
+	pm_runtime_get_sync(&pdev->dev);
 
 	if (pdev->dev.of_node)
 		pd = sh_eth_parse_dt(&pdev->dev);
@@ -2961,6 +2961,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
 		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
 
+	pm_runtime_put_sync(&pdev->dev);
 	platform_set_drvdata(pdev, ndev);
 
 	return ret;
-- 
1.9.0

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

[Sergei Shtylyov]

Hello.

On 17-03-2014 15:37, 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.

> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
> pm system does not suspend it during the probe() call and remove the
> now unnecessary pm_runtime_resume() call.

> 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>
> ---
>   drivers/net/ethernet/renesas/sh_eth.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 4f76b5e..f1cfd64 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
> @@ -2961,6 +2961,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
>   	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>   		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>
> +	pm_runtime_put_sync(&pdev->dev);

    I think you forgot to also do that on error cleanup path.

>   	platform_set_drvdata(pdev, ndev);
>
>   	return ret;

WBR, Sergei

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

[Ben Dooks]

On 17/03/14 13:01, Sergei Shtylyov wrote:
> Hello.
>
> On 17-03-2014 15:37, 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.
>
>> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
>> pm system does not suspend it during the probe() call and remove the
>> now unnecessary pm_runtime_resume() call.
>
>> 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>
>> ---
>>   drivers/net/ethernet/renesas/sh_eth.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c
>> index 4f76b5e..f1cfd64 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> [...]
>> @@ -2961,6 +2961,7 @@ static int sh_eth_drv_probe(struct
>> platform_device *pdev)
>>       pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>>           (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>>
>> +    pm_runtime_put_sync(&pdev->dev);
>
>     I think you forgot to also do that on error cleanup path.
>
>>       platform_set_drvdata(pdev, ndev);
>>
>>       return ret;
>
> WBR, Sergei

Thanks, will check this.



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

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

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Monday 17 March 2014 11:37:55 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.
> 
> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
> pm system does not suspend it during the probe() call and remove the
> now unnecessary pm_runtime_resume() call.
> 
> 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>
> ---
>  drivers/net/ethernet/renesas/sh_eth.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..f1cfd64 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -2869,7 +2869,7 @@ 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);
> +	pm_runtime_get_sync(&pdev->dev);

Now that we've found out that the problem is caused by registering the network 
device before registering the mdio bus, shouldn't the proper solution be to 
register the network device last in the probe function ?

> 
>  	if (pdev->dev.of_node)
>  		pd = sh_eth_parse_dt(&pdev->dev);
> @@ -2961,6 +2961,7 @@ static int sh_eth_drv_probe(struct platform_device
> *pdev) pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>  		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> 
> +	pm_runtime_put_sync(&pdev->dev);
>  	platform_set_drvdata(pdev, ndev);
> 
>  	return ret;

-- 
Regards,

Laurent Pinchart

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

[Sergei Shtylyov]

Hello.

On 03/17/2014 11:23 PM, Laurent Pinchart 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.

>> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
>> pm system does not suspend it during the probe() call and remove the
>> now unnecessary pm_runtime_resume() call.

>> 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>
>> ---
>>   drivers/net/ethernet/renesas/sh_eth.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)

>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..f1cfd64 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -2869,7 +2869,7 @@ 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);
>> +	pm_runtime_get_sync(&pdev->dev);

> Now that we've found out that the problem is caused by registering the network
> device before registering the mdio bus, shouldn't the proper solution be to
> register the network device last in the probe function ?

    Unfortunately, it's not easy to do, as sh_mdio_init() uses net_device::dev.
Probably could get rid of that use by partly reverting my managed device API 
patch since the only user seems to be devm_kzalloc() in that function, at 
least I hope so... still looking into this.
    BTW, quite many drivers have the same problem, doing different things 
after register_netdev() call, many of them probing MDIO as well.

WBR, Sergei

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

[Laurent Pinchart]

Hi Sergei,

On Tuesday 18 March 2014 01:30:26 Sergei Shtylyov wrote:
> On 03/17/2014 11:23 PM, Laurent Pinchart 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.
> >> 
> >> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
> >> pm system does not suspend it during the probe() call and remove the
> >> now unnecessary pm_runtime_resume() call.
> >> 
> >> 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>
> >> ---
> >> 
> >>   drivers/net/ethernet/renesas/sh_eth.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> >> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..f1cfd64 100644
> >> --- a/drivers/net/ethernet/renesas/sh_eth.c
> >> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> >> @@ -2869,7 +2869,7 @@ 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);
> >> +	pm_runtime_get_sync(&pdev->dev);
> > 
> > Now that we've found out that the problem is caused by registering the
> > network device before registering the mdio bus, shouldn't the proper
> > solution be to register the network device last in the probe function ?
> 
> Unfortunately, it's not easy to do, as sh_mdio_init() uses net_device::dev.
> Probably could get rid of that use by partly reverting my managed device API
> patch since the only user seems to be devm_kzalloc() in that function, at
> least I hope so... still looking into this.

What about using pdev->dev instead ?

> BTW, quite many drivers have the same problem, doing different things after
> register_netdev() call, many of them probing MDIO as well.

Great, that means more opportunities to fix bugs :-D

-- 
Regards,

Laurent Pinchart

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

[Sergei Shtylyov]

Hello.

On 03/18/2014 12:34 AM, Laurent Pinchart 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.

>>>> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
>>>> pm system does not suspend it during the probe() call and remove the
>>>> now unnecessary pm_runtime_resume() call.

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

>>>>    drivers/net/ethernet/renesas/sh_eth.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)

>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..f1cfd64 100644
>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>>> @@ -2869,7 +2869,7 @@ 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);
>>>> +	pm_runtime_get_sync(&pdev->dev);

>>> Now that we've found out that the problem is caused by registering the
>>> network device before registering the mdio bus, shouldn't the proper
>>> solution be to register the network device last in the probe function ?

>> Unfortunately, it's not easy to do, as sh_mdio_init() uses net_device::dev.
>> Probably could get rid of that use by partly reverting my managed device API
>> patch since the only user seems to be devm_kzalloc() in that function, at
>> least I hope so... still looking into this.

> What about using pdev->dev instead ?

    Well, that seems possible too.

>> BTW, quite many drivers have the same problem, doing different things after
>> register_netdev() call, many of them probing MDIO as well.

> Great, that means more opportunities to fix bugs :-D

    For somebody else, maybe. My manager tells me not to spend time even on 
the 'sh_eth' driver... sigh.

WBR, Sergei

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

[Ben Dooks]

On 17/03/14 11:35, Laurent Pinchart wrote:
> Hi Ben,
>
> On Monday 17 March 2014 11:20:32 Ben Dooks wrote:
>> On 15/03/14 11:19, Laurent Pinchart wrote:
>>> On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
>>>> It seems the pm_rumtime work queue is causing the device to be suspended
>>>> during initialisation,
>>>
>>> Have you investigated through which call path(s) this happens ? If device
>>> can be runtime suspended during their probe function despite calling
>>> pm_runtime_resume() then I don't see the point of that function at all.
>>>
>>>> thus the initialisation may not be able to access registers properly. Use
>>>> pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the pm
>>>> system does not suspend it during the probe() call.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>> ---
>>>>
>>>>    drivers/net/ethernet/renesas/sh_eth.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>>> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct platform_device
>>>> *pdev)
>>>>    	mdp->pdev = pdev;
>>>>    	pm_runtime_enable(&pdev->dev);
>>>>    	pm_runtime_resume(&pdev->dev);
>>>> +	pm_runtime_get_sync(&pdev->dev);
>>>
>>> I believe pm_runtime_resume() isn't needed anymore if we call
>>> pm_runtime_get_sync().
>>>
>>>>    	if (pdev->dev.of_node)
>>>>    		pd = sh_eth_parse_dt(&pdev->dev);
>>>> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct platform_device
>>>> *pdev)
>>>>    	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>>>>    		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>>>>
>>>> +	pm_runtime_put_sync(&pdev->dev);
>>>>    	platform_set_drvdata(pdev, ndev);
>>>>    	
>>>>    	return ret;
>>
>> I will look at removing the pm_runtime_resume call as well in this patch.
>
> Thank you. It would also be nice if you could find out what causes the PM
> runtime work queue to suspend the device. pm_runtime_resume() is documented as
> being the function to call at probe time. I'd like to know whether the problem
> comes from the PM runtime core itself, in which case the documentation should
> be updated or the PM runtime core should be fixed, or from operations
> performed by the sh-eth driver, in which case we might need a different fix in
> the driver.

I'm not entirely sure, pm_runtime_resume() seems like something to call
during a resume operation. It probably does not increment device use
count, and thus the pm worker thread is allowed to re-suspend the device
when it runs.

pm_runtime_get_sync() is the right thing to do as we are dealing with
access device registers during the probe and thus should ensure the
device does not get shutdown until the probe has finished.

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

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

[Laurent Pinchart]

Hi Ben,

On Monday 17 March 2014 11:40:11 Ben Dooks wrote:
> On 17/03/14 11:35, Laurent Pinchart wrote:
> > On Monday 17 March 2014 11:20:32 Ben Dooks wrote:
> >> On 15/03/14 11:19, Laurent Pinchart wrote:
> >>> On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
> >>>> It seems the pm_rumtime work queue is causing the device to be
> >>>> suspended during initialisation,
> >>> 
> >>> Have you investigated through which call path(s) this happens ? If
> >>> device can be runtime suspended during their probe function despite
> >>> calling pm_runtime_resume() then I don't see the point of that function
> >>> at all.
> >>> 
> >>>> thus the initialisation may not be able to access registers properly.
> >>>> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
> >>>> pm system does not suspend it during the probe() call.
> >>>> 
> >>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >>>> ---
> >>>> 
> >>>>    drivers/net/ethernet/renesas/sh_eth.c | 2 ++
> >>>>    1 file changed, 2 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> >>>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
> >>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
> >>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> >>>> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct
> >>>> platform_device *pdev)
> >>>>    	mdp->pdev = pdev;
> >>>>    	pm_runtime_enable(&pdev->dev);
> >>>>    	pm_runtime_resume(&pdev->dev);
> >>>> +   	pm_runtime_get_sync(&pdev->dev);
> >>> 
> >>> I believe pm_runtime_resume() isn't needed anymore if we call
> >>> pm_runtime_get_sync().
> >>> 
> >>>>    	if (pdev->dev.of_node)
> >>>>    		pd = sh_eth_parse_dt(&pdev->dev);
> >>>> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct
> >>>> platform_device *pdev)
> >>>>    	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
> >>>>    		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> >>>> 
> >>>> +   	pm_runtime_put_sync(&pdev->dev);
> >>>>    	platform_set_drvdata(pdev, ndev);
> >>>>    	
> >>>>    	return ret;
> >> 
> >> I will look at removing the pm_runtime_resume call as well in this patch.
> > 
> > Thank you. It would also be nice if you could find out what causes the PM
> > runtime work queue to suspend the device. pm_runtime_resume() is
> > documented as being the function to call at probe time. I'd like to know
> > whether the problem comes from the PM runtime core itself, in which case
> > the documentation should be updated or the PM runtime core should be
> > fixed, or from operations performed by the sh-eth driver, in which case
> > we might need a different fix in the driver.
> 
> I'm not entirely sure, pm_runtime_resume() seems like something to call
> during a resume operation. It probably does not increment device use
> count, and thus the pm worker thread is allowed to re-suspend the device
> when it runs.

Yes, but why does it do so ? The runtime PM work queue doesn't seem to go 
through devices to try and suspend them, it seems to only process delayed 
work. What queues the work that makes runtime PM suspend the device ?

> pm_runtime_get_sync() is the right thing to do as we are dealing with
> access device registers during the probe and thus should ensure the
> device does not get shutdown until the probe has finished.

pm_runtime_resume() is documented as being the function to call in the probe 
handler. I'd like to get a clarification on this from the runtime PM 
developers. If the documentation is wrong it needs to be fixed.

-- 
Regards,

Laurent Pinchart

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

[Ben Dooks]

On 17/03/14 11:53, Laurent Pinchart wrote:
> Hi Ben,
>
> On Monday 17 March 2014 11:40:11 Ben Dooks wrote:
>> On 17/03/14 11:35, Laurent Pinchart wrote:
>>> On Monday 17 March 2014 11:20:32 Ben Dooks wrote:
>>>> On 15/03/14 11:19, Laurent Pinchart wrote:
>>>>> On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
>>>>>> It seems the pm_rumtime work queue is causing the device to be
>>>>>> suspended during initialisation,
>>>>>
>>>>> Have you investigated through which call path(s) this happens ? If
>>>>> device can be runtime suspended during their probe function despite
>>>>> calling pm_runtime_resume() then I don't see the point of that function
>>>>> at all.
>>>>>
>>>>>> thus the initialisation may not be able to access registers properly.
>>>>>> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that the
>>>>>> pm system does not suspend it during the probe() call.
>>>>>>
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>> ---
>>>>>>
>>>>>>     drivers/net/ethernet/renesas/sh_eth.c | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>>>>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
>>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>>>>> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct
>>>>>> platform_device *pdev)
>>>>>>     	mdp->pdev = pdev;
>>>>>>     	pm_runtime_enable(&pdev->dev);
>>>>>>     	pm_runtime_resume(&pdev->dev);
>>>>>> +   	pm_runtime_get_sync(&pdev->dev);
>>>>>
>>>>> I believe pm_runtime_resume() isn't needed anymore if we call
>>>>> pm_runtime_get_sync().
>>>>>
>>>>>>     	if (pdev->dev.of_node)
>>>>>>     		pd = sh_eth_parse_dt(&pdev->dev);
>>>>>> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct
>>>>>> platform_device *pdev)
>>>>>>     	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>>>>>>     		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>>>>>>
>>>>>> +   	pm_runtime_put_sync(&pdev->dev);
>>>>>>     	platform_set_drvdata(pdev, ndev);
>>>>>>     	
>>>>>>     	return ret;
>>>>
>>>> I will look at removing the pm_runtime_resume call as well in this patch.
>>>
>>> Thank you. It would also be nice if you could find out what causes the PM
>>> runtime work queue to suspend the device. pm_runtime_resume() is
>>> documented as being the function to call at probe time. I'd like to know
>>> whether the problem comes from the PM runtime core itself, in which case
>>> the documentation should be updated or the PM runtime core should be
>>> fixed, or from operations performed by the sh-eth driver, in which case
>>> we might need a different fix in the driver.
>>
>> I'm not entirely sure, pm_runtime_resume() seems like something to call
>> during a resume operation. It probably does not increment device use
>> count, and thus the pm worker thread is allowed to re-suspend the device
>> when it runs.
>
> Yes, but why does it do so ? The runtime PM work queue doesn't seem to go
> through devices to try and suspend them, it seems to only process delayed
> work. What queues the work that makes runtime PM suspend the device ?
>
>> pm_runtime_get_sync() is the right thing to do as we are dealing with
>> access device registers during the probe and thus should ensure the
>> device does not get shutdown until the probe has finished.
>
> pm_runtime_resume() is documented as being the function to call in the probe
> handler. I'd like to get a clarification on this from the runtime PM
> developers. If the documentation is wrong it needs to be fixed.

Hmm, wonder if it works for things that take a long time to run,
or for things that create and probe sub-devices such as the phy?

I annotated the clk-mstp.c driver to test for removal of the eth
clock and the culprit in this case was a pm thread running
before the device finished probing and suspending. I do not have
the exact debug output as I deleted it after looking in to it.

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

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

[Laurent Pinchart]

Hi Ben,

On Monday 17 March 2014 12:56:00 Ben Dooks wrote:
> On 17/03/14 11:53, Laurent Pinchart wrote:
> > On Monday 17 March 2014 11:40:11 Ben Dooks wrote:
> >> On 17/03/14 11:35, Laurent Pinchart wrote:
> >>> On Monday 17 March 2014 11:20:32 Ben Dooks wrote:
> >>>> On 15/03/14 11:19, Laurent Pinchart wrote:
> >>>>> On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
> >>>>>> It seems the pm_rumtime work queue is causing the device to be
> >>>>>> suspended during initialisation,
> >>>>> 
> >>>>> Have you investigated through which call path(s) this happens ? If
> >>>>> device can be runtime suspended during their probe function despite
> >>>>> calling pm_runtime_resume() then I don't see the point of that
> >>>>> function at all.
> >>>>> 
> >>>>>> thus the initialisation may not be able to access registers properly.
> >>>>>> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that
> >>>>>> the pm system does not suspend it during the probe() call.
> >>>>>> 
> >>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >>>>>> ---
> >>>>>> 
> >>>>>>     drivers/net/ethernet/renesas/sh_eth.c | 2 ++
> >>>>>>     1 file changed, 2 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> >>>>>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
> >>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
> >>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> >>>>>> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct
> >>>>>> platform_device *pdev)
> >>>>>>     	mdp->pdev = pdev;
> >>>>>>     	pm_runtime_enable(&pdev->dev);
> >>>>>>     	pm_runtime_resume(&pdev->dev);
> >>>>>> +   	pm_runtime_get_sync(&pdev->dev);
> >>>>> 
> >>>>> I believe pm_runtime_resume() isn't needed anymore if we call
> >>>>> pm_runtime_get_sync().
> >>>>> 
> >>>>>>     	if (pdev->dev.of_node)
> >>>>>>     		pd = sh_eth_parse_dt(&pdev->dev);
> >>>>>> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct
> >>>>>> platform_device *pdev)
> >>>>>>     	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
> >>>>>>     		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> >>>>>> 
> >>>>>> +   	pm_runtime_put_sync(&pdev->dev);
> >>>>>>     	platform_set_drvdata(pdev, ndev);
> >>>>>>     	
> >>>>>>     	return ret;
> >>>> 
> >>>> I will look at removing the pm_runtime_resume call as well in this
> >>>> patch.
> >>> 
> >>> Thank you. It would also be nice if you could find out what causes the
> >>> PM runtime work queue to suspend the device. pm_runtime_resume() is
> >>> documented as being the function to call at probe time. I'd like to know
> >>> whether the problem comes from the PM runtime core itself, in which case
> >>> the documentation should be updated or the PM runtime core should be
> >>> fixed, or from operations performed by the sh-eth driver, in which case
> >>> we might need a different fix in the driver.
> >> 
> >> I'm not entirely sure, pm_runtime_resume() seems like something to call
> >> during a resume operation. It probably does not increment device use
> >> count, and thus the pm worker thread is allowed to re-suspend the device
> >> when it runs.
> > 
> > Yes, but why does it do so ? The runtime PM work queue doesn't seem to go
> > through devices to try and suspend them, it seems to only process delayed
> > work. What queues the work that makes runtime PM suspend the device ?
> > 
> >> pm_runtime_get_sync() is the right thing to do as we are dealing with
> >> access device registers during the probe and thus should ensure the
> >> device does not get shutdown until the probe has finished.
> > 
> > pm_runtime_resume() is documented as being the function to call in the
> > probe handler. I'd like to get a clarification on this from the runtime
> > PM developers. If the documentation is wrong it needs to be fixed.
> 
> Hmm, wonder if it works for things that take a long time to run, or for
> things that create and probe sub-devices such as the phy?

That's exactly what I'd like to know :-)

> I annotated the clk-mstp.c driver to test for removal of the eth clock and
> the culprit in this case was a pm thread running before the device finished
> probing and suspending. I do not have the exact debug output as I deleted it
> after looking in to it.

Would you be able to perform more tests ? I'd like to get to the bottom of 
this problem and find out whether the best solution would be to fix the 
runtime PM core, the runtime PM documentation or the driver (or a combination 
of them).

-- 
Regards,

Laurent Pinchart

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

[Ben Dooks]

On 17/03/14 13:27, Laurent Pinchart wrote:
> Hi Ben,
>
> On Monday 17 March 2014 12:56:00 Ben Dooks wrote:
>> On 17/03/14 11:53, Laurent Pinchart wrote:
>>> On Monday 17 March 2014 11:40:11 Ben Dooks wrote:
>>>> On 17/03/14 11:35, Laurent Pinchart wrote:
>>>>> On Monday 17 March 2014 11:20:32 Ben Dooks wrote:
>>>>>> On 15/03/14 11:19, Laurent Pinchart wrote:
>>>>>>> On Friday 14 March 2014 19:00:05 Ben Dooks wrote:
>>>>>>>> It seems the pm_rumtime work queue is causing the device to be
>>>>>>>> suspended during initialisation,
>>>>>>>
>>>>>>> Have you investigated through which call path(s) this happens ? If
>>>>>>> device can be runtime suspended during their probe function despite
>>>>>>> calling pm_runtime_resume() then I don't see the point of that
>>>>>>> function at all.
>>>>>>>
>>>>>>>> thus the initialisation may not be able to access registers properly.
>>>>>>>> Use pm_runtime_get_sync() and pm_runtime_put_sync() to ensure that
>>>>>>>> the pm system does not suspend it during the probe() call.
>>>>>>>>
>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      drivers/net/ethernet/renesas/sh_eth.c | 2 ++
>>>>>>>>      1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>>>>>>> b/drivers/net/ethernet/renesas/sh_eth.c index 4f76b5e..88aac2c 100644
>>>>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>>>>>>> @@ -2870,6 +2870,7 @@ static int sh_eth_drv_probe(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>      	mdp->pdev = pdev;
>>>>>>>>      	pm_runtime_enable(&pdev->dev);
>>>>>>>>      	pm_runtime_resume(&pdev->dev);
>>>>>>>> +   	pm_runtime_get_sync(&pdev->dev);
>>>>>>>
>>>>>>> I believe pm_runtime_resume() isn't needed anymore if we call
>>>>>>> pm_runtime_get_sync().
>>>>>>>
>>>>>>>>      	if (pdev->dev.of_node)
>>>>>>>>      		pd = sh_eth_parse_dt(&pdev->dev);
>>>>>>>> @@ -2961,6 +2962,7 @@ static int sh_eth_drv_probe(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>      	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
>>>>>>>>      		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
>>>>>>>>
>>>>>>>> +   	pm_runtime_put_sync(&pdev->dev);
>>>>>>>>      	platform_set_drvdata(pdev, ndev);
>>>>>>>>      	
>>>>>>>>      	return ret;
>>>>>>
>>>>>> I will look at removing the pm_runtime_resume call as well in this
>>>>>> patch.
>>>>>
>>>>> Thank you. It would also be nice if you could find out what causes the
>>>>> PM runtime work queue to suspend the device. pm_runtime_resume() is
>>>>> documented as being the function to call at probe time. I'd like to know
>>>>> whether the problem comes from the PM runtime core itself, in which case
>>>>> the documentation should be updated or the PM runtime core should be
>>>>> fixed, or from operations performed by the sh-eth driver, in which case
>>>>> we might need a different fix in the driver.
>>>>
>>>> I'm not entirely sure, pm_runtime_resume() seems like something to call
>>>> during a resume operation. It probably does not increment device use
>>>> count, and thus the pm worker thread is allowed to re-suspend the device
>>>> when it runs.
>>>
>>> Yes, but why does it do so ? The runtime PM work queue doesn't seem to go
>>> through devices to try and suspend them, it seems to only process delayed
>>> work. What queues the work that makes runtime PM suspend the device ?
>>>
>>>> pm_runtime_get_sync() is the right thing to do as we are dealing with
>>>> access device registers during the probe and thus should ensure the
>>>> device does not get shutdown until the probe has finished.
>>>
>>> pm_runtime_resume() is documented as being the function to call in the
>>> probe handler. I'd like to get a clarification on this from the runtime
>>> PM developers. If the documentation is wrong it needs to be fixed.
>>
>> Hmm, wonder if it works for things that take a long time to run, or for
>> things that create and probe sub-devices such as the phy?
>
> That's exactly what I'd like to know :-)
>
>> I annotated the clk-mstp.c driver to test for removal of the eth clock and
>> the culprit in this case was a pm thread running before the device finished
>> probing and suspending. I do not have the exact debug output as I deleted it
>> after looking in to it.
>
> Would you be able to perform more tests ? I'd like to get to the bottom of
> this problem and find out whether the best solution would be to fix the
> runtime PM core, the runtime PM documentation or the driver (or a combination
> of them).

 From pm_runtime.h:

static inline int pm_runtime_get_sync(struct device *dev)
{
	return __pm_runtime_resume(dev, RPM_GET_PUT);
}

static inline int pm_request_resume(struct device *dev)
{
	return __pm_runtime_resume(dev, RPM_ASYNC);
}

So it looks like pm_runtime_resume() does not protect against
the possibility that something else may re-suspend the device.

I have yet to ascertain how this ends up happening with device probe,
it seems to be very dependant on the code.


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

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

[Geert Uytterhoeven]

On Mon, Mar 17, 2014 at 2:36 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> I have yet to ascertain how this ends up happening with device probe,
> it seems to be very dependant on the code.

Adding a WARN() in cpg_mstp_clock_endisable():

[<c0278c9c>] (cpg_mstp_clock_endisable) from [<c0278e10>]
(cpg_mstp_clock_disable+0x14/0
x18)
 r10:eec7a000 r9:00000003 r8:eed41508 r7:00000001 r6:a0000113 r5:eec23a00
 r4:eec23a00
[<c0278dfc>] (cpg_mstp_clock_disable) from [<c0275bf4>]
(__clk_disable+0x68/0x78)
[<c0275b8c>] (__clk_disable) from [<c0275cf4>] (clk_disable+0x20/0x2c)
 r4:60000193 r3:00000002
[<c0275cd4>] (clk_disable) from [<c01fd000>] (pm_clk_suspend+0x54/0x78)
 r5:eed41500 r4:eed414c0
[<c01fcfac>] (pm_clk_suspend) from [<c01f87f8>] (__rpm_callback+0x4c/0x7c)
 r8:0000000c r7:00000004 r6:00000000 r5:eed42810 r4:eed42810 r3:c01fcfac
[<c01f87ac>] (__rpm_callback) from [<c01f8878>] (rpm_callback+0x50/0x84)
 r5:eed42810 r4:eec7a000
[<c01f8828>] (rpm_callback) from [<c01f8e54>] (rpm_suspend+0x214/0x3e4)
 r6:00000000 r5:00000000 r4:eed42810 r3:eed42810
[<c01f8c40>] (rpm_suspend) from [<c01f9224>] (rpm_idle+0x184/0x19c)
 r10:00000010 r9:00000000 r8:ee4710bc r7:00000004 r6:00000004 r5:00000000
 r4:eed42810
[<c01f90a0>] (rpm_idle) from [<c01f9328>] (__pm_runtime_idle+0x64/0x7c)
 r6:60000113 r5:eed42870 r4:eed42810 r3:eed42870
[<c01f92c4>] (__pm_runtime_idle) from [<c023ee90>]
(sh_eth_get_stats+0x228/0x23c)
 r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000
[<c023ec68>] (sh_eth_get_stats) from [<c028c194>] (dev_get_stats+0x5c/0x84)
 r4:eec7bc30 r3:c023ec68
[<c028c138>] (dev_get_stats) from [<c029cb08>] (rtnl_fill_ifinfo+0x410/0x8dc)
 r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684
[<c029c6f8>] (rtnl_fill_ifinfo) from [<c029d9f4>] (rtmsg_ifinfo+0x6c/0xd4)
 r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0
 r4:00000000
[<c029d988>] (rtmsg_ifinfo) from [<c0291cd0>] (register_netdevice+0x3dc/0x428)
 r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000
[<c02918f4>] (register_netdevice) from [<c0291d38>] (register_netdev+0x1c/0x2c)
 r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000
[<c0291d1c>] (register_netdev) from [<c0240528>] (sh_eth_drv_probe+0x994/0xb7c)
 r4:ee428000 r3:00000040
[<c023fb94>] (sh_eth_drv_probe) from [<c01f3e6c>] (platform_drv_probe+0x20/0x50)
 r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830
 r4:eed42810
[<c01f3e4c>] (platform_drv_probe) from [<c01f2428>]
(driver_probe_device+0xc8/0x1f8)
 r5:eed42844 r4:eed42810
[<c01f2360>] (driver_probe_device) from [<c01f25c0>] (__driver_attach+0x68/0x8c)
 r7:00000000 r6:c047d830 r5:eed42844 r4:eed42810
[<c01f2558>] (__driver_attach) from [<c01f0cd4>] (bus_for_each_dev+0x5c/0x94)
 r6:c01f2558 r5:eec7be48 r4:c047d830 r3:c01f2558
[<c01f0c78>] (bus_for_each_dev) from [<c01f1f84>] (driver_attach+0x20/0x28)
 r7:00000000 r6:c047a148 r5:ee461480 r4:c047d830
[<c01f1f64>] (driver_attach) from [<c01f1b98>] (bus_add_driver+0xb4/0x1c8)
[<c01f1ae4>] (bus_add_driver) from [<c01f2c98>] (driver_register+0xa4/0xe8)
 r7:c0484780 r6:00000000 r5:c0454c18 r4:c047d830
[<c01f2bf4>] (driver_register) from [<c01f3854>]
(__platform_driver_register+0x50/0x64)
 r5:c0454c18 r4:c0442acc
[<c01f3804>] (__platform_driver_register) from [<c0442ae4>]
(sh_eth_driver_init+0x18/0x20)
[<c0442acc>] (sh_eth_driver_init) from [<c0009668>] (do_one_initcall+0x98/0x13c)
[<c00095d0>] (do_one_initcall) from [<c042bc60>]
(kernel_init_freeable+0x100/0x1cc)
 r9:c045c544 r8:00000065 r7:c0484780 r6:c0454bf8 r5:c0454c18 r4:00000006
[<c042bb60>] (kernel_init_freeable) from [<c032afa8>] (kernel_init+0x10/0xec)
 r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c032af98
 r4:00000000
[<c032af98>] (kernel_init) from [<c000ed98>] (ret_from_fork+0x14/0x3c)
 r4:00000000 r3:00000000


-- 
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] sh_eth: ensure pm_runtime cannot suspend the device during init

[Laurent Pinchart]

Hi Ben,

On Monday 17 March 2014 13:36:33 Ben Dooks wrote:

[snip]

>  From pm_runtime.h:
> 
> static inline int pm_runtime_get_sync(struct device *dev)
> {
> 	return __pm_runtime_resume(dev, RPM_GET_PUT);
> }
> 
> static inline int pm_request_resume(struct device *dev)
> {
> 	return __pm_runtime_resume(dev, RPM_ASYNC);
> }
> 
> So it looks like pm_runtime_resume() does not protect against the
> possibility that something else may re-suspend the device.

Correct.

> I have yet to ascertain how this ends up happening with device probe, it
> seems to be very dependant on the code.

We might be doing something wrong in the driver from a runtime PM point of 
view that leads to the device being suspended. I'd like to catch that instead 
of hiding it by a pm_runtime_get_sync() call. If it turns out that we're not 
doing anything wrong then replacing pm_runtime_resume() with 
pm_runtime_get_sync() would of course be fine.

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

On Mon, Mar 17, 2014 at 2:38 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> [<c01f92c4>] (__pm_runtime_idle) from [<c023ee90>]
> (sh_eth_get_stats+0x228/0x23c)
>  r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000
> [<c023ec68>] (sh_eth_get_stats) from [<c028c194>] (dev_get_stats+0x5c/0x84)
>  r4:eec7bc30 r3:c023ec68
> [<c028c138>] (dev_get_stats) from [<c029cb08>] (rtnl_fill_ifinfo+0x410/0x8dc)
>  r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684
> [<c029c6f8>] (rtnl_fill_ifinfo) from [<c029d9f4>] (rtmsg_ifinfo+0x6c/0xd4)
>  r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0
>  r4:00000000
> [<c029d988>] (rtmsg_ifinfo) from [<c0291cd0>] (register_netdevice+0x3dc/0x428)
>  r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000
> [<c02918f4>] (register_netdevice) from [<c0291d38>] (register_netdev+0x1c/0x2c)
>  r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000
> [<c0291d1c>] (register_netdev) from [<c0240528>] (sh_eth_drv_probe+0x994/0xb7c)
>  r4:ee428000 r3:00000040
> [<c023fb94>] (sh_eth_drv_probe) from [<c01f3e6c>] (platform_drv_probe+0x20/0x50)
>  r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830
>  r4:eed42810

sh_eth_get_stats() calls

    pm_runtime_get_sync(&mdp->pdev->dev);
    ...
    pm_runtime_put_sync(&mdp->pdev->dev);

so adding the extra pm_runtime_get_sync() in ->probe() prevents the
pm_runtime_put_sync() from putting the hardware to sleep.

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] sh_eth: ensure pm_runtime cannot suspend the device during init

[Ben Dooks]

On 17/03/14 13:38, Geert Uytterhoeven wrote:
> On Mon, Mar 17, 2014 at 2:36 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> I have yet to ascertain how this ends up happening with device probe,
>> it seems to be very dependant on the code.
>
> Adding a WARN() in cpg_mstp_clock_endisable():
>
> [<c0278c9c>] (cpg_mstp_clock_endisable) from [<c0278e10>]
> (cpg_mstp_clock_disable+0x14/0
> x18)
>   r10:eec7a000 r9:00000003 r8:eed41508 r7:00000001 r6:a0000113 r5:eec23a00
>   r4:eec23a00
> [<c0278dfc>] (cpg_mstp_clock_disable) from [<c0275bf4>]
> (__clk_disable+0x68/0x78)
> [<c0275b8c>] (__clk_disable) from [<c0275cf4>] (clk_disable+0x20/0x2c)
>   r4:60000193 r3:00000002
> [<c0275cd4>] (clk_disable) from [<c01fd000>] (pm_clk_suspend+0x54/0x78)
>   r5:eed41500 r4:eed414c0
> [<c01fcfac>] (pm_clk_suspend) from [<c01f87f8>] (__rpm_callback+0x4c/0x7c)
>   r8:0000000c r7:00000004 r6:00000000 r5:eed42810 r4:eed42810 r3:c01fcfac
> [<c01f87ac>] (__rpm_callback) from [<c01f8878>] (rpm_callback+0x50/0x84)
>   r5:eed42810 r4:eec7a000
> [<c01f8828>] (rpm_callback) from [<c01f8e54>] (rpm_suspend+0x214/0x3e4)
>   r6:00000000 r5:00000000 r4:eed42810 r3:eed42810
> [<c01f8c40>] (rpm_suspend) from [<c01f9224>] (rpm_idle+0x184/0x19c)
>   r10:00000010 r9:00000000 r8:ee4710bc r7:00000004 r6:00000004 r5:00000000
>   r4:eed42810
> [<c01f90a0>] (rpm_idle) from [<c01f9328>] (__pm_runtime_idle+0x64/0x7c)
>   r6:60000113 r5:eed42870 r4:eed42810 r3:eed42870
> [<c01f92c4>] (__pm_runtime_idle) from [<c023ee90>]
> (sh_eth_get_stats+0x228/0x23c)
>   r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000
> [<c023ec68>] (sh_eth_get_stats) from [<c028c194>] (dev_get_stats+0x5c/0x84)
>   r4:eec7bc30 r3:c023ec68
> [<c028c138>] (dev_get_stats) from [<c029cb08>] (rtnl_fill_ifinfo+0x410/0x8dc)
>   r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684
> [<c029c6f8>] (rtnl_fill_ifinfo) from [<c029d9f4>] (rtmsg_ifinfo+0x6c/0xd4)
>   r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0
>   r4:00000000
> [<c029d988>] (rtmsg_ifinfo) from [<c0291cd0>] (register_netdevice+0x3dc/0x428)
>   r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000
> [<c02918f4>] (register_netdevice) from [<c0291d38>] (register_netdev+0x1c/0x2c)
>   r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000
> [<c0291d1c>] (register_netdev) from [<c0240528>] (sh_eth_drv_probe+0x994/0xb7c)
>   r4:ee428000 r3:00000040
> [<c023fb94>] (sh_eth_drv_probe) from [<c01f3e6c>] (platform_drv_probe+0x20/0x50)
>   r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830
>   r4:eed42810
> [<c01f3e4c>] (platform_drv_probe) from [<c01f2428>]
> (driver_probe_device+0xc8/0x1f8)
>   r5:eed42844 r4:eed42810
> [<c01f2360>] (driver_probe_device) from [<c01f25c0>] (__driver_attach+0x68/0x8c)
>   r7:00000000 r6:c047d830 r5:eed42844 r4:eed42810
> [<c01f2558>] (__driver_attach) from [<c01f0cd4>] (bus_for_each_dev+0x5c/0x94)
>   r6:c01f2558 r5:eec7be48 r4:c047d830 r3:c01f2558
> [<c01f0c78>] (bus_for_each_dev) from [<c01f1f84>] (driver_attach+0x20/0x28)
>   r7:00000000 r6:c047a148 r5:ee461480 r4:c047d830
> [<c01f1f64>] (driver_attach) from [<c01f1b98>] (bus_add_driver+0xb4/0x1c8)
> [<c01f1ae4>] (bus_add_driver) from [<c01f2c98>] (driver_register+0xa4/0xe8)
>   r7:c0484780 r6:00000000 r5:c0454c18 r4:c047d830
> [<c01f2bf4>] (driver_register) from [<c01f3854>]
> (__platform_driver_register+0x50/0x64)
>   r5:c0454c18 r4:c0442acc
> [<c01f3804>] (__platform_driver_register) from [<c0442ae4>]
> (sh_eth_driver_init+0x18/0x20)
> [<c0442acc>] (sh_eth_driver_init) from [<c0009668>] (do_one_initcall+0x98/0x13c)
> [<c00095d0>] (do_one_initcall) from [<c042bc60>]
> (kernel_init_freeable+0x100/0x1cc)
>   r9:c045c544 r8:00000065 r7:c0484780 r6:c0454bf8 r5:c0454c18 r4:00000006
> [<c042bb60>] (kernel_init_freeable) from [<c032afa8>] (kernel_init+0x10/0xec)
>   r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c032af98
>   r4:00000000
> [<c032af98>] (kernel_init) from [<c000ed98>] (ret_from_fork+0x14/0x3c)
>   r4:00000000 r3:00000000

this explains it, the call to stats causes a get_sync/put_sync which
puts the device into a state where it /could/ be suspended and thus
does get suspended in this case from the pm code.

I'm not /sure/ why the pm_runtime code is not protecting against
running this when a device probe is in progress, but it seems the
best thing is to ensure that we always do a get/put sync in the
driver to ensure we have a reference during probe.

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

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

[Laurent Pinchart]

Hi Ben,

On Monday 17 March 2014 13:44:02 Ben Dooks wrote:
> On 17/03/14 13:38, Geert Uytterhoeven wrote:
> > On Mon, Mar 17, 2014 at 2:36 PM, Ben Dooks wrote:
> >> I have yet to ascertain how this ends up happening with device probe,
> >> it seems to be very dependant on the code.
> > 
> > Adding a WARN() in cpg_mstp_clock_endisable():
> > 
> > [<c0278c9c>] (cpg_mstp_clock_endisable) from [<c0278e10>]
> > (cpg_mstp_clock_disable+0x14/0x18)
> >   r10:eec7a000 r9:00000003 r8:eed41508 r7:00000001 r6:a0000113 r5:eec23a00
> >   r4:eec23a00
> > [<c0278dfc>] (cpg_mstp_clock_disable) from [<c0275bf4>]
> > (__clk_disable+0x68/0x78)
> > [<c0275b8c>] (__clk_disable) from [<c0275cf4>] (clk_disable+0x20/0x2c)
> >   r4:60000193 r3:00000002
> > [<c0275cd4>] (clk_disable) from [<c01fd000>] (pm_clk_suspend+0x54/0x78)
> >   r5:eed41500 r4:eed414c0
> > [<c01fcfac>] (pm_clk_suspend) from [<c01f87f8>] (__rpm_callback+0x4c/0x7c)
> >   r8:0000000c r7:00000004 r6:00000000 r5:eed42810 r4:eed42810 r3:c01fcfac
> > [<c01f87ac>] (__rpm_callback) from [<c01f8878>] (rpm_callback+0x50/0x84)
> >   r5:eed42810 r4:eec7a000
> > [<c01f8828>] (rpm_callback) from [<c01f8e54>] (rpm_suspend+0x214/0x3e4)
> >   r6:00000000 r5:00000000 r4:eed42810 r3:eed42810
> > [<c01f8c40>] (rpm_suspend) from [<c01f9224>] (rpm_idle+0x184/0x19c)
> >   r10:00000010 r9:00000000 r8:ee4710bc r7:00000004 r6:00000004 r5:00000000
> >   r4:eed42810
> > [<c01f90a0>] (rpm_idle) from [<c01f9328>] (__pm_runtime_idle+0x64/0x7c)
> >   r6:60000113 r5:eed42870 r4:eed42810 r3:eed42870
> > [<c01f92c4>] (__pm_runtime_idle) from [<c023ee90>]
> > (sh_eth_get_stats+0x228/0x23c)
> >   r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000
> > [<c023ec68>] (sh_eth_get_stats) from [<c028c194>]
> > (dev_get_stats+0x5c/0x84)
> >   r4:eec7bc30 r3:c023ec68
> > [<c028c138>] (dev_get_stats) from [<c029cb08>]
> > (rtnl_fill_ifinfo+0x410/0x8dc)
> >   r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684
> > [<c029c6f8>] (rtnl_fill_ifinfo) from [<c029d9f4>] (rtmsg_ifinfo+0x6c/0xd4)
> >   r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0
> >   r4:00000000
> > [<c029d988>] (rtmsg_ifinfo) from [<c0291cd0>]
> > (register_netdevice+0x3dc/0x428)> 
> >   r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000
> > [<c02918f4>] (register_netdevice) from [<c0291d38>]
> > (register_netdev+0x1c/0x2c)> 
> >   r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000
> > [<c0291d1c>] (register_netdev) from [<c0240528>]
> > (sh_eth_drv_probe+0x994/0xb7c)
> >   r4:ee428000 r3:00000040
> > [<c023fb94>] (sh_eth_drv_probe) from [<c01f3e6c>]
> > (platform_drv_probe+0x20/0x50)
> >   r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830
> >   r4:eed42810

[snip]

> this explains it, the call to stats causes a get_sync/put_sync which
> puts the device into a state where it /could/ be suspended and thus
> does get suspended in this case from the pm code.
> 
> I'm not /sure/ why the pm_runtime code is not protecting against
> running this when a device probe is in progress, but it seems the
> best thing is to ensure that we always do a get/put sync in the
> driver to ensure we have a reference during probe.

Wouldn't it be better to register the MDIO bus before registering the network 
device ? It looks like the current order is prone to race conditions.

Another potential issue, does the network layer guarantee that the device will 
always be opened by an .ndo_open() call before performing any MDIO operation ? 
sh_mmd_ctrl, sh_set_mdio and sh_get_mdio access sh-eth registers, could we be 
missing runtime PM calls in those call paths ?

-- 
Regards,

Laurent Pinchart

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

[Ben Dooks]

On 17/03/14 13:54, Laurent Pinchart wrote:
> Hi Ben,
>
> On Monday 17 March 2014 13:44:02 Ben Dooks wrote:
>> On 17/03/14 13:38, Geert Uytterhoeven wrote:
>>> On Mon, Mar 17, 2014 at 2:36 PM, Ben Dooks wrote:
>>>> I have yet to ascertain how this ends up happening with device probe,
>>>> it seems to be very dependant on the code.
>>>
>>> Adding a WARN() in cpg_mstp_clock_endisable():
>>>
>>> [<c0278c9c>] (cpg_mstp_clock_endisable) from [<c0278e10>]
>>> (cpg_mstp_clock_disable+0x14/0x18)
>>>    r10:eec7a000 r9:00000003 r8:eed41508 r7:00000001 r6:a0000113 r5:eec23a00
>>>    r4:eec23a00
>>> [<c0278dfc>] (cpg_mstp_clock_disable) from [<c0275bf4>]
>>> (__clk_disable+0x68/0x78)
>>> [<c0275b8c>] (__clk_disable) from [<c0275cf4>] (clk_disable+0x20/0x2c)
>>>    r4:60000193 r3:00000002
>>> [<c0275cd4>] (clk_disable) from [<c01fd000>] (pm_clk_suspend+0x54/0x78)
>>>    r5:eed41500 r4:eed414c0
>>> [<c01fcfac>] (pm_clk_suspend) from [<c01f87f8>] (__rpm_callback+0x4c/0x7c)
>>>    r8:0000000c r7:00000004 r6:00000000 r5:eed42810 r4:eed42810 r3:c01fcfac
>>> [<c01f87ac>] (__rpm_callback) from [<c01f8878>] (rpm_callback+0x50/0x84)
>>>    r5:eed42810 r4:eec7a000
>>> [<c01f8828>] (rpm_callback) from [<c01f8e54>] (rpm_suspend+0x214/0x3e4)
>>>    r6:00000000 r5:00000000 r4:eed42810 r3:eed42810
>>> [<c01f8c40>] (rpm_suspend) from [<c01f9224>] (rpm_idle+0x184/0x19c)
>>>    r10:00000010 r9:00000000 r8:ee4710bc r7:00000004 r6:00000004 r5:00000000
>>>    r4:eed42810
>>> [<c01f90a0>] (rpm_idle) from [<c01f9328>] (__pm_runtime_idle+0x64/0x7c)
>>>    r6:60000113 r5:eed42870 r4:eed42810 r3:eed42870
>>> [<c01f92c4>] (__pm_runtime_idle) from [<c023ee90>]
>>> (sh_eth_get_stats+0x228/0x23c)
>>>    r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000
>>> [<c023ec68>] (sh_eth_get_stats) from [<c028c194>]
>>> (dev_get_stats+0x5c/0x84)
>>>    r4:eec7bc30 r3:c023ec68
>>> [<c028c138>] (dev_get_stats) from [<c029cb08>]
>>> (rtnl_fill_ifinfo+0x410/0x8dc)
>>>    r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684
>>> [<c029c6f8>] (rtnl_fill_ifinfo) from [<c029d9f4>] (rtmsg_ifinfo+0x6c/0xd4)
>>>    r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0
>>>    r4:00000000
>>> [<c029d988>] (rtmsg_ifinfo) from [<c0291cd0>]
>>> (register_netdevice+0x3dc/0x428)>
>>>    r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000
>>> [<c02918f4>] (register_netdevice) from [<c0291d38>]
>>> (register_netdev+0x1c/0x2c)>
>>>    r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000
>>> [<c0291d1c>] (register_netdev) from [<c0240528>]
>>> (sh_eth_drv_probe+0x994/0xb7c)
>>>    r4:ee428000 r3:00000040
>>> [<c023fb94>] (sh_eth_drv_probe) from [<c01f3e6c>]
>>> (platform_drv_probe+0x20/0x50)
>>>    r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830
>>>    r4:eed42810
>
> [snip]
>
>> this explains it, the call to stats causes a get_sync/put_sync which
>> puts the device into a state where it /could/ be suspended and thus
>> does get suspended in this case from the pm code.
>>
>> I'm not /sure/ why the pm_runtime code is not protecting against
>> running this when a device probe is in progress, but it seems the
>> best thing is to ensure that we always do a get/put sync in the
>> driver to ensure we have a reference during probe.
>
> Wouldn't it be better to register the MDIO bus before registering the network
> device ? It looks like the current order is prone to race conditions.

Not sure, requires input?

> Another potential issue, does the network layer guarantee that the device will
> always be opened by an .ndo_open() call before performing any MDIO operation ?
> sh_mmd_ctrl, sh_set_mdio and sh_get_mdio access sh-eth registers, could we be
> missing runtime PM calls in those call paths ?

I'm not sure if there is any guarantee, I don't think the PHY driver
does any sort of open on probe. I do have a patch to ensure that the
MDIO operations are wrapped with pm_runtime_{get,put}_sync() calls as
the probe of the PHY often fails without it.

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

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

[Geert Uytterhoeven]

(adding Sergei)

On Mon, Mar 17, 2014 at 2:54 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 17 March 2014 13:44:02 Ben Dooks wrote:
>> On 17/03/14 13:38, Geert Uytterhoeven wrote:
>> > On Mon, Mar 17, 2014 at 2:36 PM, Ben Dooks wrote:
>> >> I have yet to ascertain how this ends up happening with device probe,
>> >> it seems to be very dependant on the code.
>> >
>> > Adding a WARN() in cpg_mstp_clock_endisable():
>> >
>> > [<c0278c9c>] (cpg_mstp_clock_endisable) from [<c0278e10>]
>> > (cpg_mstp_clock_disable+0x14/0x18)
>> >   r10:eec7a000 r9:00000003 r8:eed41508 r7:00000001 r6:a0000113 r5:eec23a00
>> >   r4:eec23a00
>> > [<c0278dfc>] (cpg_mstp_clock_disable) from [<c0275bf4>]
>> > (__clk_disable+0x68/0x78)
>> > [<c0275b8c>] (__clk_disable) from [<c0275cf4>] (clk_disable+0x20/0x2c)
>> >   r4:60000193 r3:00000002
>> > [<c0275cd4>] (clk_disable) from [<c01fd000>] (pm_clk_suspend+0x54/0x78)
>> >   r5:eed41500 r4:eed414c0
>> > [<c01fcfac>] (pm_clk_suspend) from [<c01f87f8>] (__rpm_callback+0x4c/0x7c)
>> >   r8:0000000c r7:00000004 r6:00000000 r5:eed42810 r4:eed42810 r3:c01fcfac
>> > [<c01f87ac>] (__rpm_callback) from [<c01f8878>] (rpm_callback+0x50/0x84)
>> >   r5:eed42810 r4:eec7a000
>> > [<c01f8828>] (rpm_callback) from [<c01f8e54>] (rpm_suspend+0x214/0x3e4)
>> >   r6:00000000 r5:00000000 r4:eed42810 r3:eed42810
>> > [<c01f8c40>] (rpm_suspend) from [<c01f9224>] (rpm_idle+0x184/0x19c)
>> >   r10:00000010 r9:00000000 r8:ee4710bc r7:00000004 r6:00000004 r5:00000000
>> >   r4:eed42810
>> > [<c01f90a0>] (rpm_idle) from [<c01f9328>] (__pm_runtime_idle+0x64/0x7c)
>> >   r6:60000113 r5:eed42870 r4:eed42810 r3:eed42870
>> > [<c01f92c4>] (__pm_runtime_idle) from [<c023ee90>]
>> > (sh_eth_get_stats+0x228/0x23c)
>> >   r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000
>> > [<c023ec68>] (sh_eth_get_stats) from [<c028c194>]
>> > (dev_get_stats+0x5c/0x84)
>> >   r4:eec7bc30 r3:c023ec68
>> > [<c028c138>] (dev_get_stats) from [<c029cb08>]
>> > (rtnl_fill_ifinfo+0x410/0x8dc)
>> >   r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684
>> > [<c029c6f8>] (rtnl_fill_ifinfo) from [<c029d9f4>] (rtmsg_ifinfo+0x6c/0xd4)
>> >   r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0
>> >   r4:00000000
>> > [<c029d988>] (rtmsg_ifinfo) from [<c0291cd0>]
>> > (register_netdevice+0x3dc/0x428)>
>> >   r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000
>> > [<c02918f4>] (register_netdevice) from [<c0291d38>]
>> > (register_netdev+0x1c/0x2c)>
>> >   r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000
>> > [<c0291d1c>] (register_netdev) from [<c0240528>]
>> > (sh_eth_drv_probe+0x994/0xb7c)
>> >   r4:ee428000 r3:00000040
>> > [<c023fb94>] (sh_eth_drv_probe) from [<c01f3e6c>]
>> > (platform_drv_probe+0x20/0x50)
>> >   r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830
>> >   r4:eed42810
>
> [snip]
>
>> this explains it, the call to stats causes a get_sync/put_sync which
>> puts the device into a state where it /could/ be suspended and thus
>> does get suspended in this case from the pm code.
>>
>> I'm not /sure/ why the pm_runtime code is not protecting against
>> running this when a device probe is in progress, but it seems the
>> best thing is to ensure that we always do a get/put sync in the
>> driver to ensure we have a reference during probe.
>
> Wouldn't it be better to register the MDIO bus before registering the network
> device ? It looks like the current order is prone to race conditions.

Indeed, typically register_netdev() is the last call of the .probe() function.

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] sh_eth: ensure pm_runtime cannot suspend the device during init

[Sergei Shtylyov]

Hello.

On 17-03-2014 18:02, Geert Uytterhoeven wrote:

> (adding Sergei)

[...]
>>>>> I have yet to ascertain how this ends up happening with device probe,
>>>>> it seems to be very dependant on the code.

>>>> Adding a WARN() in cpg_mstp_clock_endisable():

>>>> [<c0278c9c>] (cpg_mstp_clock_endisable) from [<c0278e10>]
>>>> (cpg_mstp_clock_disable+0x14/0x18)
>>>>    r10:eec7a000 r9:00000003 r8:eed41508 r7:00000001 r6:a0000113 r5:eec23a00
>>>>    r4:eec23a00
>>>> [<c0278dfc>] (cpg_mstp_clock_disable) from [<c0275bf4>]
>>>> (__clk_disable+0x68/0x78)
>>>> [<c0275b8c>] (__clk_disable) from [<c0275cf4>] (clk_disable+0x20/0x2c)
>>>>    r4:60000193 r3:00000002
>>>> [<c0275cd4>] (clk_disable) from [<c01fd000>] (pm_clk_suspend+0x54/0x78)
>>>>    r5:eed41500 r4:eed414c0
>>>> [<c01fcfac>] (pm_clk_suspend) from [<c01f87f8>] (__rpm_callback+0x4c/0x7c)
>>>>    r8:0000000c r7:00000004 r6:00000000 r5:eed42810 r4:eed42810 r3:c01fcfac
>>>> [<c01f87ac>] (__rpm_callback) from [<c01f8878>] (rpm_callback+0x50/0x84)
>>>>    r5:eed42810 r4:eec7a000
>>>> [<c01f8828>] (rpm_callback) from [<c01f8e54>] (rpm_suspend+0x214/0x3e4)
>>>>    r6:00000000 r5:00000000 r4:eed42810 r3:eed42810
>>>> [<c01f8c40>] (rpm_suspend) from [<c01f9224>] (rpm_idle+0x184/0x19c)
>>>>    r10:00000010 r9:00000000 r8:ee4710bc r7:00000004 r6:00000004 r5:00000000
>>>>    r4:eed42810
>>>> [<c01f90a0>] (rpm_idle) from [<c01f9328>] (__pm_runtime_idle+0x64/0x7c)
>>>>    r6:60000113 r5:eed42870 r4:eed42810 r3:eed42870
>>>> [<c01f92c4>] (__pm_runtime_idle) from [<c023ee90>]
>>>> (sh_eth_get_stats+0x228/0x23c)
>>>>    r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000
>>>> [<c023ec68>] (sh_eth_get_stats) from [<c028c194>]
>>>> (dev_get_stats+0x5c/0x84)
>>>>    r4:eec7bc30 r3:c023ec68
>>>> [<c028c138>] (dev_get_stats) from [<c029cb08>]
>>>> (rtnl_fill_ifinfo+0x410/0x8dc)
>>>>    r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684
>>>> [<c029c6f8>] (rtnl_fill_ifinfo) from [<c029d9f4>] (rtmsg_ifinfo+0x6c/0xd4)
>>>>    r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0
>>>>    r4:00000000
>>>> [<c029d988>] (rtmsg_ifinfo) from [<c0291cd0>]
>>>> (register_netdevice+0x3dc/0x428)>
>>>>    r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000
>>>> [<c02918f4>] (register_netdevice) from [<c0291d38>]
>>>> (register_netdev+0x1c/0x2c)>
>>>>    r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000
>>>> [<c0291d1c>] (register_netdev) from [<c0240528>]
>>>> (sh_eth_drv_probe+0x994/0xb7c)
>>>>    r4:ee428000 r3:00000040
>>>> [<c023fb94>] (sh_eth_drv_probe) from [<c01f3e6c>]
>>>> (platform_drv_probe+0x20/0x50)
>>>>    r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830
>>>>    r4:eed42810

>> [snip]

>>> this explains it, the call to stats causes a get_sync/put_sync which
>>> puts the device into a state where it /could/ be suspended and thus
>>> does get suspended in this case from the pm code.

>>> I'm not /sure/ why the pm_runtime code is not protecting against
>>> running this when a device probe is in progress, but it seems the
>>> best thing is to ensure that we always do a get/put sync in the
>>> driver to ensure we have a reference during probe.

>> Wouldn't it be better to register the MDIO bus before registering the network
>> device ? It looks like the current order is prone to race conditions.

> Indeed, typically register_netdev() is the last call of the .probe() function.

    I'll see what can be done. I've been looking at the probe() method recently...

> Gr{oetje,eeting}s,

>                          Geert

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 03/17/2014 05:01 PM, Ben Dooks wrote:

>>>>> I have yet to ascertain how this ends up happening with device probe,
>>>>> it seems to be very dependant on the code.

>>>> Adding a WARN() in cpg_mstp_clock_endisable():

[...]

>>> this explains it, the call to stats causes a get_sync/put_sync which
>>> puts the device into a state where it /could/ be suspended and thus
>>> does get suspended in this case from the pm code.

>>> I'm not /sure/ why the pm_runtime code is not protecting against
>>> running this when a device probe is in progress, but it seems the
>>> best thing is to ensure that we always do a get/put sync in the
>>> driver to ensure we have a reference during probe.

>> Wouldn't it be better to register the MDIO bus before registering the network
>> device ? It looks like the current order is prone to race conditions.

> Not sure, requires input?

    People say that the driver should be ready to receive the ndo_open() 
method call even before register_netdev() returns. I have prepared the patch 
to probe MDIO before calling register_netdev() now, need to sanity test it.

>> Another potential issue, does the network layer guarantee that the device will
>> always be opened by an .ndo_open() call before performing any MDIO operation ?
>> sh_mmd_ctrl, sh_set_mdio and sh_get_mdio access sh-eth registers, could we be
>> missing runtime PM calls in those call paths ?

> I'm not sure if there is any guarantee, I don't think the PHY driver
> does any sort of open on probe. I do have a patch to ensure that the
> MDIO operations are wrapped with pm_runtime_{get,put}_sync() calls as
> the probe of the PHY often fails without it.

    Respin this patch please (addressing my comments), so that DaveM could 
take it.

WBR, Sergei

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

[Laurent Pinchart]

Hi Ben and Sergei,

On Wednesday 19 March 2014 00:17:43 Sergei Shtylyov wrote:
> On 03/17/2014 05:01 PM, Ben Dooks wrote:
> >>>>> I have yet to ascertain how this ends up happening with device probe,
> >>>>> it seems to be very dependant on the code.
> >>>> 
> >>>> Adding a WARN() in cpg_mstp_clock_endisable():
> [...]
> 
> >>> this explains it, the call to stats causes a get_sync/put_sync which
> >>> puts the device into a state where it /could/ be suspended and thus
> >>> does get suspended in this case from the pm code.
> >>> 
> >>> I'm not /sure/ why the pm_runtime code is not protecting against
> >>> running this when a device probe is in progress, but it seems the
> >>> best thing is to ensure that we always do a get/put sync in the
> >>> driver to ensure we have a reference during probe.
> >> 
> >> Wouldn't it be better to register the MDIO bus before registering the
> >> network device ? It looks like the current order is prone to race
> >> conditions.
> >
> > Not sure, requires input?
> 
> People say that the driver should be ready to receive the ndo_open() method
> call even before register_netdev() returns. I have prepared the patch to
> probe MDIO before calling register_netdev() now, need to sanity test it.

Thank you.

> >> Another potential issue, does the network layer guarantee that the device
> >> will always be opened by an .ndo_open() call before performing any MDIO
> >> operation ? sh_mmd_ctrl, sh_set_mdio and sh_get_mdio access sh-eth
> >> registers, could we be missing runtime PM calls in those call paths ?
> > 
> > I'm not sure if there is any guarantee, I don't think the PHY driver
> > does any sort of open on probe. I do have a patch to ensure that the
> > MDIO operations are wrapped with pm_runtime_{get,put}_sync() calls as
> > the probe of the PHY often fails without it.
> 
> Respin this patch please (addressing my comments), so that DaveM could take
> it.

Before wrapping MDIO operations in runtime PM calls, could we check whether 
that's really needed ? Moving PHY registration before network device 
registration will fix PHY probe problems, we might not need any other change.

-- 
Regards,

Laurent Pinchart

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

[Sergei Shtylyov]

[-- Attachment #1: Type: text/plain, Size: 11808 bytes --]

Hello.

On 03/18/2014 11:45 PM, Laurent Pinchart wrote:

>>>>>>> I have yet to ascertain how this ends up happening with device probe,
>>>>>>> it seems to be very dependant on the code.

>>>>>> Adding a WARN() in cpg_mstp_clock_endisable():
>> [...]

>>>>> this explains it, the call to stats causes a get_sync/put_sync which
>>>>> puts the device into a state where it /could/ be suspended and thus
>>>>> does get suspended in this case from the pm code.

>>>>> I'm not /sure/ why the pm_runtime code is not protecting against
>>>>> running this when a device probe is in progress, but it seems the
>>>>> best thing is to ensure that we always do a get/put sync in the
>>>>> driver to ensure we have a reference during probe.

>>>> Wouldn't it be better to register the MDIO bus before registering the
>>>> network device ? It looks like the current order is prone to race
>>>> conditions.

>>> Not sure, requires input?

>> People say that the driver should be ready to receive the ndo_open() method
>> call even before register_netdev() returns. I have prepared the patch to
>> probe MDIO before calling register_netdev() now, need to sanity test it.

> Thank you.

    Not at all, actually. I've probvably missed something subtle in the 
mdiobus_register() call chain, and so hilarity ensued when I booted:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at include/linux/kref.h:47 kobject_get+0x50/0x68()
CPU: 0 PID: 1 Comm: swapper Tainted: G        W    3.14.0-rc7-dirty #339
Backtrace:
[<c0010804>] (dump_backtrace) from [<c00109a0>] (show_stack+0x18/0x1c)
  r6:c03eedcb r5:00000009 r4:00000000 r3:00204140
[<c0010988>] (show_stack) from [<c0355c14>] (dump_stack+0x20/0x28)
[<c0355bf4>] (dump_stack) from [<c001c19c>] (warn_slowpath_common+0x68/0x88)
[<c001c134>] (warn_slowpath_common) from [<c001c294>] 
(warn_slowpath_null+0x24/0x2c)
  r8:eeca2a10 r7:ee7fd440 r6:ee7fd448 r5:c04791d7 r4:eeda8258
[<c001c270>] (warn_slowpath_null) from [<c015f474>] (kobject_get+0x50/0x68)
[<c015f424>] (kobject_get) from [<c01c6f68>] (get_device+0x1c/0x24)
  r5:00000000 r4:ee7fd440
[<c01c6f4c>] (get_device) from [<c01c706c>] (device_add+0xc4/0x510)
[<c01c6fa8>] (device_add) from [<c01c74d4>] (device_register+0x1c/0x20)
  r10:ee7fd400 r8:eeca2a10 r7:ee7fd440 r6:ef2058e8 r5:ee7fd404 r4:ee7fd440
[<c01c74b8>] (device_register) from [<c021150c>] (mdiobus_register+0x84/0x168)
  r4:ee7fd400 r3:00000000
[<c0211488>] (mdiobus_register) from [<c028cb64>] (of_mdiobus_register+0x3c/0x1ac)
  r8:eeca2a10 r7:eeda8250 r6:ef2058e8 r5:ee7ec690 r4:ee7fd400 r3:00000080
[<c028cb28>] (of_mdiobus_register) from [<c021410c>] 
(sh_eth_drv_probe+0x9f4/0xb58)
  r10:ee7fd400 r9:ffffffff r8:eeca2a10 r7:eeda8250 r6:eeca2a10 r5:ee7ec690
  r4:eeda8000
[<c0213718>] (sh_eth_drv_probe) from [<c01cb6e8>] (platform_drv_probe+0x20/0x50)
  r10:c044779c r9:00000000 r8:c043ccd0 r7:c01ca5d4 r6:c04706bc r5:c04706bc
  r4:eeca2a10
[<c01cb6c8>] (platform_drv_probe) from [<c01ca480>] 
(driver_probe_device+0xbc/0x210)
  r5:00000000 r4:eeca2a10
[<c01ca3c4>] (driver_probe_device) from [<c01ca644>] (__driver_attach+0x70/0x94)
  r6:c04706bc r5:eeca2a44 r4:eeca2a10 r3:00000000
[<c01ca5d4>] (__driver_attach) from [<c01c8bf4>] (bus_for_each_dev+0x5c/0x98)
  r6:c04706bc r5:eec4de40 r4:00000000 r3:c01ca5d4
[<c01c8b98>] (bus_for_each_dev) from [<c01ca1c8>] (driver_attach+0x20/0x28)
  r7:c046d680 r6:00000000 r5:ee7f1500 r4:c04706bc
[<c01ca1a8>] (driver_attach) from [<c01c93c4>] (bus_add_driver+0xc8/0x1c8)
[<c01c92fc>] (bus_add_driver) from [<c01caccc>] (driver_register+0xa4/0xe8)
  r7:c0479280 r6:c044f56c r5:c0447790 r4:c04706bc
[<c01cac28>] (driver_register) from [<c01cbee8>] 
(__platform_driver_register+0x50/0x64)
  r5:c0447790 r4:00000006
[<c01cbe98>] (__platform_driver_register) from [<c043cce8>] 
(sh_eth_driver_init+0x18/0x20)
[<c043ccd0>] (sh_eth_driver_init) from [<c042ab18>] (do_one_initcall+0x9c/0x140)
[<c042aa7c>] (do_one_initcall) from [<c042acac>] (kernel_init_freeable+0xf0/0x1b8)
  r10:c044779c r9:00000000 r8:00000083 r7:c0479280 r6:c044f56c r5:c0447790
  r4:00000006
[<c042abbc>] (kernel_init_freeable) from [<c0353144>] (kernel_init+0x14/0xec)
  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0353130 r4:c0479280
[<c0353130>] (kernel_init) from [<c000de78>] (ret_from_fork+0x14/0x3c)
  r4:00000000 r3:00000000
---[ end trace 3406ff24bd97382f ]---
Unable to handle kernel NULL pointer dereference at virtual address 0000000c
pgd = c0004000
[0000000c] *pgd=00000000
Internal error: Oops: 5 [#1] ARM
CPU: 0 PID: 1 Comm: swapper Tainted: G        W    3.14.0-rc7-dirty #339
task: eec30b80 ti: eec4c000 task.ti: eec4c000
PC is at kobj_child_ns_ops+0x18/0x34
LR is at kobj_ns_ops+0x14/0x18
pc : [<c015f7ec>]    lr : [<c015f81c>]    psr: a0000153
sp : eec4dc40  ip : eec4dc50  fp : eec4dc4c
r10: ee7fd400  r9 : ffffffff  r8 : eeca2a10
r7 : c046fe7c  r6 : eeda8258  r5 : 00000000  r4 : ee7ec5c0
r3 : 00000000  r2 : eed03ac4  r1 : ee7ec5c4  r0 : eeda8258
Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c53c7d  Table: 40004059  DAC: 00000015
Process swapper (pid: 1, stack limit = 0xeec4c238)
Stack: (0xeec4dc40 to 0xeec4e000)
dc40: eec4dc5c eec4dc50 c015f81c c015f7e0 eec4dc74 eec4dc60 c015f834 c015f814
dc60: eed03ac4 ee7ec5c0 eec4dcac eec4dc78 c015f910 c015f82c 00000000 00000000
dc80: eec4dcac eec4dc90 ee7ec5c0 00000000 eeda8258 c046fe7c eeca2a10 ee7fd400
dca0: eec4dcd4 eec4dcb0 c015fc4c c015f868 eec4dcd4 eec4dcdc c001c294 00000000
dcc0: ee7ec5c0 eeda8258 eec4dcfc eec4dce0 c01c6e54 c015fbe8 c03f2cf2 c040d158
dce0: ee7fd440 00000000 ee7fd448 eeda8250 eec4dd34 eec4dd00 c01c707c c01c6d68
dd00: c01d38e4 c003f4ec 00000020 ee7fd440 ee7fd440 ee7fd404 ef2058e8 ee7fd440
dd20: eeca2a10 ee7fd400 eec4dd4c eec4dd38 c01c74d4 c01c6fb4 00000000 ee7fd400
dd40: eec4dd74 eec4dd50 c021150c c01c74c4 00000080 ee7fd400 ee7ec690 ef2058e8
dd60: eeda8250 eeca2a10 eec4ddac eec4dd78 c028cb64 c0211494 c01cca9c c01cc770
dd80: c01cc738 eeda8000 ee7ec690 eeca2a10 eeda8250 eeca2a10 ffffffff ee7fd400
dda0: eec4dde4 eec4ddb0 c021410c c028cb34 ffffffff 00000000 c01ca5d4 eeca2a10
ddc0: c04706bc c04706bc c01ca5d4 c043ccd0 00000000 c044779c eec4ddfc eec4dde8
dde0: c01cb6e8 c0213724 eeca2a10 00000000 eec4de1c eec4de00 c01ca480 c01cb6d4
de00: 00000000 eeca2a10 eeca2a44 c04706bc eec4de3c eec4de20 c01ca644 c01ca3d0
de20: c01ca5d4 00000000 eec4de40 c04706bc eec4de64 eec4de40 c01c8bf4 c01ca5e0
de40: eec2fe4c eec995b0 c04706bc ee7f1500 00000000 c046d680 eec4de74 eec4de68
de60: c01ca1c8 c01c8ba4 eec4de9c eec4de78 c01c93c4 c01ca1b4 c040d672 eec4de88
de80: c04706bc c0447790 c044f56c c0479280 eec4deb4 eec4dea0 c01caccc c01c9308
dea0: 00000006 c0447790 eec4dec4 eec4deb8 c01cbee8 c01cac34 eec4ded4 eec4dec8
dec0: c043cce8 c01cbea4 eec4df54 eec4ded8 c042ab18 c043ccdc eec4df04 eec4dee8
dee0: c00de144 ef201db7 eec4df00 eec4def8 c042a4e8 c0162c3c ef201db7 ef201dba
df00: eec4df54 eec4df10 c003284c c042a4d8 00000000 c0428600 00000006 00000006
df20: 00000083 c0427dd0 eec4df54 00000006 c0447790 c044f56c c0479280 00000083
df40: 00000000 c044779c eec4df94 eec4df58 c042acac c042aa88 00000006 00000006
df60: c042a4cc fd2bef9f 75ff2fd1 05a3f498 c0479280 c0353130 00000000 00000000
df80: 00000000 00000000 eec4dfac eec4df98 c0353144 c042abc8 00000000 00000000
dfa0: 00000000 eec4dfb0 c000de78 c035313c 00000000 00000000 00000000 00000000
dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 f7df77e1 9dbd73f4
Backtrace:
[<c015f7d4>] (kobj_child_ns_ops) from [<c015f81c>] (kobj_ns_ops+0x14/0x18)
[<c015f808>] (kobj_ns_ops) from [<c015f834>] (kobject_namespace+0x14/0x3c)
[<c015f820>] (kobject_namespace) from [<c015f910>] 
(kobject_add_internal+0xb4/0x294) 

  r4:ee7ec5c0 r3:eed03ac4
[<c015f85c>] (kobject_add_internal) from [<c015fc4c>] (kobject_add+0x74/0x94)
  r10:ee7fd400 r8:eeca2a10 r7:c046fe7c r6:eeda8258 r5:00000000 r4:ee7ec5c0
[<c015fbdc>] (kobject_add) from [<c01c6e54>] (get_device_parent+0xf8/0x154)
  r3:c040d158 r2:c03f2cf2
  r6:eeda8258 r5:ee7ec5c0 r4:00000000
[<c01c6d5c>] (get_device_parent) from [<c01c707c>] (device_add+0xd4/0x510)
  r7:eeda8250 r6:ee7fd448 r5:00000000 r4:ee7fd440
[<c01c6fa8>] (device_add) from [<c01c74d4>] (device_register+0x1c/0x20)
  r10:ee7fd400 r8:eeca2a10 r7:ee7fd440 r6:ef2058e8 r5:ee7fd404 r4:ee7fd440
[<c01c74b8>] (device_register) from [<c021150c>] (mdiobus_register+0x84/0x168)
  r4:ee7fd400 r3:00000000
[<c0211488>] (mdiobus_register) from [<c028cb64>] (of_mdiobus_register+0x3c/0x1ac)
  r8:eeca2a10 r7:eeda8250 r6:ef2058e8 r5:ee7ec690 r4:ee7fd400 r3:00000080
[<c028cb28>] (of_mdiobus_register) from [<c021410c>] 
(sh_eth_drv_probe+0x9f4/0xb58)
  r10:ee7fd400 r9:ffffffff r8:eeca2a10 r7:eeda8250 r6:eeca2a10 r5:ee7ec690
  r4:eeda8000
[<c0213718>] (sh_eth_drv_probe) from [<c01cb6e8>] (platform_drv_probe+0x20/0x50)
  r10:c044779c r9:00000000 r8:c043ccd0 r7:c01ca5d4 r6:c04706bc r5:c04706bc
  r4:eeca2a10
[<c01cb6c8>] (platform_drv_probe) from [<c01ca480>] 
(driver_probe_device+0xbc/0x210)
  r5:00000000 r4:eeca2a10
[<c01ca3c4>] (driver_probe_device) from [<c01ca644>] (__driver_attach+0x70/0x94)
  r6:c04706bc r5:eeca2a44 r4:eeca2a10 r3:00000000
[<c01ca5d4>] (__driver_attach) from [<c01c8bf4>] (bus_for_each_dev+0x5c/0x98)
  r6:c04706bc r5:eec4de40 r4:00000000 r3:c01ca5d4
[<c01c8b98>] (bus_for_each_dev) from [<c01ca1c8>] (driver_attach+0x20/0x28)
  r7:c046d680 r6:00000000 r5:ee7f1500 r4:c04706bc
[<c01ca1a8>] (driver_attach) from [<c01c93c4>] (bus_add_driver+0xc8/0x1c8)
[<c01c92fc>] (bus_add_driver) from [<c01caccc>] (driver_register+0xa4/0xe8)
  r7:c0479280 r6:c044f56c r5:c0447790 r4:c04706bc
[<c01cac28>] (driver_register) from [<c01cbee8>] 
(__platform_driver_register+0x50/0x64)
  r5:c0447790 r4:00000006
[<c01cbe98>] (__platform_driver_register) from [<c043cce8>] 
(sh_eth_driver_init+0x18/0x20)
[<c043ccd0>] (sh_eth_driver_init) from [<c042ab18>] (do_one_initcall+0x9c/0x140)
[<c042aa7c>] (do_one_initcall) from [<c042acac>] (kernel_init_freeable+0xf0/0x1b8
  r10:c044779c r9:00000000 r8:00000083 r7:c0479280 r6:c044f56c r5:c0447790
  r4:00000006
[<c042abbc>] (kernel_init_freeable) from [<c0353144>] (kernel_init+0x14/0xec)
  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0353130 r4:c0479280
[<c0353130>] (kernel_init) from [<c000de78>] (ret_from_fork+0x14/0x3c)
  r4:00000000 r3:00000000
Code: e24cb004 e2503000 0a000005 e5933014 (e593300c)
---[ end trace 3406ff24bd973830 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

    This looks rather hopeless to me. I'm attaching the patch just in case, 
it's against renesas-devel-v3.14-rc7-20140318.

>>>> Another potential issue, does the network layer guarantee that the device
>>>> will always be opened by an .ndo_open() call before performing any MDIO
>>>> operation ? sh_mmd_ctrl, sh_set_mdio and sh_get_mdio access sh-eth
>>>> registers, could we be missing runtime PM calls in those call paths ?

>>> I'm not sure if there is any guarantee, I don't think the PHY driver
>>> does any sort of open on probe. I do have a patch to ensure that the
>>> MDIO operations are wrapped with pm_runtime_{get,put}_sync() calls as
>>> the probe of the PHY often fails without it.

>> Respin this patch please (addressing my comments), so that DaveM could take
>> it.

> Before wrapping MDIO operations in runtime PM calls, could we check whether
> that's really needed ? Moving PHY registration before network device
> registration will fix PHY probe problems, we might not need any other change.

    There's also sh_eth_do_ioctl() that calls phy_mii_ioctl() which does PHY 
reads/writes. Although the device probably needs to be opened first.
Anyway, looks like we do need that Ben's patch or something alike -- maybe 
another RPM resume in sh_mdio_init()...

WBR, Sergei


[-- Attachment #2: sh_eth-fix-MDIO-probing-vs-open-race.patch --]
[-- Type: text/x-patch, Size: 3026 bytes --]

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/ethernet/renesas/sh_eth.c |   35 ++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

Index: renesas/drivers/net/ethernet/renesas/sh_eth.c
===================================================================
--- renesas.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ renesas/drivers/net/ethernet/renesas/sh_eth.c
@@ -2615,9 +2615,10 @@ static int sh_mdio_init(struct net_devic
 	int ret, i;
 	struct bb_info *bitbang;
 	struct sh_eth_private *mdp = netdev_priv(ndev);
+	struct platform_device *pdev = mdp->pdev;
 
 	/* create bit control struct for PHY */
-	bitbang = devm_kzalloc(&ndev->dev, sizeof(struct bb_info),
+	bitbang = devm_kzalloc(&pdev->dev, sizeof(struct bb_info),
 			       GFP_KERNEL);
 	if (!bitbang) {
 		ret = -ENOMEM;
@@ -2644,10 +2645,10 @@ static int sh_mdio_init(struct net_devic
 	mdp->mii_bus->name = "sh_mii";
 	mdp->mii_bus->parent = &ndev->dev;
 	snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
-		 mdp->pdev->name, id);
+		 pdev->name, id);
 
 	/* PHY IRQ */
-	mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
+	mdp->mii_bus->irq = devm_kzalloc(&pdev->dev,
 					 sizeof(int) * PHY_MAX_ADDR,
 					 GFP_KERNEL);
 	if (!mdp->mii_bus->irq) {
@@ -2655,10 +2656,9 @@ static int sh_mdio_init(struct net_devic
 		goto out_free_bus;
 	}
 
-	/* register mdio bus */
-	if (ndev->dev.parent->of_node) {
-		ret = of_mdiobus_register(mdp->mii_bus,
-					  ndev->dev.parent->of_node);
+	/* register MDIO bus */
+	if (pdev->dev.of_node) {
+		ret = of_mdiobus_register(mdp->mii_bus, pdev->dev.of_node);
 	} else {
 		for (i = 0; i < PHY_MAX_ADDR; i++)
 			mdp->mii_bus->irq[i] = PHY_POLL;
@@ -2904,6 +2904,13 @@ static int sh_eth_drv_probe(struct platf
 		}
 	}
 
+	/* MDIO bus init */
+	ret = sh_mdio_init(ndev, pdev->id, pd);
+	if (ret) {
+		dev_err(&ndev->dev, "failed to initialise MDIO\n");
+		goto out_release;
+	}
+
 	netif_napi_add(ndev, &mdp->napi, sh_eth_poll, 64);
 
 	/* network device register */
@@ -2911,13 +2918,6 @@ static int sh_eth_drv_probe(struct platf
 	if (ret)
 		goto out_napi_del;
 
-	/* mdio bus init */
-	ret = sh_mdio_init(ndev, pdev->id, pd);
-	if (ret) {
-		dev_err(&ndev->dev, "failed to initialise MDIO\n");
-		goto out_unregister;
-	}
-
 	/* print device information */
 	pr_info("Base address at 0x%x, %pM, IRQ %d.\n",
 		(u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
@@ -2926,12 +2926,11 @@ static int sh_eth_drv_probe(struct platf
 
 	return ret;
 
-out_unregister:
-	unregister_netdev(ndev);
-
 out_napi_del:
 	netif_napi_del(&mdp->napi);
 
+	sh_mdio_release(ndev);
+
 out_release:
 	/* net_dev free */
 	if (ndev)
@@ -2946,9 +2945,9 @@ static int sh_eth_drv_remove(struct plat
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct sh_eth_private *mdp = netdev_priv(ndev);
 
-	sh_mdio_release(ndev);
 	unregister_netdev(ndev);
 	netif_napi_del(&mdp->napi);
+	sh_mdio_release(ndev);
 	pm_runtime_disable(&pdev->dev);
 	free_netdev(ndev);
 

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

[Ben Dooks]

On 17/03/14 14:41, Laurent Pinchart wrote:
> Hi Ben,
>
> On Monday 17 March 2014 13:36:33 Ben Dooks wrote:
>
> [snip]
>
>>   From pm_runtime.h:
>>
>> static inline int pm_runtime_get_sync(struct device *dev)
>> {
>> 	return __pm_runtime_resume(dev, RPM_GET_PUT);
>> }
>>
>> static inline int pm_request_resume(struct device *dev)
>> {
>> 	return __pm_runtime_resume(dev, RPM_ASYNC);
>> }
>>
>> So it looks like pm_runtime_resume() does not protect against the
>> possibility that something else may re-suspend the device.
>
> Correct.
>
>> I have yet to ascertain how this ends up happening with device probe, it
>> seems to be very dependant on the code.
>
> We might be doing something wrong in the driver from a runtime PM point of
> view that leads to the device being suspended. I'd like to catch that instead
> of hiding it by a pm_runtime_get_sync() call. If it turns out that we're not
> doing anything wrong then replacing pm_runtime_resume() with
> pm_runtime_get_sync() would of course be fine.

My view is the pm_runtime)_resume() is just nasty, as it makes no
guarantees that the device cannot be re-suspended.


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

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

[Laurent Pinchart]

Hi Ben,

On Wednesday 19 March 2014 09:19:44 Ben Dooks wrote:
> On 17/03/14 14:41, Laurent Pinchart wrote:
> > On Monday 17 March 2014 13:36:33 Ben Dooks wrote:
> > 
> > [snip]
> > 
> >>   From pm_runtime.h:
> >> static inline int pm_runtime_get_sync(struct device *dev)
> >> {
> >> 	return __pm_runtime_resume(dev, RPM_GET_PUT);
> >> }
> >> 
> >> static inline int pm_request_resume(struct device *dev)
> >> {
> >> 	return __pm_runtime_resume(dev, RPM_ASYNC);
> >> }
> >> 
> >> So it looks like pm_runtime_resume() does not protect against the
> >> possibility that something else may re-suspend the device.
> > 
> > Correct.
> > 
> >> I have yet to ascertain how this ends up happening with device probe, it
> >> seems to be very dependant on the code.
> > 
> > We might be doing something wrong in the driver from a runtime PM point of
> > view that leads to the device being suspended. I'd like to catch that
> > instead of hiding it by a pm_runtime_get_sync() call. If it turns out
> > that we're not doing anything wrong then replacing pm_runtime_resume()
> > with pm_runtime_get_sync() would of course be fine.
> 
> My view is the pm_runtime)_resume() is just nasty, as it makes no guarantees
> that the device cannot be re-suspended.

My point is that requiring such a guarantee at probe time might be a sign that 
something else is wrong (which was the case with the sh-eth driver). I'm not 
saying it should never be done of course.

-- 
Regards,

Laurent Pinchart