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

On 8/12/2026 3:09 PM, Bjorn Helgaas wrote:
> 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.

Yes, it could impact in other places and I agree checking for ~0 here 
will not help.

Thanks

Farhan

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