| Message ID | 170752273749.1693615.1293677023514112939.stgit@bgt-140510-bm01.eng.stellus.in (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/IOV: sriov_numvfs bug fixes | expand |
On 2/9/24 3:52 PM, Jim Harris wrote: > Wait to call kobject_uevent() until all of the associated changes are done, > including updating the num_VFs value. This avoids any possibility of > userspace responding to the event and getting a stale value (although > probably impossible to actually happen). > > Suggested-by: Leon Romanovsky <leonro@nvidia.com> > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> > Signed-off-by: Jim Harris <jim.harris@samsung.com> > --- It looks fine to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > drivers/pci/iov.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index 0ca20cd518d5..c00b0f822526 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -677,8 +677,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) > if (rc) > goto err_pcibios; > > - kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); > iov->num_VFs = nr_virtfn; > + kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); > > return 0; > >
On Fri, Feb 09, 2024 at 07:22:17PM -0800, Kuppuswamy Sathyanarayanan wrote: > On 2/9/24 3:52 PM, Jim Harris wrote: > > @@ -677,8 +677,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) > > if (rc) > > goto err_pcibios; > > > > - kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); > > iov->num_VFs = nr_virtfn; > > + kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); Since it's accessed unlocked now, I *think* this wants appropriate barriers to ensure the order is observed the same on all CPUs. Something like 'smp_store_release(&iov->num_VFs, nr_virtfn)' for writing it, and use 'smp_load_acquire()' on the read-side.
On Mon, Feb 12, 2024 at 08:17:58AM -0700, Keith Busch wrote: > On Fri, Feb 09, 2024 at 07:22:17PM -0800, Kuppuswamy Sathyanarayanan wrote: > > On 2/9/24 3:52 PM, Jim Harris wrote: > > > @@ -677,8 +677,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) > > > if (rc) > > > goto err_pcibios; > > > > > > - kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); > > > iov->num_VFs = nr_virtfn; > > > + kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); > > Since it's accessed unlocked now, I *think* this wants appropriate > barriers to ensure the order is observed the same on all CPUs. Something > like 'smp_store_release(&iov->num_VFs, nr_virtfn)' for writing it, and > use 'smp_load_acquire()' on the read-side. It is unlocked only for sysfs read. IMHO it is overkill to use stores for this case. Thanks >
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 0ca20cd518d5..c00b0f822526 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -677,8 +677,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) if (rc) goto err_pcibios; - kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); iov->num_VFs = nr_virtfn; + kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE); return 0;