Re: [PATCH v23 5/5] PCI/MSI: Enable memory decoding before restoring MSI-X messages

On Wed, Aug 05, 2026 at 05:07:58PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Missing error check on PCI_COMMAND read leads to permanent register corruption and unintended device enablement on transient PCI errors.

This does seem like a possible issue, but we do the same thing all
over the place, including in pci_intx_for_msi() just above. I don't
think checking for ~0 here will significantly reduce the chance of
problems.

> --
> 
> --- Patch [5]: [PATCH v23 5/5] PCI/MSI: Enable memory decoding before restoring MSI-X messages ---
> commit 6d8ddd25b8ed47c23845049339a3c29cd4e53440
> Author: Farhan Ali <[email protected]>
> 
> PCI/MSI: Enable memory decoding before restoring MSI-X messages
> 
> This commit temporarily enables the Memory bit in the Command register
> before writing MSI-X messages to prevent Unsupported Request errors.
> It then restores the original Command register state afterwards.
> 
> > diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> > index 209373c92e9e1..79c7e84d314b7 100644
> > --- a/drivers/pci/msi/msi.c
> > +++ b/drivers/pci/msi/msi.c
> 
> [ ... ]
> 
> > @@ -879,6 +880,14 @@ void __pci_restore_msix_state(struct pci_dev *dev)
> >  	pci_msix_clear_and_set_ctrl(dev, 0,
> >  				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
> >  
> > +	/*
> > +	 * The restored device state may not have Memory decoding enabled
> > +	 * in the Command register. Since the MSI-X was enabled for the
> > +	 * device, enable Memory decoding before restoring MSI-X.
> > +	 */
> > +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > +	pci_write_config_word(dev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
> 
> [Severity: High]
> If the device is inaccessible or experiencing a transient error (e.g., an AER
> link reset), can this read fail and set cmd to 0xFFFF?
> 
> Looking at pci_read_config_word(), it uses PCI_SET_ERROR_RESPONSE() on failure:
> 
> int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
> {
>         if (pci_dev_is_disconnected(dev)) {
>                 PCI_SET_ERROR_RESPONSE(val);
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>         ...
> }
> 
> If cmd is set to 0xFFFF, the subsequent pci_write_config_word() will write
> 0xFFFF back to the Command register.
> 
> Does this unintentionally enable all device capabilities, including Bus
> Mastering (which could lead to rogue DMA) and SERR#? 
> 
> Should we check the return value of pci_read_config_word() before writing?
> 
> > +
> >  	write_msg = arch_restore_msi_irqs(dev);
> >  
> >  	scoped_guard (msi_descs_lock, &dev->dev) {
> 
> [ ... ]
> 
> > @@ -889,6 +898,7 @@ void __pci_restore_msix_state(struct pci_dev *dev)
> >  		}
> >  	}
> >  
> > +	pci_write_config_word(dev, PCI_COMMAND, cmd);
> 
> [Severity: High]
> If cmd was populated with the error response 0xFFFF, does this write
> permanently commit the corrupted state back to the device?
> 
> >  	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5