On Wed, Feb 01, 2017 at 02:39:02PM +0200, Matan Barak wrote:
> +#define idr_get_xxxx(_type, _write, _handle, _context) ({ \
> + const struct uverbs_obj_idr_type * const type = \
> + &uverbs_type_attrs_##_type; \
> + struct ib_uobject *uobj = type->type.ops->lookup_get(&type->type, \
> + _context, _handle, _write); \
> + \
> + IS_ERR(uobj) ? NULL : uobj->object; })
I think you should follow the usual convention and have a full
suite of accessor helpers in a header. This header could even be in
patch #2
static struct ib_uobject *__uobj_get(const struct uverbs_obj_type *type, bool write, struct ib_ucontext *ucontext, int id)
{
[..]
}
#define uobj_get_read(_type,...) ((struct ib_##_type *)_uobj_get(&uverbs_type_attrs_##_type, false, __VA_ARGS__))
#define uobj_get_write(_type,...) ((struct ib_##_type *)_uobj_get(&uverbs_type_attrs_##_type, false, __VA_ARGS__))
static inline void uobj_put_write(struct ib_uobject *uobj)
{
uobj->type->ops->lookup_put(uobj, true);
}
static inline void uobj_put_read(struct ib_uobject *uobj)
{
uobj->type->ops->lookup_put(uobj, false);
}
static inline struct ib_uobject *__uobj_alloc(const struct uverbs_obj_type *type,struct ib_ucontext *ucontext)
{
return type->alloc_begin(type, ucontext);
}
#define uobj_alloc_begin(_type, ...) ((struct ib_##_type *)_uobj_alloc(&uverbs_type_attrs_##_type, __VA_ARGS__))
and so on
> static struct ib_pd *idr_read_pd(int pd_handle, struct ib_ucontext *context)
> {
> - return idr_read_obj(pd_handle, context, 0);
> + return idr_get_xxxx(pd, false, pd_handle, context);
> }
And you can just totally drop all of these inlines and use
uobj_get_read(pd, pd_handle, context);
At the call site.
> static struct ib_xrcd *idr_read_xrcd(int xrcd_handle, struct ib_ucontext *context,
> struct ib_uobject **uobj)
> {
> - *uobj = idr_read_uobj(xrcd_handle, context, 0);
> - return *uobj ? (*uobj)->object : NULL;
> + *uobj = uverbs_type_attrs_xrcd.type.ops->lookup_get(&uverbs_type_attrs_xrcd.type,
> + context, xrcd_handle,
> + false);
> + return IS_ERR(*uobj) ? NULL : (*uobj)->object;
Why didn't this use idr_get_xxx ? Why is it so weird?
>
> @@ -621,9 +439,11 @@ ssize_t ib_uverbs_dealloc_pd(struct ib_uverbs_file *file,
> if (copy_from_user(&cmd, buf, sizeof cmd))
> return -EFAULT;
>
> - uobj = idr_write_uobj(cmd.pd_handle, file->ucontext);
> - if (!uobj)
> - return -EINVAL;
> + uobj = uverbs_type_attrs_pd.type.ops->lookup_get(&uverbs_type_attrs_pd.type,
> + file->ucontext,
> + cmd.pd_handle, true);
> + if (IS_ERR(uobj))
> + return PTR_ERR(uobj);
> pd = uobj->object;
>
> if (atomic_read(&pd->usecnt)) {
> @@ -636,21 +456,12 @@ ssize_t ib_uverbs_dealloc_pd(struct ib_uverbs_file *file,
> if (ret)
> goto err_put;
>
> - uobj->live = 0;
> - put_uobj_write(uobj);
> -
> - idr_remove_uobj(uobj);
> -
> - mutex_lock(&file->mutex);
> - list_del(&uobj->list);
> - mutex_unlock(&file->mutex);
> -
> - put_uobj(uobj);
> + uobj->type->ops->destroy_commit(uobj);
And here I think you should move the pd->device_dealloc_pd into
destroy_commit by having it call what you called hot_plug, and turn
this simply into:
return uobj_remove_or_put(uobj, RDMA_REMOVE_DESTROY);
static inline int uobj_remove_or_put(struct ib_uobject *uobj, enum rdma_remove_reason why)
{
int rc = uobj->type->ops->remove(uobj, why);
if (!rc)
uverbs_uobject_put(uobj);
/* If we are using a forced destruction mode then the driver is
not permitted to fail. */
WARN_ON(!rc && why != RDMA_REMOVE_DESTROY);
return rc;
}
And generally use this pattern for all the destroy verbs.
I didn't study this patch closely since it will change a lot, but I
think this:
4 files changed, 260 insertions(+), 788 deletions(-)
Pretty much confirms this is the right overall approach.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
