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

[Kamal Heib <kamalheib1@gmail.com>]

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.

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.

Thanks,
Kamal