Re: [PATCH] media: ipu3-cio2: disable MSI on probe failure and remove

[Sakari Ailus]

Hi Myeonghun,

Thank you for the patch.

On Wed, Jul 15, 2026 at 04:56:07PM +0900, Myeonghun Pak wrote:
> cio2_pci_probe() enables MSI with pci_enable_msi() but pci_disable_msi() is
> never called, so MSI is left enabled on the probe error paths and on normal
> removal in cio2_pci_remove().
> 
> The IRQ is requested with devm_request_irq() and freed by devres only after
> remove() (or a failed probe) returns, so a plain pci_disable_msi() in
> remove() would tear the MSI vector down before free_irq() runs. Register it
> with devm_add_action_or_reset() right after pci_enable_msi() instead: devres
> releases in reverse order, so the IRQ is freed before MSI is disabled, on
> every error path and on remove.
> 
> This issue was identified during our ongoing static-analysis research while
> reviewing kernel code.
> 
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 986b9afd7c..3f738ca681 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1654,6 +1654,13 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>  
>  /**************** PCI interface ****************/
>  
> +static void cio2_disable_msi(void *data)
> +{
> +	struct pci_dev *pci_dev = data;
> +
> +	pci_disable_msi(pci_dev);
> +}
> +
>  static int cio2_pci_probe(struct pci_dev *pci_dev,
>  			  const struct pci_device_id *id)
>  {
> @@ -1707,6 +1714,10 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  		return r;
>  	}
>  
> +	r = devm_add_action_or_reset(dev, cio2_disable_msi, pci_dev);

I think I'd do this without using devm_*().

Alternatively, pcim_enable_msi() could be nice. There would probably be
other similar functions that could benefit from similar wrappers so that
might be best kept separate in any case, also for backporting reasons.

> +	if (r)
> +		return r;
> +
>  	r = cio2_fbpt_init_dummy(cio2);
>  	if (r)
>  		return r;

-- 
Kind regards,

Sakari Ailus