RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 5:25 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Tue, 27 Aug 2019 11:52:21 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Cornelia Huck
> > > Sent: Tuesday, August 27, 2019 5:05 PM
> > > To: Parav Pandit
> > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> > >
> > > On Tue, 27 Aug 2019 11:07:37 +
> > > Parav Pandit wrote:
> > >
> > > > > -Original Message-
> > > > > From: Cornelia Huck
> > > > > Sent: Tuesday, August 27, 2019 4:17 PM
> > > > > To: Parav Pandit
> > > > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> > > > >
> > > > > On Mon, 26 Aug 2019 15:41:18 -0500 Parav Pandit
> > > > > wrote:
> > >
> > > > > > +static ssize_t alias_show(struct device *device,
> > > > > > + struct device_attribute *attr, char *buf) {
> > > > > > + struct mdev_device *dev = mdev_from_dev(device);
> > > > > > +
> > > > > > + if (!dev->alias)
> > > > > > + return -EOPNOTSUPP;
> > > > >
> > > > > I'm wondering how to make this consumable by userspace in the
> > > > > easiest
> > > way.
> > > > > - As you do now (userspace gets an error when trying to read)?
> > > > > - Returning an empty value (nothing to see here, move along)?
> > > > No. This is confusing, to return empty value, because it says that
> > > > there is an
> > > alias but it is some weird empty string.
> > > > If there is alias, it shows exactly what it is.
> > > > If no alias it returns an error code = unsupported -> inline with
> > > > other widely
> > > used subsystem.
> > > >
> > > > > - Or not creating the attribute at all? That would match what
> > > > > userspace
> > > > > sees on older kernels, so it needs to be able to deal with
> > > > > that
> > > > New sysfs files can appear. Tool cannot say that I was not
> > > > expecting this file
> > > here.
> > > > User space is supposed to work with the file they are off interest.
> > > > Mdev interface has option to specify vendor specific files, though
> > > > in usual
> > > manner it's not recommended.
> > > > So there is no old user space, new kernel issue here.
> > >
> > > I'm not talking about old userspace/new kernel, but new userspace/old
> kernel.
> > > Code that wants to consume this attribute needs to be able to cope
> > > with its absence anyway.
> > >
> > Old kernel doesn't have alias file.
> > If some tool tries to read this file it will fail to open non existing
> > file; open()
> system call is already taking care of it.
>
> Yes, that was exactly my argument?
I misunderstood probably.
I re-read all 3 options you posted.
I do not see any issue in reporting error code similar to other widely used
netdev subsystem, hence propose what is posted in the patch.
Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
On Tue, 27 Aug 2019 11:52:21 +
Parav Pandit wrote:
> > -Original Message-
> > From: Cornelia Huck
> > Sent: Tuesday, August 27, 2019 5:05 PM
> > To: Parav Pandit
> > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org
> > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> >
> > On Tue, 27 Aug 2019 11:07:37 +
> > Parav Pandit wrote:
> >
> > > > -Original Message-
> > > > From: Cornelia Huck
> > > > Sent: Tuesday, August 27, 2019 4:17 PM
> > > > To: Parav Pandit
> > > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> > > >
> > > > On Mon, 26 Aug 2019 15:41:18 -0500
> > > > Parav Pandit wrote:
> >
> > > > > +static ssize_t alias_show(struct device *device,
> > > > > + struct device_attribute *attr, char *buf) {
> > > > > + struct mdev_device *dev = mdev_from_dev(device);
> > > > > +
> > > > > + if (!dev->alias)
> > > > > + return -EOPNOTSUPP;
> > > >
> > > > I'm wondering how to make this consumable by userspace in the easiest
> > way.
> > > > - As you do now (userspace gets an error when trying to read)?
> > > > - Returning an empty value (nothing to see here, move along)?
> > > No. This is confusing, to return empty value, because it says that there
> > > is an
> > alias but it is some weird empty string.
> > > If there is alias, it shows exactly what it is.
> > > If no alias it returns an error code = unsupported -> inline with other
> > > widely
> > used subsystem.
> > >
> > > > - Or not creating the attribute at all? That would match what userspace
> > > > sees on older kernels, so it needs to be able to deal with that
> > > New sysfs files can appear. Tool cannot say that I was not expecting this
> > > file
> > here.
> > > User space is supposed to work with the file they are off interest.
> > > Mdev interface has option to specify vendor specific files, though in
> > > usual
> > manner it's not recommended.
> > > So there is no old user space, new kernel issue here.
> >
> > I'm not talking about old userspace/new kernel, but new userspace/old
> > kernel.
> > Code that wants to consume this attribute needs to be able to cope with its
> > absence anyway.
> >
> Old kernel doesn't have alias file.
> If some tool tries to read this file it will fail to open non existing file;
> open() system call is already taking care of it.
Yes, that was exactly my argument?
RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 5:05 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Tue, 27 Aug 2019 11:07:37 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Cornelia Huck
> > > Sent: Tuesday, August 27, 2019 4:17 PM
> > > To: Parav Pandit
> > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> > >
> > > On Mon, 26 Aug 2019 15:41:18 -0500
> > > Parav Pandit wrote:
>
> > > > +static ssize_t alias_show(struct device *device,
> > > > + struct device_attribute *attr, char *buf) {
> > > > + struct mdev_device *dev = mdev_from_dev(device);
> > > > +
> > > > + if (!dev->alias)
> > > > + return -EOPNOTSUPP;
> > >
> > > I'm wondering how to make this consumable by userspace in the easiest
> way.
> > > - As you do now (userspace gets an error when trying to read)?
> > > - Returning an empty value (nothing to see here, move along)?
> > No. This is confusing, to return empty value, because it says that there is
> > an
> alias but it is some weird empty string.
> > If there is alias, it shows exactly what it is.
> > If no alias it returns an error code = unsupported -> inline with other
> > widely
> used subsystem.
> >
> > > - Or not creating the attribute at all? That would match what userspace
> > > sees on older kernels, so it needs to be able to deal with that
> > New sysfs files can appear. Tool cannot say that I was not expecting this
> > file
> here.
> > User space is supposed to work with the file they are off interest.
> > Mdev interface has option to specify vendor specific files, though in usual
> manner it's not recommended.
> > So there is no old user space, new kernel issue here.
>
> I'm not talking about old userspace/new kernel, but new userspace/old kernel.
> Code that wants to consume this attribute needs to be able to cope with its
> absence anyway.
>
Old kernel doesn't have alias file.
If some tool tries to read this file it will fail to open non existing file;
open() system call is already taking care of it.
Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
On Tue, 27 Aug 2019 11:07:37 +
Parav Pandit wrote:
> > -Original Message-
> > From: Cornelia Huck
> > Sent: Tuesday, August 27, 2019 4:17 PM
> > To: Parav Pandit
> > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org
> > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> >
> > On Mon, 26 Aug 2019 15:41:18 -0500
> > Parav Pandit wrote:
> > > +static ssize_t alias_show(struct device *device,
> > > + struct device_attribute *attr, char *buf) {
> > > + struct mdev_device *dev = mdev_from_dev(device);
> > > +
> > > + if (!dev->alias)
> > > + return -EOPNOTSUPP;
> >
> > I'm wondering how to make this consumable by userspace in the easiest way.
> > - As you do now (userspace gets an error when trying to read)?
> > - Returning an empty value (nothing to see here, move along)?
> No. This is confusing, to return empty value, because it says that there is
> an alias but it is some weird empty string.
> If there is alias, it shows exactly what it is.
> If no alias it returns an error code = unsupported -> inline with other
> widely used subsystem.
>
> > - Or not creating the attribute at all? That would match what userspace
> > sees on older kernels, so it needs to be able to deal with that
> New sysfs files can appear. Tool cannot say that I was not expecting this
> file here.
> User space is supposed to work with the file they are off interest.
> Mdev interface has option to specify vendor specific files, though in usual
> manner it's not recommended.
> So there is no old user space, new kernel issue here.
I'm not talking about old userspace/new kernel, but new userspace/old
kernel. Code that wants to consume this attribute needs to be able to
cope with its absence anyway.
>
> > anyway.
> >
> > > +
> > > + return sprintf(buf, "%s\n", dev->alias); } static
> > > +DEVICE_ATTR_RO(alias);
> > > +
> > > static const struct attribute *mdev_device_attrs[] = {
> > > + _attr_alias.attr,
> > > _attr_remove.attr,
> > > NULL,
> > > };
>
RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 4:17 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Mon, 26 Aug 2019 15:41:18 -0500
> Parav Pandit wrote:
>
> > Expose mdev alias as string in a sysfs tree so that such attribute can
> > be used to generate netdevice name by systemd/udev or can be used to
> > match other kernel objects based on the alias of the mdev.
>
> What about
>
> "Expose the optional alias for an mdev device as a sysfs attribute.
> This way, userspace tools such as udev may make use of the alias, for example
> to create a netdevice name for the mdev."
>
Ok. I will change it.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_sysfs.c | 13 +
>
> I think the documentation should be updated as well.
>
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_sysfs.c
> > b/drivers/vfio/mdev/mdev_sysfs.c index 43afe0e80b76..59f4e3cc5233
> > 100644
> > --- a/drivers/vfio/mdev/mdev_sysfs.c
> > +++ b/drivers/vfio/mdev/mdev_sysfs.c
> > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev,
> > struct device_attribute *attr,
> >
> > static DEVICE_ATTR_WO(remove);
> >
> > +static ssize_t alias_show(struct device *device,
> > + struct device_attribute *attr, char *buf) {
> > + struct mdev_device *dev = mdev_from_dev(device);
> > +
> > + if (!dev->alias)
> > + return -EOPNOTSUPP;
>
> I'm wondering how to make this consumable by userspace in the easiest way.
> - As you do now (userspace gets an error when trying to read)?
> - Returning an empty value (nothing to see here, move along)?
No. This is confusing, to return empty value, because it says that there is an
alias but it is some weird empty string.
If there is alias, it shows exactly what it is.
If no alias it returns an error code = unsupported -> inline with other widely
used subsystem.
> - Or not creating the attribute at all? That would match what userspace
> sees on older kernels, so it needs to be able to deal with that
New sysfs files can appear. Tool cannot say that I was not expecting this file
here.
User space is supposed to work with the file they are off interest.
Mdev interface has option to specify vendor specific files, though in usual
manner it's not recommended.
So there is no old user space, new kernel issue here.
> anyway.
>
> > +
> > + return sprintf(buf, "%s\n", dev->alias); } static
> > +DEVICE_ATTR_RO(alias);
> > +
> > static const struct attribute *mdev_device_attrs[] = {
> > + _attr_alias.attr,
> > _attr_remove.attr,
> > NULL,
> > };
Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
On Mon, 26 Aug 2019 15:41:18 -0500
Parav Pandit wrote:
> Expose mdev alias as string in a sysfs tree so that such attribute can
> be used to generate netdevice name by systemd/udev or can be used to
> match other kernel objects based on the alias of the mdev.
What about
"Expose the optional alias for an mdev device as a sysfs attribute.
This way, userspace tools such as udev may make use of the alias, for
example to create a netdevice name for the mdev."
>
> Signed-off-by: Parav Pandit
> ---
> drivers/vfio/mdev/mdev_sysfs.c | 13 +
I think the documentation should be updated as well.
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index 43afe0e80b76..59f4e3cc5233 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct
> device_attribute *attr,
>
> static DEVICE_ATTR_WO(remove);
>
> +static ssize_t alias_show(struct device *device,
> + struct device_attribute *attr, char *buf)
> +{
> + struct mdev_device *dev = mdev_from_dev(device);
> +
> + if (!dev->alias)
> + return -EOPNOTSUPP;
I'm wondering how to make this consumable by userspace in the easiest way.
- As you do now (userspace gets an error when trying to read)?
- Returning an empty value (nothing to see here, move along)?
- Or not creating the attribute at all? That would match what userspace
sees on older kernels, so it needs to be able to deal with that
anyway.
> +
> + return sprintf(buf, "%s\n", dev->alias);
> +}
> +static DEVICE_ATTR_RO(alias);
> +
> static const struct attribute *mdev_device_attrs[] = {
> + _attr_alias.attr,
> _attr_remove.attr,
> NULL,
> };
RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Alex Williamson
> Sent: Tuesday, August 27, 2019 7:24 AM
> To: Parav Pandit
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Mon, 26 Aug 2019 15:41:18 -0500
> Parav Pandit wrote:
>
> > Expose mdev alias as string in a sysfs tree so that such attribute can
> > be used to generate netdevice name by systemd/udev or can be used to
> > match other kernel objects based on the alias of the mdev.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_sysfs.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_sysfs.c
> > b/drivers/vfio/mdev/mdev_sysfs.c index 43afe0e80b76..59f4e3cc5233
> > 100644
> > --- a/drivers/vfio/mdev/mdev_sysfs.c
> > +++ b/drivers/vfio/mdev/mdev_sysfs.c
> > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev,
> > struct device_attribute *attr,
> >
> > static DEVICE_ATTR_WO(remove);
> >
> > +static ssize_t alias_show(struct device *device,
> > + struct device_attribute *attr, char *buf) {
> > + struct mdev_device *dev = mdev_from_dev(device);
> > +
> > + if (!dev->alias)
> > + return -EOPNOTSUPP;
>
> Wouldn't it be better to not create the alias at all? Thanks,
>
In other subsystem such as netdev sysfs files are always created that returns
either returns EOPNOTSUPP or attribute value.
I guess overhead of create multiple groups or creating individual sysfs files
outweigh the simplify of single group.
I think its ok to keep it simple this way.
> Alex
>
> > +
> > + return sprintf(buf, "%s\n", dev->alias); } static
> > +DEVICE_ATTR_RO(alias);
> > +
> > static const struct attribute *mdev_device_attrs[] = {
> > + _attr_alias.attr,
> > _attr_remove.attr,
> > NULL,
> > };
Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
On Mon, 26 Aug 2019 15:41:18 -0500
Parav Pandit wrote:
> Expose mdev alias as string in a sysfs tree so that such attribute can
> be used to generate netdevice name by systemd/udev or can be used to
> match other kernel objects based on the alias of the mdev.
>
> Signed-off-by: Parav Pandit
> ---
> drivers/vfio/mdev/mdev_sysfs.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index 43afe0e80b76..59f4e3cc5233 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct
> device_attribute *attr,
>
> static DEVICE_ATTR_WO(remove);
>
> +static ssize_t alias_show(struct device *device,
> + struct device_attribute *attr, char *buf)
> +{
> + struct mdev_device *dev = mdev_from_dev(device);
> +
> + if (!dev->alias)
> + return -EOPNOTSUPP;
Wouldn't it be better to not create the alias at all? Thanks,
Alex
> +
> + return sprintf(buf, "%s\n", dev->alias);
> +}
> +static DEVICE_ATTR_RO(alias);
> +
> static const struct attribute *mdev_device_attrs[] = {
> + _attr_alias.attr,
> _attr_remove.attr,
> NULL,
> };
[PATCH 3/4] mdev: Expose mdev alias in sysfs tree
Expose mdev alias as string in a sysfs tree so that such attribute can
be used to generate netdevice name by systemd/udev or can be used to
match other kernel objects based on the alias of the mdev.
Signed-off-by: Parav Pandit
---
drivers/vfio/mdev/mdev_sysfs.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 43afe0e80b76..59f4e3cc5233 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR_WO(remove);
+static ssize_t alias_show(struct device *device,
+ struct device_attribute *attr, char *buf)
+{
+ struct mdev_device *dev = mdev_from_dev(device);
+
+ if (!dev->alias)
+ return -EOPNOTSUPP;
+
+ return sprintf(buf, "%s\n", dev->alias);
+}
+static DEVICE_ATTR_RO(alias);
+
static const struct attribute *mdev_device_attrs[] = {
+ _attr_alias.attr,
_attr_remove.attr,
NULL,
};
--
2.19.2
