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

[Pasha Tatashin]

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

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.