| Message ID | 20240920100711.2744120-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: wwan: qcom_bam_dmux: Fix missing pm_runtime_disable() | expand |
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. Also please provide details of the platform on which you have tested your patch. > > Fixes: 21a0ffd9b38c ("net: wwan: Add Qualcomm BAM-DMUX WWAN network driver") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > drivers/net/wwan/qcom_bam_dmux.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) >
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. I think this looks reasonable, except that pm_runtime_set_suspended() should be redundant since it's the default runtime PM state. Thanks, Stephan
On Fri, Sep 20, 2024 at 06:07:11PM +0800, 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. > > Fixes: 21a0ffd9b38c ("net: wwan: Add Qualcomm BAM-DMUX WWAN network driver") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > drivers/net/wwan/qcom_bam_dmux.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c > index 26ca719fa0de..34a4e8095161 100644 > --- a/drivers/net/wwan/qcom_bam_dmux.c > +++ b/drivers/net/wwan/qcom_bam_dmux.c > @@ -823,17 +823,17 @@ static int bam_dmux_probe(struct platform_device *pdev) > ret = devm_request_threaded_irq(dev, pc_ack_irq, NULL, bam_dmux_pc_ack_irq, > IRQF_ONESHOT, NULL, dmux); > if (ret) > - return ret; > + goto err_disable_pm; > > ret = devm_request_threaded_irq(dev, dmux->pc_irq, NULL, bam_dmux_pc_irq, > IRQF_ONESHOT, NULL, dmux); > if (ret) > - return ret; > + goto err_disable_pm; > > ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL, > &dmux->pc_state); > if (ret) > - return ret; > + goto err_disable_pm; > > /* Check if remote finished initialization before us */ > if (dmux->pc_state) { > @@ -844,6 +844,12 @@ static int bam_dmux_probe(struct platform_device *pdev) > } > > return 0; > + > +err_disable_pm: > + pm_runtime_disable(dev); > + pm_runtime_dont_use_autosuspend(dev); > + pm_runtime_set_suspended(dev); Please drop the pm_runtime_set_suspended(dev); line, it should be unneeded since runtime PM documentation says: the initial runtime PM status of all devices is ‘suspended’ Thanks, Stephan
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. Are we guaranteed that the IRQs can not be delivered after suspending the device? Also is there a race between IRQs being enabled, manual check of the IRQ state and the pc_ack / power_off calls? > > I think this looks reasonable, except that pm_runtime_set_suspended() > should be redundant since it's the default runtime PM state. > > Thanks, > Stephan
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
On 2024/9/20 21:38, Stephan Gerhold wrote: > 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? Maybe use IRQF_NO_AUTOEN flag to reuqest irq and enable_irq() after that? > > Thanks, > Stephan
On Mon, Sep 23, 2024 at 10:25:28AM +0800, Jinjie Ruan wrote: > > > On 2024/9/20 21:38, Stephan Gerhold wrote: > > 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? > > Maybe use IRQF_NO_AUTOEN flag to reuqest irq and enable_irq() after that? > I thought about that too, but I think that might introduce a small window in between the two calls where we would miss the state change: irq_get_irqchip_state(..., IRQCHIP_STATE_LINE_LEVEL, ...); /* if an interrupt arrives here we will miss the state change */ enable_irq(); Thanks, Stephan
On 2024/9/20 20:45, Stephan Gerhold wrote: > On Fri, Sep 20, 2024 at 06:07:11PM +0800, 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. >> >> Fixes: 21a0ffd9b38c ("net: wwan: Add Qualcomm BAM-DMUX WWAN network driver") >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> drivers/net/wwan/qcom_bam_dmux.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c >> index 26ca719fa0de..34a4e8095161 100644 >> --- a/drivers/net/wwan/qcom_bam_dmux.c >> +++ b/drivers/net/wwan/qcom_bam_dmux.c >> @@ -823,17 +823,17 @@ static int bam_dmux_probe(struct platform_device *pdev) >> ret = devm_request_threaded_irq(dev, pc_ack_irq, NULL, bam_dmux_pc_ack_irq, >> IRQF_ONESHOT, NULL, dmux); >> if (ret) >> - return ret; >> + goto err_disable_pm; >> >> ret = devm_request_threaded_irq(dev, dmux->pc_irq, NULL, bam_dmux_pc_irq, >> IRQF_ONESHOT, NULL, dmux); >> if (ret) >> - return ret; >> + goto err_disable_pm; >> >> ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL, >> &dmux->pc_state); >> if (ret) >> - return ret; >> + goto err_disable_pm; >> >> /* Check if remote finished initialization before us */ >> if (dmux->pc_state) { >> @@ -844,6 +844,12 @@ static int bam_dmux_probe(struct platform_device *pdev) >> } >> >> return 0; >> + >> +err_disable_pm: >> + pm_runtime_disable(dev); >> + pm_runtime_dont_use_autosuspend(dev); >> + pm_runtime_set_suspended(dev); > > Please drop the pm_runtime_set_suspended(dev); line, it should be > unneeded since runtime PM documentation says: > > the initial runtime PM status of all devices is ‘suspended’ Thank you! > > Thanks, > Stephan
diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c index 26ca719fa0de..34a4e8095161 100644 --- a/drivers/net/wwan/qcom_bam_dmux.c +++ b/drivers/net/wwan/qcom_bam_dmux.c @@ -823,17 +823,17 @@ static int bam_dmux_probe(struct platform_device *pdev) ret = devm_request_threaded_irq(dev, pc_ack_irq, NULL, bam_dmux_pc_ack_irq, IRQF_ONESHOT, NULL, dmux); if (ret) - return ret; + goto err_disable_pm; ret = devm_request_threaded_irq(dev, dmux->pc_irq, NULL, bam_dmux_pc_irq, IRQF_ONESHOT, NULL, dmux); if (ret) - return ret; + goto err_disable_pm; ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL, &dmux->pc_state); if (ret) - return ret; + goto err_disable_pm; /* Check if remote finished initialization before us */ if (dmux->pc_state) { @@ -844,6 +844,12 @@ static int bam_dmux_probe(struct platform_device *pdev) } return 0; + +err_disable_pm: + pm_runtime_disable(dev); + pm_runtime_dont_use_autosuspend(dev); + pm_runtime_set_suspended(dev); + return ret; } static void bam_dmux_remove(struct platform_device *pdev)
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. Fixes: 21a0ffd9b38c ("net: wwan: Add Qualcomm BAM-DMUX WWAN network driver") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- drivers/net/wwan/qcom_bam_dmux.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)