Re: [PATCH rdma-next 04/14] RDMA/core: Allow to override device op

[Kamal Heib <kamalheib1@gmail.com>]

On Tue, May 26, 2020 at 08:53:42AM +0300, Leon Romanovsky wrote:
> On Tue, May 26, 2020 at 02:21:25AM +0300, Kamal Heib wrote:
> > On Mon, May 25, 2020 at 11:26:41AM -0300, Jason Gunthorpe wrote:
> > > On Wed, May 13, 2020 at 12:50:24PM +0300, Leon Romanovsky wrote:
> > > > From: Maor Gottlieb <maorg@mellanox.com>
> > > >
> > > > Current device ops implementation allows only two stages "set"/"not set"
> > > > and requires caller to check if function pointer exists before
> > > > calling it.
> > > >
> > > > In order to simplify this repetitive task, let's give an option to
> > > > overwrite those pointers. This will allow us to set dummy functions
> > > > for the specific function pointers.
> > > >
> > > > Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > > ---
> > > >  drivers/infiniband/core/device.c | 9 ++++-----
> > > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > > index d9f565a779df..9486e60b42cc 100644
> > > > --- a/drivers/infiniband/core/device.c
> > > > +++ b/drivers/infiniband/core/device.c
> > > > @@ -2542,11 +2542,10 @@ EXPORT_SYMBOL(ib_get_net_dev_by_params);
> > > >  void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
> > > >  {
> > > >  	struct ib_device_ops *dev_ops = &dev->ops;
> > > > -#define SET_DEVICE_OP(ptr, name)                                               \
> > > > -	do {                                                                   \
> > > > -		if (ops->name)                                                 \
> > > > -			if (!((ptr)->name))				       \
> > > > -				(ptr)->name = ops->name;                       \
> > > > +#define SET_DEVICE_OP(ptr, name)					\
> > > > +	do {								\
> > > > +		if (ops->name)						\
> > > > +			(ptr)->name = ops->name;			\
> > > >  	} while (0)
> > >
> > > Did you carefully check every driver to be sure it is OK with this?
> > >
> > > Maybe Kamal remembers why it was like this?
> > >
> > > Jason
> >
> > The idea was to set a specific op only once by the provider when there
> > is a valid function for the op, this was done to make sure that if
> > the op isn't supported by the provider then it will be set to NULL.
> 
> This is not changed.
>

Well, This is changed in patch #6 as dummy functions are now allowed...,
Instead of setting NULL when the op isn't supported by the provider.

I'm wondering why allow only fill_res_entry() and fill_stat_entry() to
be a dummy function as there is multiple ops that can be dummy too,
and why this has to be in this patch set instead in a seperiate patch
set for all the ops that can be dummy? 

Thanks,
Kamal

> >
> > I think it will be more cleaner from the provider point of view to
> > see which ops are supported or not supported in the provider code. by
> > overriding the ops in the core this will make things more confusing.
> 
> Actually the patch does quite opposite, set defaults by IB/core and
> overwrite it by the provider later and not vice versa. The IB/core
> won't overwrite defined by the provider ops. From provider point of view
> no change.
> 
> Thanks
> 
> >
> > Thanks,
> > Kamal