RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias

[Parav Pandit <parav@mellanox.com>]

> -----Original Message-----
> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Tuesday, August 27, 2019 4:54 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 1/4] mdev: Introduce sha1 based mdev alias
> 
> On Tue, 27 Aug 2019 11:12:23 +0000
> Parav Pandit <parav@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Cornelia Huck <cohuck@redhat.com>
> > > Sent: Tuesday, August 27, 2019 3:54 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 1/4] mdev: Introduce sha1 based mdev alias
> > >
> > > On Mon, 26 Aug 2019 15:41:16 -0500
> > > Parav Pandit <parav@mellanox.com> wrote:
> > >
> > > > Whenever a parent requests to generate mdev alias, generate a mdev
> > > > alias.
> > > > It is an optional attribute that parent can request to generate
> > > > for each of its child mdev.
> > > > mdev alias is generated using sha1 from the mdev name.
> > >
> > > Maybe add some motivation here as well?
> > >
> > > "Some vendor drivers want an identifier for an mdev device that is
> > > shorter than the uuid, due to length restrictions in the consumers of that
> identifier.
> > >
> > > Add a callback that allows a vendor driver to request an alias of a
> > > specified length to be generated (via sha1) for an mdev device. If
> > > generated, that alias is checked for collisions."
> > >
> > I did described the motivation in the cover letter with example and this
> design discussion thread.
> 
> Yes, but adding it to the patch description makes it available in the git history.
> 
Ok.

> > I will include above summary in v1.
> >
> > > What about:
> > >
> > > * @get_alias_length: optional callback to specify length of the alias to
> create
> > > *                    Returns unsigned integer: length of the alias to be created,
> > > *                                              0 to not create an alias
> > >
> > Ack.
> >
> > > I also think it might be beneficial to add a device parameter here
> > > now (rather than later); that seems to be something that makes sense.
> > >
> > Without showing the use, it shouldn't be added.
> 
> It just feels like an omission: Why should the vendor driver only be able to
> return one value here, without knowing which device it is for?
> If a driver supports different devices, it may have different requirements for
> them.
> 
Sure. Lets first have this requirement to add it.
I am against adding this length field itself without an actual vendor use case, which is adding some complexity in code today.
But it was ok to have length field instead of bool.

Lets not further add "no-requirement futuristic knobs" which hasn't shown its need yet.
When a vendor driver needs it, there is nothing prevents such addition.

> >
> > > >   * Parent device that support mediated device should be
> > > > registered with
> > > mdev
> > > >   * module with mdev_parent_ops structure.
> > > >   **/
> > > > @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> > > >  	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> > > >  			 unsigned long arg);
> > > >  	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
> > > *vma);
> > > > +	unsigned int (*get_alias_length)(void);
> > > >  };
> > > >
> > > >  /* interface for exporting mdev supported type attributes */
> >