Re: [PATCH] PCI: mvebu: Dispose INTx irqs prior to removing INTx domain

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

+ Marc

On Wed, Jun 19, 2024 at 04:28:29PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Documentation for irq_domain_remove() says that all mapping within the
> domain must be disposed prior to domain remove.
> 
> Currently INTx irqs are not disposed in pci-mvebu.c device unbind callback
> which cause that kernel crashes after unloading driver and trying to read
> /sys/kernel/debug/irq/irqs/<num> or /proc/interrupts.
> 

Thanks a lot for respinning this patch. It is indeed fixing a real problem since
this driver can be unloaded runtime.

It is always a debate on whether an irqchip controller should be allowed to be
removed runtime or not, but I hope Marc will provide some inputs here.

> Fixes: ec075262648f ("PCI: mvebu: Implement support for legacy INTx interrupts")
> Reported-by: Hajo Noerenberg <hajo-linux-bugzilla@noerenberg.de>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <kabel@kernel.org>
> [ refactored a little ]
> Signed-off-by: Marek Behún <kabel@kernel.org>

But this patch looks good to me. I hope that we should be able to use this patch
as a precedent for other drivers as well.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> This was discussed back in 2022
>   https://lore.kernel.org/linux-arm-kernel/20220709161858.15031-1-pali@kernel.org/
> IMO Pali gave good arguments about why it should be applied, and Lorenzo
> agreed.
> 
> Can we get this applied?
> ---
>  drivers/pci/controller/pci-mvebu.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 29fe09c99e7d..91a02b23aeb1 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -1683,8 +1683,15 @@ static void mvebu_pcie_remove(struct platform_device *pdev)
>  			irq_set_chained_handler_and_data(irq, NULL, NULL);
>  
>  		/* Remove IRQ domains. */
> -		if (port->intx_irq_domain)
> +		if (port->intx_irq_domain) {
> +			for (int j = 0; j < PCI_NUM_INTX; j++) {
> +				int virq = irq_find_mapping(port->intx_irq_domain, j);
> +
> +				if (virq > 0)
> +					irq_dispose_mapping(virq);
> +			}
>  			irq_domain_remove(port->intx_irq_domain);
> +		}
>  
>  		/* Free config space for emulated root bridge. */
>  		pci_bridge_emul_cleanup(&port->bridge);
> -- 
> 2.44.2
> 
> 

-- 
மணிவண்ணன் சதாசிவம்