Re: [PATCH] media: rkvdec: Fix an error handling path in rkvdec_probe()

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5260 bytes --]

Le mardi 29 juillet 2025 à 21:33 +0200, Christophe JAILLET a écrit :
> Le 29/07/2025 à 00:50, Nicolas Dufresne a écrit :
> > Hi,
> > 
> > Le dimanche 27 juillet 2025 à 12:02 +0200, Christophe JAILLET a écrit :
> > > If an error occurs after a successful iommu_paging_domain_alloc() call, it
> > > should be undone by a corresponding iommu_domain_free() call, as already
> > > done in the remove function.
> > > 
> > > Fixes: ff8c5622f9f7 ("media: rkvdec: Restore iommu addresses on errors")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > Compile tested only
> > > ---
> > >   drivers/media/platform/rockchip/rkvdec/rkvdec.c | 11 ++++++++---
> > >   1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > index d707088ec0dc..eb0d41f85d89 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> > > @@ -1169,15 +1169,17 @@ static int rkvdec_probe(struct platform_device *pdev)
> > >   	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> > >   
> > >   	irq = platform_get_irq(pdev, 0);
> > > -	if (irq <= 0)
> > > -		return -ENXIO;
> > > +	if (irq <= 0) {
> > > +		ret = -ENXIO;
> > > +		goto err_free_domain;
> > > +	}
> > >   
> > >   	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > >   					rkvdec_irq_handler, IRQF_ONESHOT,
> > >   					dev_name(&pdev->dev), rkvdec);
> > >   	if (ret) {
> > >   		dev_err(&pdev->dev, "Could not request vdec IRQ\n");
> > > -		return ret;
> > > +		goto err_free_domain;
> > >   	}
> > >   
> > >   	pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
> > 
> > Have you considered moving the allocation of the domain right above the above
> > line instead ? The empty domain can't possibly be used unless the probe have
> > fully completed.
> 
> That would not change things much. We still need to handle 
> rkvdec_v4l2_init() failure a few lines below.
> 
> If it is correct to move it at the very end of the function, after 
> rkvdec_v4l2_init(), then the patch would be simpler.
> 
> 
> Honestly, I'm not very confident with it. request_threaded_irq() 
> documentation states that "From the point this call is made your handler 
> function may be invoked."
> And rkvdec_irq_handler() may call rkvdec_iommu_restore() which uses 
> empty_domain.


This is a supposition in the doc. If you get familiar with codec, they either
have a firmware that needs to be booted, or it is trigger based, meaning if we
don't trigger any work, there will not be any interrupt. This is not true for
all kind of hardware though.

> 
> Not sure if I'm right and if this can happen, but the existing order 
> looks safer to me.
> 
> That said, if it is fine for you, I can send a v2.
> 
> 
> This would be:
> 
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c 
> b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> index d707088ec0dc..6eae10e16c73 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> @@ -1159,13 +1159,6 @@ static int rkvdec_probe(struct platform_device *pdev)
>                  return ret;
>          }
> 
> -       if (iommu_get_domain_for_dev(&pdev->dev)) {
> -               rkvdec->empty_domain = 
> iommu_paging_domain_alloc(rkvdec->dev);
> -
> -               if (!rkvdec->empty_domain)
> -                       dev_warn(rkvdec->dev, "cannot alloc new empty 
> domain\n");
> -       }
> -
>          vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> 
>          irq = platform_get_irq(pdev, 0);
> @@ -1188,6 +1181,13 @@ static int rkvdec_probe(struct platform_device *pdev)
>          if (ret)
>                  goto err_disable_runtime_pm;
> 
> +       if (iommu_get_domain_for_dev(&pdev->dev)) {
> +               rkvdec->empty_domain = 
> iommu_paging_domain_alloc(rkvdec->dev);
> +
> +               if (!rkvdec->empty_domain)
> +                       dev_warn(rkvdec->dev, "cannot alloc new empty 
> domain\n");
> +       }
> +
>          return 0;

For me this looks cleaner, but as you stated its a matter of taste more then
anything. A better answer lives in cleanup.h, but I'm not going to ask to port
drivers just yet.

So let me know, it can go like this too.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Nicolas

> 
>   err_disable_runtime_pm:
> 
> 
> CJ
> 
> > 
> > Nicolas
> > 
> > > @@ -1193,6 +1195,9 @@ static int rkvdec_probe(struct platform_device *pdev)
> > >   err_disable_runtime_pm:
> > >   	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > >   	pm_runtime_disable(&pdev->dev);
> > > +err_free_domain:
> > > +	if (rkvdec->empty_domain)
> > > +		iommu_domain_free(rkvdec->empty_domain);
> > >   	return ret;
> > >   }
> > >   

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]