Re: REGRESSION with pcim_intx()

[Philipp Stanner <pstanner@redhat.com>]

On Wed, 2024-07-24 at 14:13 +0900, Damien Le Moal wrote:
> On 7/24/24 1:56 PM, Damien Le Moal wrote:
> > 
> > Commit 25216afc9db5 ("PCI: Add managed pcim_intx()") is causing a
> > regression,
> > which is easy to see using qemu with an AHCI device and the ahci
> > driver
> > compiled as a module.
> > 
> > 1) Boot qemu: the AHCI controller is initialized and the drive(s)
> > attached to
> > it visible.
> > 2) Run "rmmod ahci": the drives go away, all normal
> > 3) Re-initialize the AHCI adapter and rescan the drives by running
> > "modprobe
> > ahci". That fails with the message "pci 0000:00:1f.2: Resources
> > present before
> > probing"
> > 
> > The reason is that before commit 25216afc9db5, pci_intx(dev, 0) was
> > called to
> > disable INTX as MSI are used for the adapter, and for that case,
> > pci_intx()
> > would NOT allocate a device resource if the INTX enable/disable
> > state was not
> > being changed:
> > 
> > 	if (new != pci_command) {
> > 		struct pci_devres *dr;
> > 
> > 		pci_write_config_word(pdev, PCI_COMMAND, new);
> > 
> > 		dr = find_pci_dr(pdev);
> > 		if (dr && !dr->restore_intx) {
> > 			dr->restore_intx = 1;
> > 			dr->orig_intx = !enable;
> > 		}
> > 	}
> > 
> > The former code was only looking for the resource and not
> > allocating it.
> > 
> > Now, with pcim_intx() being used, the intx resource is *always*
> > allocated,
> > including when INTX is disabled when the device is being disabled
> > on rmmod.
> > This leads to the device resource list to always have the intx
> > resource
> > remaining and thus causes the modprobe error.
> > 
> > Reverting Commit 25216afc9db5 is one solution to fix this, and I
> > can send a
> > patch for that, unless someone has an idea how to fix this ? I
> > tried but I do
> > not see a clean way of fixing this...
> > Thoughts ?
> 
> This change works as a fix, but it is not pretty...
> 
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 3780a9f9ec00..4e14f87e3d22 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -466,13 +466,22 @@ static struct pcim_intx_devres
> *get_or_create_intx_devres(struct device *dev)
>  int pcim_intx(struct pci_dev *pdev, int enable)
>  {
>         struct pcim_intx_devres *res;
> +       u16 pci_command, new;
>  
> -       res = get_or_create_intx_devres(&pdev->dev);
> -       if (!res)
> -               return -ENOMEM;
> +       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) {
> +               res = get_or_create_intx_devres(&pdev->dev);
> +               if (!res)
> +                       return -ENOMEM;
>  
> -       res->orig_intx = !enable;
> -       __pcim_intx(pdev, enable);
> +               res->orig_intx = !enable;
> +               pci_write_config_word(pdev, PCI_COMMAND, new);
> +       }
>  
>         return 0;
>  }

Thanks for the report and the fix.

Please let me take a look into this. Might take a day.


Regards,
P.


> 
>