Re: [PATCH v4 01/10] fwctl: Add basic structure for a class subsystem with a cdev

[Dan Williams <dan.j.williams@intel.com>]

Jason Gunthorpe wrote:
> Create the class, character device and functions for a fwctl driver to
> un/register to the subsystem.
> 
> A typical fwctl driver has a sysfs presence like:
> 
> $ ls -l /dev/fwctl/fwctl0
> crw------- 1 root root 250, 0 Apr 25 19:16 /dev/fwctl/fwctl0
> 
> $ ls /sys/class/fwctl/fwctl0
> dev  device  power  subsystem  uevent
> 
> $ ls /sys/class/fwctl/fwctl0/device/infiniband/
> ibp0s10f0
> 
> $ ls /sys/class/infiniband/ibp0s10f0/device/fwctl/
> fwctl0/
> 
> $ ls /sys/devices/pci0000:00/0000:00:0a.0/fwctl/fwctl0
> dev  device  power  subsystem  uevent
> 
> Which allows userspace to link all the multi-subsystem driver components
> together and learn the subsystem specific names for the device's
> components.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
[..]
> +struct fwctl_device *_fwctl_alloc_device(struct device *parent,
> +					 const struct fwctl_ops *ops,
> +					 size_t size);
> +/**
> + * fwctl_alloc_device - Allocate a fwctl
> + * @parent: Physical device that provides the FW interface
> + * @ops: Driver ops to register
> + * @drv_struct: 'struct driver_fwctl' that holds the struct fwctl_device
> + * @member: Name of the struct fwctl_device in @drv_struct
> + *
> + * This allocates and initializes the fwctl_device embedded in the drv_struct.
> + * Upon success the pointer must be freed via fwctl_put(). Returns a 'drv_struct
> + * \*' on success, NULL on error.
> + */
> +#define fwctl_alloc_device(parent, ops, drv_struct, member)               \
> +	({                                                                \
> +		static_assert(__same_type(struct fwctl_device,            \
> +					  ((drv_struct *)NULL)->member)); \
> +		static_assert(offsetof(drv_struct, member) == 0);         \
> +		(drv_struct *)_fwctl_alloc_device(parent, ops,            \
> +						  sizeof(drv_struct));    \
> +	})

I have already suggested someone else copy this approach to context
allocation. What do you think of generalizing this in
include/linux/container_of.h as:

#define container_alloc(core_struct, drv_struct, member, alloc_fn, ...)    \
       ({                                                                 \
               static_assert(__same_type(core_struct,                     \
                                         ((drv_struct *)NULL)->member));  \
               static_assert(offsetof(drv_struct, member) == 0);          \
               (drv_struct *)(alloc_fn)(sizeof(drv_struct), __VA_ARGS__); \
       })

...and then fwctl_alloc_device becomes:

#define fwctl_alloc_device(parent, ops, drv_struct, member) \
	container_alloc(struct fwctl_device, drv_struct, member, \
			_fwctl_alloc_device, parent, ops);

Either way, you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>