The multiple BWMONv4 instances available on the X1E80100 SoC use the
same interrupt number. Mark them are shared to allow for re-use across
instances.
Signed-off-by: Sibi Sankar <[email protected]>
---
drivers/soc/qcom/icc-bwmon.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
index fb323b3364db..d69e0797eeda 100644
--- a/drivers/soc/qcom/icc-bwmon.c
+++ b/drivers/soc/qcom/icc-bwmon.c
@@ -783,7 +783,8 @@ static int bwmon_probe(struct platform_device *pdev)
bwmon_disable(bwmon);
ret = devm_request_threaded_irq(dev, bwmon->irq, bwmon_intr,
bwmon_intr_thread,
- IRQF_ONESHOT, dev_name(dev), bwmon);
+ IRQF_ONESHOT | IRQF_SHARED,
+ dev_name(dev), bwmon);
if (ret)
return dev_err_probe(dev, ret, "failed to request IRQ\n");
--
2.34.1
On 04/06/2024 03:11, Sibi Sankar wrote:
> The multiple BWMONv4 instances available on the X1E80100 SoC use the
> same interrupt number. Mark them are shared to allow for re-use across
> instances.
Would be nice if you also mention you checked that it is safe to have
both devm and shared interrupts (so you investigated possibility of race
on exit path).
Best regards,
Krzysztof
On 6/4/24 12:16, Krzysztof Kozlowski wrote:
> On 04/06/2024 03:11, Sibi Sankar wrote:
>> The multiple BWMONv4 instances available on the X1E80100 SoC use the
>> same interrupt number. Mark them are shared to allow for re-use across
>> instances.
Hey Krzysztof,
Thanks for taking time to review the series :)
>
> Would be nice if you also mention you checked that it is safe to have
> both devm and shared interrupts (so you investigated possibility of race
> on exit path).
I didn't see any problems with devm being used with SHARED when I posted
it out. After your review comments I went back again to vett the exit
path for races and ran into an pre-existing splat [1] but the bwmon
instances work as expected on module removal/re-insertion.
[1] -
https://lore.kernel.org/lkml/[email protected]/
-Sibi
>
> Best regards,
> Krzysztof
>
On 13/06/2024 19:02, Sibi Sankar wrote:
>
>
> On 6/4/24 12:16, Krzysztof Kozlowski wrote:
>> On 04/06/2024 03:11, Sibi Sankar wrote:
>>> The multiple BWMONv4 instances available on the X1E80100 SoC use the
>>> same interrupt number. Mark them are shared to allow for re-use across
>>> instances.
>
> Hey Krzysztof,
>
> Thanks for taking time to review the series :)
>
>>
>> Would be nice if you also mention you checked that it is safe to have
>> both devm and shared interrupts (so you investigated possibility of race
>> on exit path).
>
> I didn't see any problems with devm being used with SHARED when I posted
> it out. After your review comments I went back again to vett the exit
> path for races and ran into an pre-existing splat [1] but the bwmon
> instances work as expected on module removal/re-insertion.
Using devm and shared interrupts is in general sign of possible race
issues and should be avoided. Just "not seeing problems" is not an
argument for me, to be honest.
Best regards,
Krzysztof
On 6/14/24 13:54, Krzysztof Kozlowski wrote:
> On 13/06/2024 19:02, Sibi Sankar wrote:
>>
>>
>> On 6/4/24 12:16, Krzysztof Kozlowski wrote:
>>> On 04/06/2024 03:11, Sibi Sankar wrote:
>>>> The multiple BWMONv4 instances available on the X1E80100 SoC use the
>>>> same interrupt number. Mark them are shared to allow for re-use across
>>>> instances.
>>
>> Hey Krzysztof,
>>
>> Thanks for taking time to review the series :)
>>
>>>
>>> Would be nice if you also mention you checked that it is safe to have
>>> both devm and shared interrupts (so you investigated possibility of race
>>> on exit path).
>>
>> I didn't see any problems with devm being used with SHARED when I posted
>> it out. After your review comments I went back again to vett the exit
>> path for races and ran into an pre-existing splat [1] but the bwmon
>> instances work as expected on module removal/re-insertion.
>
> Using devm and shared interrupts is in general sign of possible race
> issues and should be avoided. Just "not seeing problems" is not an
> argument for me, to be honest.
Didn't I go further and say I got it tested though? Also can you
elaborate on what race do you think the bwmon will hit rather than
being too generic about it?
-Sibi
>
> Best regards,
> Krzysztof
>
On Sat, Jun 15, 2024 at 01:49:34AM GMT, Sibi Sankar wrote:
>
>
> On 6/14/24 13:54, Krzysztof Kozlowski wrote:
> > On 13/06/2024 19:02, Sibi Sankar wrote:
> > >
> > >
> > > On 6/4/24 12:16, Krzysztof Kozlowski wrote:
> > > > On 04/06/2024 03:11, Sibi Sankar wrote:
> > > > > The multiple BWMONv4 instances available on the X1E80100 SoC use the
> > > > > same interrupt number. Mark them are shared to allow for re-use across
> > > > > instances.
> > >
> > > Hey Krzysztof,
> > >
> > > Thanks for taking time to review the series :)
> > >
> > > >
> > > > Would be nice if you also mention you checked that it is safe to have
> > > > both devm and shared interrupts (so you investigated possibility of race
> > > > on exit path).
> > >
> > > I didn't see any problems with devm being used with SHARED when I posted
> > > it out. After your review comments I went back again to vett the exit
> > > path for races and ran into an pre-existing splat [1] but the bwmon
> > > instances work as expected on module removal/re-insertion.
> >
> > Using devm and shared interrupts is in general sign of possible race
> > issues and should be avoided. Just "not seeing problems" is not an
> > argument for me, to be honest.
>
> Didn't I go further and say I got it tested though? Also can you
> elaborate on what race do you think the bwmon will hit rather than
> being too generic about it?
devm_request_threaded_irq means that the IRQ is freed after the
bwmon_remove() function returns. Having IRQF_SHARED means that the IRQ
can still be triggered even though IRQ for this device has been disabled
in bwmon_disable().
In this particular case such IRQ probably won't cause issues, but at
least it needs to be validated and probably commented in bwmon_remove().
Just stating that "you tested and had no problems" usually isn't enough
for the expected race condition issues.
--
With best wishes
Dmitry
On 6/15/24 03:12, Dmitry Baryshkov wrote:
> On Sat, Jun 15, 2024 at 01:49:34AM GMT, Sibi Sankar wrote:
>>
>>
>> On 6/14/24 13:54, Krzysztof Kozlowski wrote:
>>> On 13/06/2024 19:02, Sibi Sankar wrote:
>>>>
>>>>
>>>> On 6/4/24 12:16, Krzysztof Kozlowski wrote:
>>>>> On 04/06/2024 03:11, Sibi Sankar wrote:
>>>>>> The multiple BWMONv4 instances available on the X1E80100 SoC use the
>>>>>> same interrupt number. Mark them are shared to allow for re-use across
>>>>>> instances.
>>>>
>>>> Hey Krzysztof,
>>>>
>>>> Thanks for taking time to review the series :)
>>>>
>>>>>
>>>>> Would be nice if you also mention you checked that it is safe to have
>>>>> both devm and shared interrupts (so you investigated possibility of race
>>>>> on exit path).
>>>>
>>>> I didn't see any problems with devm being used with SHARED when I posted
>>>> it out. After your review comments I went back again to vett the exit
>>>> path for races and ran into an pre-existing splat [1] but the bwmon
>>>> instances work as expected on module removal/re-insertion.
>>>
>>> Using devm and shared interrupts is in general sign of possible race
>>> issues and should be avoided. Just "not seeing problems" is not an
>>> argument for me, to be honest.
>>
>> Didn't I go further and say I got it tested though? Also can you
>> elaborate on what race do you think the bwmon will hit rather than
>> being too generic about it?
>
> devm_request_threaded_irq means that the IRQ is freed after the
> bwmon_remove() function returns. Having IRQF_SHARED means that the IRQ
> can still be triggered even though IRQ for this device has been disabled
> in bwmon_disable().
>
> In this particular case such IRQ probably won't cause issues, but at
> least it needs to be validated and probably commented in bwmon_remove().
> Just stating that "you tested and had no problems" usually isn't enough
> for the expected race condition issues.
Cool, thanks for the info. I'll get this fixed in the next re-spin.
-Sibi
>