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 > > <[email protected]> wrote: > > > > > On Fri, 10 Jul 2026 21:26:12 +0000, David Matlack <[email protected]> > > > 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.
