Hi Phil,
On Thu, Jul 7, 2022 at 6:37 PM Phil Edworthy <phil.edworthy@xxxxxxxxxxx> wrote:
> On 07 July 2022 08:21 Phil Edworthy wrote:
> > On 03 July 2022 16:17 Andy Shevchenko wrote:
> > > On Sun, Jul 03, 2022 at 10:41:45AM +0200, Geert Uytterhoeven wrote:
> > > > On Sat, Jul 2, 2022 at 1:51 PM Andy Shevchenko
> > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > On Fri, Jul 01, 2022 at 05:39:16PM +0100, Phil Edworthy wrote:
> > > > > > Yet another i2c controller from Renesas that is found on the
> > RZ/V2M
> > > > > > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
> > >
> > > ...
> > All other suggested changes are ok.
> >
> >
> > > > > > + pm_runtime_get_sync(dev);
> > > >
> > > > pm_runtime_resume_and_get() ;-)
> > >
> > > This makes sense only if we test for error. Otherwise the put might
> > > imbalance
> > > counter.
> > I added code to check the return value and to my surprise it returned
> > -EACCES.
> > Some digging later, this only happens when I have an i2c controller
> > enabled that doesn't have any children.
> >
> > rpm_resume() returns -EACCES [1] because runtime_status and last_status
> > are set to RPM_SUSPENDED.
> >
> > The i2c controller that does have a child has runtime_status = RPM_ACTIVE
> > as there is a call to pm_runtime_resume_and_get() on it due to the i2c
> > controller performing an i2c transfer for the slave device.
> >
> > I am currently struggling to work out why this is happening...
>
> First pm_suspend() works it's way down to __pm_runtime_disable():
> __pm_runtime_disable+0x134/0x1e0
> __device_suspend_late+0x28/0x1c4
> dpm_suspend_late+0x158/0x230
> suspend_devices_and_enter+0x1c8/0x4b4
> pm_suspend+0x210/0x28c
> At the end of which, runtime_status and last_status are both RPM_SUSPENDED,
> and disable_depth = 1 [1]
>
> After that rzv2m_i2c_suspend() is called triggering the EACCES error
> condition [2]:
> rpm_resume+0x338/0x630
> __pm_runtime_resume+0x4c/0x80
> rzv2m_i2c_suspend+0x24/0xb0
> pm_generic_suspend_noirq+0x30/0x50
> genpd_finish_suspend+0xb0/0x130
> genpd_suspend_noirq+0x14/0x20
> __device_suspend_noirq+0x68/0x1d0
> dpm_noirq_suspend_devices+0x110/0x1dc
> dpm_suspend_noirq+0x24/0xa0
> suspend_devices_and_enter+0x2f0/0x4b4
> pm_suspend+0x210/0x28c
>
> I think using runtime PM from within driver suspend/resume is simply not
> supported. However I had some difficulty following the runtime PM code,
> so I could be wrong.
Oh, it's a NOIRQ system sleep op. You indeed cannot use runtime resume
from such a callback, as the latter may sleep.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
