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

[Niklas Schnelle]

On Mon, 2026-06-22 at 13:49 -0700, Farhan Ali wrote:
> On 6/22/2026 1:29 PM, Thomas Gleixner wrote:
> > On Mon, Jun 22 2026 at 10:18, 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
> > > restored state of device may not have the Memory bit enabled, even if 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 and restore
> > > it afterwards.
> > > 
> > > Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> > > ---
> > > 
--- 8< ---
> > > @@ -882,6 +883,8 @@ void __pci_restore_msix_state(struct pci_dev *dev)
> > >   	pci_intx_for_msi(dev, 0);
> > >   	pci_msix_clear_and_set_ctrl(dev, 0,
> > >   				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
> > > +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > > +	pci_write_config_word(dev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
> > Can we please have a comment there which explains this? Three month down
> > the road this will results in head scratching otherwise.
> > 
> > I agree with Niklas that this wants a Fixes and a Cc:stable tag.
> > 
> > Other than that:
> > 
> > Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxx>
> > 
> > Thanks,
> > 
> >          tglx
> > 
> I can add a comment, how about something like below?
> 
> "The restored device state may not have Memory decoding enabled in 
> Command register. Since the MSI-X was enabled for the device, enable 
> Memory decoding before restoring MSI-X"

Missing "the" before "Command register" other than that this sounds
good to me and I agree with Thomas that a comment makes sense here.

> 
> For the Fixes tag, do you have a suggestion for a commit? This behavior 
> has been present since 41017f0cac92 ("[PATCH] PCI: MSI(X) save/restore 
> for suspend/resume" which introduced these restore functions. So should 
> be Fixes against 41017f0cac92?

I'm not sure but my guess would be that for suspend/resume this isn't
an issue since the suspend part would save the state with Memory Space
enabled. So it wouldn't be broken in this original use-case. I think
the problem only occurs in case restore is done for error recovery.
Might it make sense to have the Fixes tag point to a2f1e22390ac
("PCI/ERR: Ensure error recoverability at all times") even though that
only helps to expose the issue?

Thanks,
Niklas