| Message ID | 1553296835-37522-7-git-send-email-parav@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/mdev: Improve vfio/mdev core module | expand |
On Fri, 2019-03-22 at 18:20 -0500, Parav Pandit wrote: > mdev_remove_sysfs_files() should follow exact mirror sequence of a > create, similar to what is followed in error unwinding path of > mdev_create_sysfs_files(). > > Fixes: 7b96953bc640 ("vfio: Mediated device Core driver") > Signed-off-by: Parav Pandit <parav@mellanox.com> > --- > drivers/vfio/mdev/mdev_sysfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c > index ce5dd21..c782fa9 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -280,7 +280,7 @@ int mdev_create_sysfs_files(struct device *dev, struct > mdev_type *type) > > void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type) > { > + sysfs_remove_files(&dev->kobj, mdev_device_attrs); > sysfs_remove_link(&dev->kobj, "mdev_type"); > sysfs_remove_link(type->devices_kobj, dev_name(dev)); > - sysfs_remove_files(&dev->kobj, mdev_device_attrs); > } I agree with that. Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On Fri, 22 Mar 2019 18:20:33 -0500 Parav Pandit <parav@mellanox.com> wrote: > mdev_remove_sysfs_files() should follow exact mirror sequence of a > create, similar to what is followed in error unwinding path of > mdev_create_sysfs_files(). > > Fixes: 7b96953bc640 ("vfio: Mediated device Core driver") > Signed-off-by: Parav Pandit <parav@mellanox.com> > --- > drivers/vfio/mdev/mdev_sysfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c > index ce5dd21..c782fa9 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -280,7 +280,7 @@ int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type) > > void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type) > { > + sysfs_remove_files(&dev->kobj, mdev_device_attrs); > sysfs_remove_link(&dev->kobj, "mdev_type"); > sysfs_remove_link(type->devices_kobj, dev_name(dev)); > - sysfs_remove_files(&dev->kobj, mdev_device_attrs); > } Ok, I agree this is good practice, but what qualifies a "Fixes:" tag here? The fixes reference is incorrect in any case, 6a62c1dfb5c7 changed the creation ordering and didn't update the remove ordering to match, but I still don't see an actual problem with the remove ordering that necessitates the tag. Please clarify. Thanks, Alex
> -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Monday, March 25, 2019 3:21 PM > To: Parav Pandit <parav@mellanox.com> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > kwankhede@nvidia.com > Subject: Re: [PATCH 6/8] vfio/mdev: Follow correct remove sequence > > On Fri, 22 Mar 2019 18:20:33 -0500 > Parav Pandit <parav@mellanox.com> wrote: > > > mdev_remove_sysfs_files() should follow exact mirror sequence of a > > create, similar to what is followed in error unwinding path of > > mdev_create_sysfs_files(). > > > > Fixes: 7b96953bc640 ("vfio: Mediated device Core driver") > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > --- > > drivers/vfio/mdev/mdev_sysfs.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c > > b/drivers/vfio/mdev/mdev_sysfs.c index ce5dd21..c782fa9 100644 > > --- a/drivers/vfio/mdev/mdev_sysfs.c > > +++ b/drivers/vfio/mdev/mdev_sysfs.c > > @@ -280,7 +280,7 @@ int mdev_create_sysfs_files(struct device *dev, > > struct mdev_type *type) > > > > void mdev_remove_sysfs_files(struct device *dev, struct mdev_type > > *type) { > > + sysfs_remove_files(&dev->kobj, mdev_device_attrs); > > sysfs_remove_link(&dev->kobj, "mdev_type"); > > sysfs_remove_link(type->devices_kobj, dev_name(dev)); > > - sysfs_remove_files(&dev->kobj, mdev_device_attrs); > > } > > Ok, I agree this is good practice, but what qualifies a "Fixes:" tag here? The > fixes reference is incorrect in any case, 6a62c1dfb5c7 changed the creation > ordering and didn't update the remove ordering to match, but I still don't > see an actual problem with the remove ordering that necessitates the tag. > Please clarify. Thanks, > In netdev and rdma subsystem we always follow Fixes tag line whenever there is fix, small or big. So following good practice is better. I will fix the tag number in v1. > Alex
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c index ce5dd21..c782fa9 100644 --- a/drivers/vfio/mdev/mdev_sysfs.c +++ b/drivers/vfio/mdev/mdev_sysfs.c @@ -280,7 +280,7 @@ int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type) void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type) { + sysfs_remove_files(&dev->kobj, mdev_device_attrs); sysfs_remove_link(&dev->kobj, "mdev_type"); sysfs_remove_link(type->devices_kobj, dev_name(dev)); - sysfs_remove_files(&dev->kobj, mdev_device_attrs); }
mdev_remove_sysfs_files() should follow exact mirror sequence of a create, similar to what is followed in error unwinding path of mdev_create_sysfs_files(). Fixes: 7b96953bc640 ("vfio: Mediated device Core driver") Signed-off-by: Parav Pandit <parav@mellanox.com> --- drivers/vfio/mdev/mdev_sysfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)