[v1] media: venus: hfi: explicitly release IRQ during teardown

[PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Jorge Ramirez-Ortiz 8 months ago

Ensure the IRQ is released before dismantling the ISR handler and
clearing related pointers.

This prevents any possibility of the interrupt triggering after the
handler context has been invalidated.

Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index b5f2ea879950..d9d62d965bcf 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
 	venus_interface_queues_release(hdev);
 	mutex_destroy(&hdev->lock);
 	kfree(hdev);
+	devm_free_irq(core->dev, core->irq, core);
 	core->ops = NULL;
 }
 
-- 
2.34.1

Re: [PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Vikash Garodia 7 months, 3 weeks ago

On 6/12/2025 1:59 PM, Jorge Ramirez-Ortiz wrote:
> Ensure the IRQ is released before dismantling the ISR handler and
> clearing related pointers.
> 
> This prevents any possibility of the interrupt triggering after the
> handler context has been invalidated.
> 
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> ---
>  drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index b5f2ea879950..d9d62d965bcf 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
>  	venus_interface_queues_release(hdev);
>  	mutex_destroy(&hdev->lock);
>  	kfree(hdev);
> +	devm_free_irq(core->dev, core->irq, core);
Could you please check and add the handling here [1] as well ?

[1]
https://elixir.bootlin.com/linux/v6.16-rc1/source/drivers/media/platform/qcom/venus/core.c#L427

Regards,
Vikash
>  	core->ops = NULL;
>  }
>

Re: [PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Jorge Ramirez 7 months, 3 weeks ago

On 16/06/25 17:26:24, Vikash Garodia wrote:
> 
> On 6/12/2025 1:59 PM, Jorge Ramirez-Ortiz wrote:
> > Ensure the IRQ is released before dismantling the ISR handler and
> > clearing related pointers.
> > 
> > This prevents any possibility of the interrupt triggering after the
> > handler context has been invalidated.
> > 
> > Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> > ---
> >  drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> > index b5f2ea879950..d9d62d965bcf 100644
> > --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> > @@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
> >  	venus_interface_queues_release(hdev);
> >  	mutex_destroy(&hdev->lock);
> >  	kfree(hdev);
> > +	devm_free_irq(core->dev, core->irq, core);
> Could you please check and add the handling here [1] as well ?
> 
> [1]
> https://elixir.bootlin.com/linux/v6.16-rc1/source/drivers/media/platform/qcom/venus/core.c#L427

hi Vikash, sorry I dont get your point - what do you mean?

> 
> Regards,
> Vikash
> >  	core->ops = NULL;
> >  }
> >

Re: [PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Vikash Garodia 7 months, 3 weeks ago

Hi Jorge,

On 6/16/2025 6:29 PM, Jorge Ramirez wrote:
> On 16/06/25 17:26:24, Vikash Garodia wrote:
>>
>> On 6/12/2025 1:59 PM, Jorge Ramirez-Ortiz wrote:
>>> Ensure the IRQ is released before dismantling the ISR handler and
>>> clearing related pointers.
>>>
>>> This prevents any possibility of the interrupt triggering after the
>>> handler context has been invalidated.
>>>
>>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
>>> ---
>>>  drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>>> index b5f2ea879950..d9d62d965bcf 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>>> @@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
>>>  	venus_interface_queues_release(hdev);
>>>  	mutex_destroy(&hdev->lock);
>>>  	kfree(hdev);
>>> +	devm_free_irq(core->dev, core->irq, core);
>> Could you please check and add the handling here [1] as well ?
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.16-rc1/source/drivers/media/platform/qcom/venus/core.c#L427
> 
> hi Vikash, sorry I dont get your point - what do you mean?
IRQ need to be freed even for error cases during venus_probe().

Regards,
Vikash
> 
>>
>> Regards,
>> Vikash
>>>  	core->ops = NULL;
>>>  }
>>>

Re: [PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Jorge Ramirez 7 months, 3 weeks ago

On 16/06/25 20:14:36, Vikash Garodia wrote:
> Hi Jorge,
> 
> On 6/16/2025 6:29 PM, Jorge Ramirez wrote:
> > On 16/06/25 17:26:24, Vikash Garodia wrote:
> >>
> >> On 6/12/2025 1:59 PM, Jorge Ramirez-Ortiz wrote:
> >>> Ensure the IRQ is released before dismantling the ISR handler and
> >>> clearing related pointers.
> >>>
> >>> This prevents any possibility of the interrupt triggering after the
> >>> handler context has been invalidated.
> >>>
> >>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> >>> ---
> >>>  drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> >>> index b5f2ea879950..d9d62d965bcf 100644
> >>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> >>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> >>> @@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
> >>>  	venus_interface_queues_release(hdev);
> >>>  	mutex_destroy(&hdev->lock);
> >>>  	kfree(hdev);
> >>> +	devm_free_irq(core->dev, core->irq, core);
> >> Could you please check and add the handling here [1] as well ?
> >>
> >> [1]
> >> https://elixir.bootlin.com/linux/v6.16-rc1/source/drivers/media/platform/qcom/venus/core.c#L427
> > 
> > hi Vikash, sorry I dont get your point - what do you mean?
> IRQ need to be freed even for error cases during venus_probe().
>

but  this is what the current patch does (venus_hfi_destroy is called at
the end of probe error handling as well).

> Regards,
> Vikash
> > 
> >>
> >> Regards,
> >> Vikash
> >>>  	core->ops = NULL;
> >>>  }
> >>>

Re: [PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Jorge Ramirez 7 months, 3 weeks ago

On 16/06/25 17:32:38, Jorge Ramirez wrote:
> On 16/06/25 20:14:36, Vikash Garodia wrote:
> > Hi Jorge,
> > 
> > On 6/16/2025 6:29 PM, Jorge Ramirez wrote:
> > > On 16/06/25 17:26:24, Vikash Garodia wrote:
> > >>
> > >> On 6/12/2025 1:59 PM, Jorge Ramirez-Ortiz wrote:
> > >>> Ensure the IRQ is released before dismantling the ISR handler and
> > >>> clearing related pointers.
> > >>>
> > >>> This prevents any possibility of the interrupt triggering after the
> > >>> handler context has been invalidated.
> > >>>
> > >>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> > >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> > >>> ---
> > >>>  drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
> > >>>  1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> > >>> index b5f2ea879950..d9d62d965bcf 100644
> > >>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> > >>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> > >>> @@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
> > >>>  	venus_interface_queues_release(hdev);
> > >>>  	mutex_destroy(&hdev->lock);
> > >>>  	kfree(hdev);
> > >>> +	devm_free_irq(core->dev, core->irq, core);
> > >> Could you please check and add the handling here [1] as well ?
> > >>
> > >> [1]
> > >> https://elixir.bootlin.com/linux/v6.16-rc1/source/drivers/media/platform/qcom/venus/core.c#L427
> > > 
> > > hi Vikash, sorry I dont get your point - what do you mean?
> > IRQ need to be freed even for error cases during venus_probe().
> >
> 
> but  this is what the current patch does (venus_hfi_destroy is called at
> the end of probe error handling as well).
>

for background, this fixes a null derreference in the Venus driver -
reproduceable in RB3Gen2 on a particular error condition during probe.

> > Regards,
> > Vikash
> > > 
> > >>
> > >> Regards,
> > >> Vikash
> > >>>  	core->ops = NULL;
> > >>>  }
> > >>>

Re: [PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Bryan O'Donoghue 7 months, 3 weeks ago

On 18/06/2025 08:07, Jorge Ramirez wrote:
> On 16/06/25 17:32:38, Jorge Ramirez wrote:
>> On 16/06/25 20:14:36, Vikash Garodia wrote:
>>> Hi Jorge,
>>>
>>> On 6/16/2025 6:29 PM, Jorge Ramirez wrote:
>>>> On 16/06/25 17:26:24, Vikash Garodia wrote:
>>>>>
>>>>> On 6/12/2025 1:59 PM, Jorge Ramirez-Ortiz wrote:
>>>>>> Ensure the IRQ is released before dismantling the ISR handler and
>>>>>> clearing related pointers.
>>>>>>
>>>>>> This prevents any possibility of the interrupt triggering after the
>>>>>> handler context has been invalidated.
>>>>>>
>>>>>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>>>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
>>>>>> ---
>>>>>>   drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
>>>>>>   1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>>>>>> index b5f2ea879950..d9d62d965bcf 100644
>>>>>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>>>>>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>>>>>> @@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
>>>>>>   	venus_interface_queues_release(hdev);
>>>>>>   	mutex_destroy(&hdev->lock);
>>>>>>   	kfree(hdev);
>>>>>> +	devm_free_irq(core->dev, core->irq, core);
>>>>> Could you please check and add the handling here [1] as well ?
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/linux/v6.16-rc1/source/drivers/media/platform/qcom/venus/core.c#L427
>>>>
>>>> hi Vikash, sorry I dont get your point - what do you mean?
>>> IRQ need to be freed even for error cases during venus_probe().
>>>
>>
>> but  this is what the current patch does (venus_hfi_destroy is called at
>> the end of probe error handling as well).
>>
> 
> for background, this fixes a null derreference in the Venus driver -
> reproduceable in RB3Gen2 on a particular error condition during probe.
Shouldn't it be the case that devm removes the handler for us anyway ?

Why not ->         disable_irq_nosync(core->irq);

i.e. disable the IRQ until the normal/expected exit path removes it.

---
bod

Re: [PATCH] media: venus: hfi: explicitly release IRQ during teardown

Posted by Jorge Ramirez 7 months, 3 weeks ago

On 18/06/25 12:08:28, Bryan O'Donoghue wrote:
> On 18/06/2025 08:07, Jorge Ramirez wrote:
> > On 16/06/25 17:32:38, Jorge Ramirez wrote:
> > > On 16/06/25 20:14:36, Vikash Garodia wrote:
> > > > Hi Jorge,
> > > > 
> > > > On 6/16/2025 6:29 PM, Jorge Ramirez wrote:
> > > > > On 16/06/25 17:26:24, Vikash Garodia wrote:
> > > > > > 
> > > > > > On 6/12/2025 1:59 PM, Jorge Ramirez-Ortiz wrote:
> > > > > > > Ensure the IRQ is released before dismantling the ISR handler and
> > > > > > > clearing related pointers.
> > > > > > > 
> > > > > > > This prevents any possibility of the interrupt triggering after the
> > > > > > > handler context has been invalidated.
> > > > > > > 
> > > > > > > Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
> > > > > > > ---
> > > > > > >   drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
> > > > > > >   1 file changed, 1 insertion(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> > > > > > > index b5f2ea879950..d9d62d965bcf 100644
> > > > > > > --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> > > > > > > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> > > > > > > @@ -1678,6 +1678,7 @@ void venus_hfi_destroy(struct venus_core *core)
> > > > > > >   	venus_interface_queues_release(hdev);
> > > > > > >   	mutex_destroy(&hdev->lock);
> > > > > > >   	kfree(hdev);
> > > > > > > +	devm_free_irq(core->dev, core->irq, core);
> > > > > > Could you please check and add the handling here [1] as well ?
> > > > > > 
> > > > > > [1]
> > > > > > https://elixir.bootlin.com/linux/v6.16-rc1/source/drivers/media/platform/qcom/venus/core.c#L427
> > > > > 
> > > > > hi Vikash, sorry I dont get your point - what do you mean?
> > > > IRQ need to be freed even for error cases during venus_probe().
> > > > 
> > > 
> > > but  this is what the current patch does (venus_hfi_destroy is called at
> > > the end of probe error handling as well).
> > > 
> > 
> > for background, this fixes a null derreference in the Venus driver -
> > reproduceable in RB3Gen2 on a particular error condition during probe.
> Shouldn't it be the case that devm removes the handler for us anyway ?
> 
> Why not ->         disable_irq_nosync(core->irq);

I agree, this seems better to me too.

I guess disable_irq() is the safer/more meaningfull choice since we are
calling from non irq context.

will fix - thanks for the suggestion!

> 
> i.e. disable the IRQ until the normal/expected exit path removes it.
> 
> ---
> bod