On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@xxxxxxxx> 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@xxxxxxxxxxxx>
> > Signed-off-by: Selvin Xavier <selvin.xavier@xxxxxxxxxxxx>
> > 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.
>
> > + 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.
>
> Jason
