Re: [PATCH] net: wwan: qcom_bam_dmux: Fix missing pm_runtime_disable()

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Fri, Sep 20, 2024 at 03:05:13PM +0200, Dmitry Baryshkov wrote:
> On Fri, 20 Sept 2024 at 14:44, Stephan Gerhold
> <stephan.gerhold@linaro.org> wrote:
> >
> > On Fri, Sep 20, 2024 at 01:48:15PM +0300, Dmitry Baryshkov wrote:
> > > On Fri, Sep 20, 2024 at 06:07:11PM GMT, Jinjie Ruan wrote:
> > > > It's important to undo pm_runtime_use_autosuspend() with
> > > > pm_runtime_dont_use_autosuspend() at driver exit time.
> > > >
> > > > But the pm_runtime_disable() and pm_runtime_dont_use_autosuspend()
> > > > is missing in the error path for bam_dmux_probe(). So add it.
> > >
> > > Please use devm_pm_runtime_enable(), which handles autosuspend.
> > >
> >
> > This would conflict with the existing cleanup in bam_dmux_remove(),
> > which probably needs to stay manually managed since the tear down order
> > is quite important there.
> 
> Hmm, the setup and teardown code makes me wonder now.

Yeah, you ask the right questions. :-) It's really tricky to get this
100% right. I spent quite some time to get close, but there are likely
still some loopholes. I haven't heard of anyone running into trouble,
though. This driver has been rock solid for the past few years.

> Are we guaranteed that the IRQs can not be delivered after suspending
> the device?

I think bam_dmux_remove() should be safe. disable_irq(dmux->pc_irq)
prevents any further delivery of IRQs before doing the final power off.

> Also is there a race between IRQs being enabled, manual check of the
> IRQ state and the pc_ack / power_off calls?

Yes, I'm pretty sure this race exists in theory. I'm not sure how to
avoid it. We would need an atomic "return current state and enable IRQ"
operation, but I don't think this exists at the moment. Do you have any
suggestions?

Thanks,
Stephan