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

[Alex Williamson]

On Tue, 27 Aug 2019 15:35:10 +0200
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

> On Tue, 27 Aug 2019 11:57:07 +0000
> Parav Pandit <parav@xxxxxxxxxxxx> wrote:
> 
> > > -----Original Message-----
> > > From: Cornelia Huck <cohuck@xxxxxxxxxx>
> > > Sent: Tuesday, August 27, 2019 5:11 PM
> > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > Cc: alex.williamson@xxxxxxxxxx; Jiri Pirko <jiri@xxxxxxxxxxxx>;
> > > kwankhede@xxxxxxxxxx; davem@xxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> > > 
> > > On Tue, 27 Aug 2019 11:33:54 +0000
> > > Parav Pandit <parav@xxxxxxxxxxxx> wrote:
> > >     
> > > > > -----Original Message-----
> > > > > From: Cornelia Huck <cohuck@xxxxxxxxxx>
> > > > > Sent: Tuesday, August 27, 2019 4:54 PM
> > > > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > Cc: alex.williamson@xxxxxxxxxx; Jiri Pirko <jiri@xxxxxxxxxxxx>;
> > > > > kwankhede@xxxxxxxxxx; davem@xxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> > > > > linux- kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> > > > >
> > > > > On Tue, 27 Aug 2019 11:12:23 +0000
> > > > > Parav Pandit <parav@xxxxxxxxxxxx> wrote:
> > > > >    
> > > > > > > -----Original Message-----
> > > > > > > From: Cornelia Huck <cohuck@xxxxxxxxxx>
> > > > > > > Sent: Tuesday, August 27, 2019 3:54 PM
> > > > > > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > > > Cc: alex.williamson@xxxxxxxxxx; Jiri Pirko <jiri@xxxxxxxxxxxx>;
> > > > > > > kwankhede@xxxxxxxxxx; davem@xxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> > > > > > > linux- kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> > > > > > > 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.

Sure, make someone else have an opinion ;)  I don't have a strong one.
The argument against a dev arg, as I see it, is that it's unused
currently, so why should we try to predict a future use case.  The
argument for, is that we're defining an API between the core and vendor
driver, where our job in defining that API could certainly be seen as
anticipating future use cases so as not to unnecessarily churn the
API.  So do we lean towards a more stable API or do we lean towards
minimalism?

when called form mdev_register_device(), the arg we'd add seems obvious
because we really have nothing more to work with than the parent
device.  But this is only a sanity test and the value there seems
questionable anyway.  If we look to the real use case in
mdev_device_create() then clearly dev stands out as a likely useful
arg, but is the type or kobj also useful?  Would we forfeit the sanity
test to include those?  I don't have a lot of confidence in being able
to predict that, so without an obvious set of args, I'm fine with the
minimalist approach provided.  Thanks,

Alex