Re: REGRESSION with pcim_intx()

[Damien Le Moal <dlemoal@kernel.org>]

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;
 }


-- 
Damien Le Moal
Western Digital Research