Re: [PATCH] PCI: Fix devres regression in pci_intx()

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Christoph]

On Thu, Jul 25, 2024 at 02:07:30PM +0200, Philipp Stanner wrote:
> pci_intx() is a function that becomes managed if pcim_enable_device()
> has been called in advance. Commit 25216afc9db5 ("PCI: Add managed
> pcim_intx()") changed this behavior so that pci_intx() always leads to
> creation of a separate device resource for itself, whereas earlier, a
> shared resource was used for all PCI devres operations.
> 
> Unfortunately, pci_intx() seems to be used in some drivers' remove()
> paths; in the managed case this causes a device resource to be created
> on driver detach.
> 
> Fix the regression by only redirecting pci_intx() to its managed twin
> pcim_intx() if the pci_command changes.
> 
> Fixes: 25216afc9db5 ("PCI: Add managed pcim_intx()")
> Reported-by: Damien Le Moal <dlemoal@kernel.org>
> Closes: https://lore.kernel.org/all/b8f4ba97-84fc-4b7e-ba1a-99de2d9f0118@kernel.org/
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>

Applied to for-linus for v6.11, thanks!

> ---
> Alright, I reproduced this with QEMU as Damien described and this here
> fixes the issue on my side. Feedback welcome. Thank you very much,
> Damien.
> 
> It seems that this might yet again be the issue of drivers not being
> aware that pci_intx() might become managed, so they use it in their
> unwind path (rightfully so; there probably was no alternative back
> then).
> 
> That will make the long term cleanup difficult. But I think this for now
> is the most elegant possible workaround.
> ---
>  drivers/pci/pci.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e3a49f66982d..ffaaca0978cb 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4477,12 +4477,6 @@ void pci_intx(struct pci_dev *pdev, int enable)
>  {
>  	u16 pci_command, new;
>  
> -	/* Preserve the "hybrid" behavior for backwards compatibility */
> -	if (pci_is_managed(pdev)) {
> -		WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
> -		return;
> -	}
> -
>  	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
>  
>  	if (enable)
> @@ -4490,8 +4484,15 @@ void pci_intx(struct pci_dev *pdev, int enable)
>  	else
>  		new = pci_command | PCI_COMMAND_INTX_DISABLE;
>  
> -	if (new != pci_command)
> +	if (new != pci_command) {
> +		/* Preserve the "hybrid" behavior for backwards compatibility */
> +		if (pci_is_managed(pdev)) {
> +			WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
> +			return;
> +		}
> +
>  		pci_write_config_word(pdev, PCI_COMMAND, new);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(pci_intx);
>  
> -- 
> 2.45.2
>