Re: [PATCH v7 09/12] PCI: liveupdate: Inherit ARI Forwarding Enable on preserved bridges

[David Matlack]

On 2026-07-21 06:17 PM, Pasha Tatashin wrote:
> On 2026-07-20 16:19:00-07:00, David Matlack wrote:
> > On Fri, Jul 17, 2026 at 4:29 PM Pasha Tatashin
> > <pasha.tatashin@xxxxxxxxxx> wrote:
> > 
> > > On Fri, 10 Jul 2026 21:26:12 +0000, David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > >
> > > unsigned int    ari_enabled:1;
> > >
> > > Sashiko asks a valid question, what protects other bits in this word
> > > during modication?  At a very list a comment is needed.
> > 
> > This was my reply to Sashiko, not sure if you saw it:
> > 
> > .  pci_liveupdate_configure_ari() is called from pci_configure_ari()
> > .  which also sets dev->ari_enabled=1 and is pre-existing code.
> > .
> > .  If writing to dev->ari_enabled in this path is indeed unsafe then that
> > .  is a pre-existing bug.
> > 
> > I figured that a comment wouldn't be needed for continuing an
> > established precedent (it's ok to write to ari_enabled during this
> > path).
> 
> Overall, I agree that if there is a bug it is pre-existing. But we 
> should also take opportunities to improve existing code and make it 
> safer. In my opinion, Sashiko raises a valid concern that is not 
> obvious, since there is no comment explaining the access of non-atomic 
> bitfields in the header before the struct pci_dev definition.
> 
> In this path, it is safe because we run inside pci_device_add(), where 
> dev is brand new and has not yet been added to the public devices list.
> 
> However, it would be great to:
> 
> 1. Review the access patterns to ensure there are no concurrent writers 
>    on any other paths.
> 2. Add a brief comment in this patch right before setting ari:
> /* Safe to write without locking; device is not yet publicly visible */

Ack, will do this one in v8.

> 
> or:
> 
> 3. Create a new separate patch that adds a comment before the struct 
> pci_dev definition explaining the bitfield access pattern, and how 
> concurrent writes are avoided.