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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 27 Aug 2019 11:57:07 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > -----Original Message-----
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Tuesday, August 27, 2019 5:11 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:33:54 +0000
> > Parav Pandit <parav@mellanox.com> wrote:
> >   
> > > > -----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
> > > > > >  
> >   
> > > > > > 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.  
> > 
> > Frankly, I do not see how it adds complexity; the other callbacks have device
> > arguments already,  
> Other ioctls such as create, remove, mmap, likely need to access the parent.
> Hence it make sense to have parent pointer in there.
> 
> I am not against complexity, I am just saying, at present there is no use-case. Let have use case and we add it.
> 
> > and the vendor driver is free to ignore it if it does not have
> > a use for it. I'd rather add the argument before a possible future user tries
> > weird hacks to allow multiple values, but I'll leave the decision to the
> > maintainers.  
> Why would a possible future user tries a weird hack?
> If user needs to access parent device, that driver maintainer should ask for it.

I've seen the situation often enough that folks tried to do hacks
instead of enhancing the interface.

Again, let's get a maintainer opinion.