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

[Niklas Schnelle]

On Mon, 2026-06-15 at 11:35 -0700, Farhan Ali wrote:
> The current MSI-X restoration path assumes the Command register Memory bit
> is enabled when writing MSI-X messages. But its possible the last saved and
                                              ^ it's

> restored state of device may not have the Memory bit enabled, even if a
                   ^a

> device driver later enables Memory bit and MSI-X. Attempting to access
> Memory space without Memory bit enabled can lead to Unsupported Request
> (UR) from the device. Fix this by enabling Memory bit if we write MSI-X
> messages, and restore it afterwards.

Don't we have the same issue in __pci_restore_msi_state()?

> 
> Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> ---
>  drivers/pci/msi/msi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 81d24a270a79..d8d3c8a911ac 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -874,6 +874,7 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  {
>  	struct msi_desc *entry;
>  	bool write_msg;
> +	u16 cmd;
>  
>  	if (!dev->msix_enabled)
>  		return;
> @@ -884,6 +885,11 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
>  
>  	write_msg = arch_restore_msi_irqs(dev);
> +	if (write_msg) {
> +		pci_read_config_word(dev, PCI_COMMAND, &cmd);

Sashiko complains that the read cmd may be all 0xFF…F in case of
transient errors. Though I don't see how we'd handle that here without
a larger refactor.

> +		pci_write_config_word(dev, PCI_COMMAND,
> +				      cmd | PCI_COMMAND_MEMORY);
> +	}
>  
>  	scoped_guard (msi_descs_lock, &dev->dev) {
>  		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
> @@ -893,6 +899,9 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  		}
>  	}
>  
> +	if (write_msg)
> +		pci_write_config_word(dev, PCI_COMMAND, cmd);

I wonder if it may be safer to have the Command register reads/writes
within the msic_descs_lock critical section in case this gets executed
concurrently and so the restore above from one thread could slip
between the Memory bit enable and the beginning of the critical
section.

> +
>  	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>  }
> 
>