Re: [RFC ABI V1 4/8] RDMA/core: Add support for custom types

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Thu, Jun 30, 2016 at 04:39:27PM +0300, Matan Barak wrote:
> From: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The new ioctl infrastructure supports driver specific types.
> This is implemented by having a list of uverbs_uobject_type in the
> ib_device. Each element of this list corresponds to a specific type
> and specifies its free function, driver's type_id, etc.
> The order of elements dictates the order of process release.
> The whole type_list should be initialized before any ucontext was
> created.

Do we need to have a linked list? Can we use static const arrays like
so much other stuff in the kernel?


> +int ib_uverbs_uobject_type_add(struct list_head	*head,
> +			       void (*free)(struct uverbs_uobject_type *uobject_type,
> +					    struct ib_uobject *uobject,
> +					    struct ib_ucontext *ucontext),

Don't pass function pointers like this, create a proper 'ops' kind of
structure.

Why is this taking a list_head not a ib_device?

> +void ib_uverbs_uobject_type_remove(struct uverbs_uobject_type
> *uobject_type)

Why would a driver ever have to remove a type?

> +{
> +	/*
> +	 * Allocate a new object type for the vendor, this should be done when a
> +	 * vendor is initialized.

Please stop using the word 'vendor' and scrub it from your patches.

This is 'driver specific'

> +	/* List of object under uverbs_object_type */
> +	struct list_head	idr_list;
> +	struct uverbs_uobject_list *type;	/* ptr to ucontext type */
>  };

I'm unclear why we need a list in the driver and list in the
ucontext.. I would have thought they are almost the same.

What use is tracking the ubojects in a per-type list? Do we ever
iterate that way other than on global destruct?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html