RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree

[Parav Pandit <parav@mellanox.com>]

> -----Original Message-----
> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Tuesday, August 27, 2019 4:17 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>;
> kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; netdev@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 <parav@mellanox.com> 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 <parav@mellanox.com>
> > ---
> >  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[] = {
> > +	&dev_attr_alias.attr,
> >  	&dev_attr_remove.attr,
> >  	NULL,
> >  };