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

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

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