On Thu, Jan 31, 2019 at 10:27:51AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 31, 2019 at 11:48:06AM +0200, Leon Romanovsky wrote:
> > > > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> > > > index 92d11c35221a..c81e43d043cf 100644
> > > > +++ b/include/rdma/restrack.h
> > > > @@ -161,4 +161,26 @@ struct xarray *rdma_dev_to_xa(struct ib_device *dev,
> > > > enum rdma_restrack_type type);
> > > > void rdma_rt_read_lock(struct ib_device *dev, enum rdma_restrack_type type);
> > > > void rdma_rt_read_unlock(struct ib_device *dev, enum rdma_restrack_type type);
> > > > +
> > > > +/**
> > > > + * rdma_res_to_id() - Unique ID as seen by restrack
> > > > + * @res: resrouce to operate
> > > > + *
> > > > + * Return: unique ID
> > > > + */
> > > > +static inline u32 rdma_res_to_id(struct rdma_restrack_entry *res)
> > > > +{
> > > > + return res->id;
> > > > +}
> > > > +/**
> > > > + * rdma_rt_set_type() - Set restrack entry type,
> > > > + * prior to call rdma_restrack_add()
> > > > + * @res: resrouce to operate
> > > > + * @type: Actual type
> > > > + */
> > > > +static inline void rdma_rt_set_type(struct rdma_restrack_entry *res,
> > > > + enum rdma_restrack_type type)
> > > > +{
> > > > + res->type = type;
> > > > +}
> > >
> > > This seems a bit obfuscating, generally kernel code tries to avoid
> > > this sort of trivial accessor style, I thought
> >
> > It is not really correct, kernel code distinguishes between driver code
> > and core code. Obfuscation is not allowed in drivers, but fine for core
> > core which needs to hide implementation from their users.
>
> Do you ever intend to change these implementations?
I have some ideas how to remove those types completely, but it won't be
for now and it maybe won't work at all.
>
> > Such difference is due to need to refactor. You will need to refactor
> > core code in one place, but changes in drivers in many places.
>
> Generally I see this as being defered until a need to change actually
> arises, then you switch the open coded struct access to function and
> then rework the acessors
>
> This sort of wrapper pattern, as well as the
> rdma_rt_read_lock/rdma_rt_read_unlock wrappering don't seem like good
> kernel design.. This isn't Java.
I don't like such pattern too, but it removes some complexity from the
users. Care to send patch to convert xa_lock() back to spinlock?
>
> > If you are interested to feel the pain of obfuscated driver,
> > you are invited to do any major changes in bnxt_re driver.
>
> Well, that drivers is *full* of obfuscating layers, like this.
>
> If there was some *reason* to have the function be more than an
> assignment then maybe, but I don't see it..
>
> Jason
Attachment:
signature.asc
Description: PGP signature
