Re: [PATCH 1/8] fwctl: Add basic structure for a class subsystem with a cdev

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jun 03, 2024 at 12:53:17PM -0300, 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.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  MAINTAINERS            |   8 ++
>  drivers/Kconfig        |   2 +
>  drivers/Makefile       |   1 +
>  drivers/fwctl/Kconfig  |   9 +++
>  drivers/fwctl/Makefile |   4 +
>  drivers/fwctl/main.c   | 174 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/fwctl.h  |  68 ++++++++++++++++
>  7 files changed, 266 insertions(+)
>  create mode 100644 drivers/fwctl/Kconfig
>  create mode 100644 drivers/fwctl/Makefile
>  create mode 100644 drivers/fwctl/main.c
>  create mode 100644 include/linux/fwctl.h

<...>

> +static struct fwctl_device *
> +_alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size)
> +{
> +	struct fwctl_device *fwctl __free(kfree) = kzalloc(size, GFP_KERNEL);
> +
> +	if (!fwctl)
> +		return NULL;

<...>

> +/* Drivers use the fwctl_alloc_device() wrapper */
> +struct fwctl_device *_fwctl_alloc_device(struct device *parent,
> +					 const struct fwctl_ops *ops,
> +					 size_t size)
> +{
> +	struct fwctl_device *fwctl __free(fwctl) =
> +		_alloc_device(parent, ops, size);

I'm not a big fan of cleanup.h pattern as it hides important to me
information about memory object lifetime and by "solving" one class of
problems it creates another one.

You didn't check if fwctl is NULL before using it.

> +	int devnum;
> +
> +	devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL);
> +	if (devnum < 0)
> +		return NULL;
> +	fwctl->dev.devt = fwctl_dev + devnum;
> +
> +	cdev_init(&fwctl->cdev, &fwctl_fops);
> +	fwctl->cdev.owner = THIS_MODULE;
> +
> +	if (dev_set_name(&fwctl->dev, "fwctl%d", fwctl->dev.devt - fwctl_dev))

Did you miss ida_free() here?

> +		return NULL;
> +
> +	fwctl->ops = ops;
> +	return_ptr(fwctl);
> +}
> +EXPORT_SYMBOL_NS_GPL(_fwctl_alloc_device, FWCTL);

Thanks