On Wed, Feb 19, 2020 at 1:33 AM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> 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@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.
>
> 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
>
Got it now. I missed the fact that the ref is taken in
rdma_fill_sgid_attr for both
create_ah and modify_qp contexts. Thanks for your explanation.
I will respin the patch.
Thanks,
Selvin
> Jason
