| Message ID | 170249390826.436889.13896090394795622449.stgit@bgt-140510-bm01.eng.stellus.in (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | pci/iov: fix kobject_uevent() ordering in sriov_enable() | expand |
On Wed, Dec 13, 2023 at 06:58:28PM +0000, Jim Harris wrote: > Wait to call kobject_uevent() until all of the associated changes are done, > including updating the num_VFs value. This seems right to me. Can we add a little rationale to the commit log? E.g., something about how num_VFs is visible to userspace via sysfs and we don't want a race between (a) userspace reading num_VFs because of KOBJ_CHANGE and (b) the kernel updating num_VFs? (If that's the actual reason.) If there's a problem report about this, include that reference as well. > Suggested by: Leon Romanovsky <leonro@nvidia.com> > Signed-off-by: Jim Harris <jim.harris@samsung.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 25dbe85c4217..3b768e20c7ab 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -683,8 +683,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 Wed, Dec 13, 2023 at 01:26:52PM -0600, Bjorn Helgaas wrote: > On Wed, Dec 13, 2023 at 06:58:28PM +0000, Jim Harris wrote: > > Wait to call kobject_uevent() until all of the associated changes are done, > > including updating the num_VFs value. > > This seems right to me. Can we add a little rationale to the commit > log? E.g., something about how num_VFs is visible to userspace via > sysfs and we don't want a race between (a) userspace reading num_VFs > because of KOBJ_CHANGE and (b) the kernel updating num_VFs? (If > that's the actual reason.) > > If there's a problem report about this, include that reference as > well. There's a second patch that I haven't pushed yet that has more of the rationale that led to this suggestion from Leon - see thread "Locking between vfio hot-remove..." I'll push the two patches as a series. The second patch will effectively revert 35ff867b7, since Leon felt the reasoning behind that patch was incorrect. I'll tie all of that into the cover letter. -Jim > > > Suggested by: Leon Romanovsky <leonro@nvidia.com> > > Signed-off-by: Jim Harris <jim.harris@samsung.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 25dbe85c4217..3b768e20c7ab 100644 > > --- a/drivers/pci/iov.c > > +++ b/drivers/pci/iov.c > > @@ -683,8 +683,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; > > > >
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 25dbe85c4217..3b768e20c7ab 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -683,8 +683,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;
Wait to call kobject_uevent() until all of the associated changes are done, including updating the num_VFs value. Suggested by: Leon Romanovsky <leonro@nvidia.com> Signed-off-by: Jim Harris <jim.harris@samsung.com> --- drivers/pci/iov.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)