[PATCH] PCI: Update saved Max Read Request Size in pcie_set_readrq()

[PATCH] PCI: Update saved Max Read Request Size in pcie_set_readrq()

[Niklas Cassel]

pcie_set_readrq() changes the Max Read Request Size (MRRS) in the Device
Control register but leaves the copy saved by pci_save_state() alone, so
a pci_restore_state() that is not preceded by pci_save_state() puts back
the old value.

Since commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all
times"), the PCI core saves the state of a device when the device is
added, before a driver is bound.  The MRRS is usually set after that, by
the driver or by an enable fixup, some of which limit the MRRS to what
the hardware can handle, e.g. quirk_brcm_5719_limit_mrrs() and the
keystone and Loongson fixups.  Unless the state is saved again, a
pci_restore_state() after a reset, e.g. in the driver's ->slot_reset()
during error recovery, puts back the MRRS that the device had when it
was added.

Update the saved copy of Device Control when pcie_set_readrq() changes
the MRRS, like pcie_set_mps() does for the Max Payload Size.

Suggested-by: Shawn Lin <shawn.lin@rock-chips.com>
Assisted-by: LLM
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5b126cd342ca..30a583aeea65 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5944,8 +5944,12 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 
 	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
 						  PCI_EXP_DEVCTL_READRQ, v);
+	if (ret)
+		return pcibios_err_to_errno(ret);
 
-	return pcibios_err_to_errno(ret);
+	pcie_update_saved_devctl(dev, PCI_EXP_DEVCTL_READRQ, v);
+
+	return 0;
 }
 EXPORT_SYMBOL(pcie_set_readrq);
 

base-commit: 581cf67cb6f66eabc6bcd7f61dd64b2559270507
-- 
2.56.0

Re: [PATCH] PCI: Update saved Max Read Request Size in pcie_set_readrq()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: Data race in pcie_set_readrq() corrupts hardware and saved state
--

commit 5cff2e9dcd50378ba11b9189a892c4623d8503cd
Author: Niklas Cassel <cassel@kernel.org>

PCI: Update saved Max Read Request Size in pcie_set_readrq()

This commit updates pcie_set_readrq() to also modify the kernel's saved copy
of the Device Control register when changing the Max Read Request Size. This
ensures that pci_restore_state() restores the intended MRRS value after a
device reset or error recovery.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5944,8 +5944,12 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>  
>  	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
>  						  PCI_EXP_DEVCTL_READRQ, v);
> +	if (ret)
> +		return pcibios_err_to_errno(ret);
>  
> -	return pcibios_err_to_errno(ret);
> +	pcie_update_saved_devctl(dev, PCI_EXP_DEVCTL_READRQ, v);

[Severity: Medium]
Can this introduce a data race when called concurrently?

Looking at the newly added pcie_update_saved_devctl(), it performs an
unlocked read-modify-write on the saved state memory:

drivers/pci/pci.c:pcie_update_saved_devctl() {
        ...
        devctl = (u16 *)&save_state->cap.data[0];
        *devctl = (*devctl & ~clear) | set;
}

Both the hardware register write via pcie_capability_clear_and_set_word()
(which drops to the unlocked variant for PCI_EXP_DEVCTL) and this new state
update appear to happen without synchronization.

Concurrent access seems possible via VFIO, which allows parallel lockless
pwrite() calls to the PCI Express capability region. This routes through:

vfio_device_fops_write()
  vfio_pci_core_write()
    vfio_exp_config_write()
      pcie_set_readrq()

Could concurrent user-space writes corrupt both the hardware register and the
cached state memory, causing pci_restore_state() to mistakenly restore
invalid state later?

> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(pcie_set_readrq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013195307.181647-2-cassel@kernel.org?part=1

Re: [PATCH] PCI: Update saved Max Read Request Size in pcie_set_readrq()

[Niklas Cassel]

On Tue, Oct 13, 2026 at 08:05:19PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] pci: Data race in pcie_set_readrq() corrupts hardware and saved state
> --
> 
> commit 5cff2e9dcd50378ba11b9189a892c4623d8503cd
> Author: Niklas Cassel <cassel@kernel.org>
> 
> PCI: Update saved Max Read Request Size in pcie_set_readrq()
> 
> This commit updates pcie_set_readrq() to also modify the kernel's saved copy
> of the Device Control register when changing the Max Read Request Size. This
> ensures that pci_restore_state() restores the intended MRRS value after a
> device reset or error recovery.
> 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5944,8 +5944,12 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> >  
> >  	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> >  						  PCI_EXP_DEVCTL_READRQ, v);
> > +	if (ret)
> > +		return pcibios_err_to_errno(ret);
> >  
> > -	return pcibios_err_to_errno(ret);
> > +	pcie_update_saved_devctl(dev, PCI_EXP_DEVCTL_READRQ, v);
> 
> [Severity: Medium]
> Can this introduce a data race when called concurrently?
> 
> Looking at the newly added pcie_update_saved_devctl(), it performs an
> unlocked read-modify-write on the saved state memory:
> 
> drivers/pci/pci.c:pcie_update_saved_devctl() {
>         ...
>         devctl = (u16 *)&save_state->cap.data[0];
>         *devctl = (*devctl & ~clear) | set;
> }
> 
> Both the hardware register write via pcie_capability_clear_and_set_word()
> (which drops to the unlocked variant for PCI_EXP_DEVCTL) and this new state
> update appear to happen without synchronization.
> 
> Concurrent access seems possible via VFIO, which allows parallel lockless
> pwrite() calls to the PCI Express capability region. This routes through:
> 
> vfio_device_fops_write()
>   vfio_pci_core_write()
>     vfio_exp_config_write()
>       pcie_set_readrq()
> 
> Could concurrent user-space writes corrupt both the hardware register and the
> cached state memory, causing pci_restore_state() to mistakenly restore
> invalid state later?

No.  As far as I can tell, concurrent pcie_set_readrq() calls on the
same device are only possible via VFIO, when user space races config
writes to Device Control of its own device.  That path already does
unlocked read-modify-writes of the same register in
vfio_default_config_write(), and pcie_cap_lock does not cover Device
Control, so that race is not new.

pcie_update_saved_devctl() only modifies the MRRS field of the saved
copy, with an aligned 16-bit store, so the worst case is that the saved
MRRS is the value of the other racing write, which also passed the
checks in pcie_set_readrq().  The saved state cannot become invalid.
Also, vfio-pci never restores the live saved state without first saving
it or loading its own stored copy.

The already queued patch "PCI: Update saved Max Payload Size in
pcie_set_mps()" is not affected: VFIO virtualizes MPS and never calls
pcie_set_mps(), and all other callers run during enumeration or from a
driver's probe().

So I don't think either patch needs additional locking.


Kind regards,
Niklas

Re: [PATCH] PCI: Update saved Max Read Request Size in pcie_set_readrq()

[Shawn Lin]

Hi Niklas

在 2026/10/14 星期三 3:53, Niklas Cassel 写道:
> pcie_set_readrq() changes the Max Read Request Size (MRRS) in the Device
> Control register but leaves the copy saved by pci_save_state() alone, so
> a pci_restore_state() that is not preceded by pci_save_state() puts back
> the old value.
> 
> Since commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all
> times"), the PCI core saves the state of a device when the device is
> added, before a driver is bound.  The MRRS is usually set after that, by
> the driver or by an enable fixup, some of which limit the MRRS to what
> the hardware can handle, e.g. quirk_brcm_5719_limit_mrrs() and the
> keystone and Loongson fixups.  Unless the state is saved again, a
> pci_restore_state() after a reset, e.g. in the driver's ->slot_reset()
> during error recovery, puts back the MRRS that the device had when it
> was added.
> 
> Update the saved copy of Device Control when pcie_set_readrq() changes
> the MRRS, like pcie_set_mps() does for the Max Payload Size.
> 

Looks good to me,

Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

Thanks.

> Suggested-by: Shawn Lin <shawn.lin@rock-chips.com>
> Assisted-by: LLM
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/pci/pci.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 5b126cd342ca..30a583aeea65 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5944,8 +5944,12 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>   
>   	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
>   						  PCI_EXP_DEVCTL_READRQ, v);
> +	if (ret)
> +		return pcibios_err_to_errno(ret);
>   
> -	return pcibios_err_to_errno(ret);
> +	pcie_update_saved_devctl(dev, PCI_EXP_DEVCTL_READRQ, v);
> +
> +	return 0;
>   }
>   EXPORT_SYMBOL(pcie_set_readrq);
>   
> 
> base-commit: 581cf67cb6f66eabc6bcd7f61dd64b2559270507