RE: [PATCH 6/8] vfio/mdev: Follow correct remove sequence
> -Original Message-
> From: Alex Williamson
> Sent: Monday, March 25, 2019 3:21 PM
> To: Parav Pandit
> Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org;
> kwankh...@nvidia.com
> Subject: Re: [PATCH 6/8] vfio/mdev: Follow correct remove sequence
>
> On Fri, 22 Mar 2019 18:20:33 -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
> > ---
> > 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
Re: [PATCH 6/8] vfio/mdev: Follow correct remove sequence
On Fri, 22 Mar 2019 18:20:33 -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
> ---
> 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
Re: [PATCH 6/8] vfio/mdev: Follow correct remove sequence
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
> ---
> 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
Best regards,
Maxim Levitsky
[PATCH 6/8] vfio/mdev: Follow correct remove sequence
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
---
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);
}
--
1.8.3.1
