Re: [PATCH] PCI: Update saved Max Payload Size in pcie_set_mps()

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Oct 05, 2026 at 02:41:55PM +0200, Niklas Cassel wrote:
> pcie_set_mps() changes the Max Payload Size (MPS) in the Device Control
> register but leaves the copy saved by pci_save_state() alone, so the next
> pci_restore_state() puts back the old value.
> 
> The PCI core does change the MPS of devices whose state has already been
> saved: pci_configure_mps() reduces a Root Port's MPS when a device with a
> smaller MPS Supported is hot-added directly below it (commit 9f0e89359775
> ("PCI: Match Root Port's MPS to endpoint's MPSS as necessary")), and with
> "pci=pcie_bus_safe", pcie_bus_configure_settings() can do the same when
> pciehp calls it after a hot-add.  The Root Port's state was saved when it
> was added and again when portdrv probed it.
> 
> Since commit 3fc686d550f6 ("PCI/ERR: Add support for resetting the Root
> Ports in a platform-specific way"), pcibios_reset_secondary_bus() restores
> that saved state, without saving it first, after host->reset_root_port()
> has reset the Root Port, e.g. during AER recovery or on Link Down with the
> qcom and Rockchip DWC drivers.  The Root Port then goes back to the old,
> larger MPS while the device below it is restored to the smaller one, so
> the Root Port may send TLPs that the device treats as Malformed.
> 
> Update the saved copy of Device Control when pcie_set_mps() changes the
> MPS, like commit 909f7bf9b080 ("PCI: Update saved_config_space upon
> resource assignment") does for BARs.
> 
> Fixes: 3fc686d550f6 ("PCI/ERR: Add support for resetting the Root Ports in a platform-specific way")
> Assisted-by: LLM
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Applied to pci/mps for v7.4, thanks!

> ---
> This fix was originally sent out as part of my RFC series:
> https://lore.kernel.org/linux-pci/20260930145017.1356088-6-cassel@kernel.org/
> 
> But sending this fix as a separate patch, as the fix is really unrelated
> to the rest of the series, so it could be picked up independently.
> 
>  drivers/pci/pci.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8cc6a89130b5..f3b82eb7bfaf 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1736,6 +1736,24 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>  	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
>  }
>  
> +/*
> + * Update the saved copy of the Device Control register after changing it, so
> + * that pci_restore_state() doesn't put back a stale value.  Device Control is
> + * the first register saved by pci_save_pcie_state().
> + */
> +static void pcie_update_saved_devctl(struct pci_dev *dev, u16 clear, u16 set)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u16 *devctl;
> +
> +	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> +	if (!save_state)
> +		return;
> +
> +	devctl = (u16 *)&save_state->cap.data[0];
> +	*devctl = (*devctl & ~clear) | set;
> +}
> +
>  static int pci_save_pcix_state(struct pci_dev *dev)
>  {
>  	int pos;
> @@ -5983,8 +6001,12 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
>  
>  	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
>  						  PCI_EXP_DEVCTL_PAYLOAD, v);
> +	if (ret)
> +		return pcibios_err_to_errno(ret);
>  
> -	return pcibios_err_to_errno(ret);
> +	pcie_update_saved_devctl(dev, PCI_EXP_DEVCTL_PAYLOAD, v);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(pcie_set_mps);
>  
> -- 
> 2.55.0
>