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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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