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.
>
>
