Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Kalra, Ashish]

Hello Philipp,

I have reviewed and tested this patch, looks to be working fine and fixes the issue.

Thanks, Ashish

On 7/9/2024 3:56 AM, Philipp Stanner wrote:
> From c24bd5b66e798a341caf183fb7cdbdf235502d90 Mon Sep 17 00:00:00 2001
> From: Philipp Stanner <pstanner@xxxxxxxxxx>
> Date: Tue, 9 Jul 2024 09:45:48 +0200
> Subject: [PATCH] PCI: Fix pcim_intx() recursive calls
>
> pci_intx() calls into pcim_intx() in managed mode, i.e., when
> pcim_enable_device() had been called. This recursive call causes a bug
> by re-registering the device resource in the release callback.
>
> This is the same phenomenon that made it necessary to implement some
> functionality a second time, see __pcim_request_region().
>
> Implement __pcim_intx() to bypass the hybrid nature of pci_intx() on
> driver detach.
>
> Fixes: https://lore.kernel.org/all/20240708214656.4721-1-Ashish.Kalra@xxxxxxx/
> Reported-by: Ashish Kalra <Ashish.Kalra@xxxxxxx>
> Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> ---
> Hi Ashish,
> I hacked down this fix that should be applyable on top.
> Could you maybe have a first quick look whether this fixes the issue?
> ---
>  drivers/pci/devres.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 2f0379a4e58f..dcef049b72fe 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -408,12 +408,31 @@ static inline bool mask_contains_bar(int mask, int bar)
>  	return mask & BIT(bar);
>  }
>  
> +/*
> + * This is a copy of pci_intx() used to bypass the problem of occuring
> + * recursive function calls due to the hybrid nature of pci_intx().
> + */
> +static void __pcim_intx(struct pci_dev *pdev, int enable)
> +{
> +	u16 pci_command, new;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> +
> +	if (enable)
> +		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> +	else
> +		new = pci_command | PCI_COMMAND_INTX_DISABLE;
> +
> +	if (new != pci_command)
> +		pci_write_config_word(pdev, PCI_COMMAND, new);
> +}
> +
>  static void pcim_intx_restore(struct device *dev, void *data)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct pcim_intx_devres *res = data;
>  
> -	pci_intx(pdev, res->orig_intx);
> +	__pcim_intx(pdev, res->orig_intx);
>  }
>  
>  static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
> @@ -443,7 +462,6 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
>   */
>  int pcim_intx(struct pci_dev *pdev, int enable)
>  {
> -	u16 pci_command, new;
>  	struct pcim_intx_devres *res;
>  
>  	res = get_or_create_intx_devres(&pdev->dev);
> @@ -451,16 +469,7 @@ int pcim_intx(struct pci_dev *pdev, int enable)
>  		return -ENOMEM;
>  
>  	res->orig_intx = !enable;
> -
> -	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> -
> -	if (enable)
> -		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> -	else
> -		new = pci_command | PCI_COMMAND_INTX_DISABLE;
> -
> -	if (new != pci_command)
> -		pci_write_config_word(pdev, PCI_COMMAND, new);
> +	__pcim_intx(pdev, enable);
>  
>  	return 0;
>  }
Tested-by: Ashish Kalra <Ashish.Kalra@xxxxxxx>