Re: [PATCH for-next v3 1/2] RDMA/core: Add helper function to retrieve driver gid context from gid attr

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Feb 18, 2020 at 10:12:17PM +0530, Selvin Xavier wrote:
> On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote:
> > > Adding a helper function to retrieve the driver gid context
> > > from the gid attr.
> > >
> > > Suggested-by: Jason Gunthorpe <jgg@mellanox.com>
> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > >  drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  include/rdma/ib_cache.h         |  1 +
> > >  2 files changed, 42 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> > > index 17bfedd..1b73a71 100644
> > > +++ b/drivers/infiniband/core/cache.c
> > > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num,
> > >  EXPORT_SYMBOL(rdma_query_gid);
> > >
> > >  /**
> > > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute
> > > + * @attr:            Potinter to the GID attribute
> > > + *
> > > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW
> > > + * context corresponding to SGID attr. It takes reference to the GID
> > > + * attribute and this need to be released by the caller using
> > > + * rdma_put_gid_attr
> > > + *
> > > + * Returns HW context on success or NULL on error
> > > + *
> > > + */
> > > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr)
> > > +{
> > > +     struct ib_gid_table_entry *entry =
> > > +             container_of(attr, struct ib_gid_table_entry, attr);
> > > +     struct ib_device *device = entry->attr.device;
> > > +     u8 port_num = entry->attr.port_num;
> > > +     struct ib_gid_table *table;
> > > +     unsigned long flags;
> > > +     void *context = NULL;
> > > +
> > > +     if (!rdma_is_port_valid(device, port_num))
> > > +             return NULL;
> > > +
> > > +     table = rdma_gid_table(device, port_num);
> > > +     read_lock_irqsave(&table->rwlock, flags);
> > > +
> > > +     if (attr->index < 0 || attr->index >= table->sz ||
> > > +         !is_gid_entry_valid(table->data_vec[attr->index]))
> > > +             goto done;
> >
> > Why all this validation and locking? ib_gid_attrs are only created by
> > the core code..
> Locking and validation was added to avoid any scenario where the  gid
> entry is deleted while we are
> executing this API. I saw similar implementation for
> rdma_read_gid_attr_ndev_rcu symbol.

This is required to deref the ndev as GID_TABLE_ENTRY_PENDING_DEL no
longer has the ndev memory.

However here things are not derefing the ndev, there is no reason to
check this. The driver state attached to a gid entry should always be
valid so long as the pointer is valid. This is the entire point of the
refcounting scheme.

> > > +     get_gid_entry(entry);
> >
> > And why a get? Surely it is invalid to call this function without a
> > get already held?
> Getting the reference to the entry only if the entry is valid after
> all the checks. This is the
> reason for invoking this inside the function, rather than in the
> caller. Added a note in the symbol description
> that the caller needs to release reference using rdma_put_gid_attr,
> once the caller finished using
> the void * pointer returned.

That makes no sense, we already *must* have a get at this point or the
whole thing is really buggy

Jason