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@xxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxx>
> > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > ---
> > > > 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
