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

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

[Jinjie Ruan]

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);
+	return ret;
 }
 
 static void bam_dmux_remove(struct platform_device *pdev)
-- 
2.34.1

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Stephan Gerhold]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Stephan Gerhold]

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

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

[Jinjie Ruan]

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

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

[Stephan Gerhold]

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

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

[Stephan Gerhold]

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

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

[Jinjie Ruan]

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