REGRESSION with pcim_intx()

REGRESSION with pcim_intx()

[Damien Le Moal]

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 ?

-- 
Damien Le Moal
Western Digital Research

Re: REGRESSION with pcim_intx()

[Damien Le Moal]

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

Re: REGRESSION with pcim_intx()

[Philipp Stanner]

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.


> 
>