Re: [PATCH v8 5/7] iio: add the IIO backend framework

[andy.shevchenko@gmail.com]

Fri, Feb 02, 2024 at 04:08:36PM +0100, Nuno Sa kirjoitti:
> This is a Framework to handle complex IIO aggregate devices.
> 
> The typical architecture is to have one device as the frontend device which
> can be "linked" against one or multiple backend devices. All the IIO and
> userspace interface is expected to be registers/managed by the frontend
> device which will callback into the backends when needed (to get/set
> some configuration that it does not directly control).
> 
> The basic framework interface is pretty simple:
>  - Backends should register themselves with @devm_iio_backend_register()
>  - Frontend devices should get backends with @devm_iio_backend_get()

...

> + * Copyright (C) 2023 Analog Devices Inc.

2024 as well?

...

> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>

Missing types.h and maybe more. (E.g., IIRC linux/err.h doesn't cover
linux/errno.h for Linux internal error codes, >= 512.)

...

> +int devm_iio_backend_request_buffer(struct device *dev,
> +				    struct iio_backend *back,
> +				    struct iio_dev *indio_dev)
> +{
> +	struct iio_backend_buffer_pair *pair;
> +	struct iio_buffer *buffer;
> +
> +	buffer = iio_backend_ptr_op_call(back, request_buffer, indio_dev);
> +	if (IS_ERR(buffer))
> +		return PTR_ERR(buffer);
> +
> +	pair = devm_kzalloc(dev, sizeof(*pair), GFP_KERNEL);
> +	if (!pair)
> +		return -ENOMEM;

Shouldn't we try memory allocation first? Otherwise seems to me like freeing
buffer is missed here.

> +	/* weak reference should be all what we need */
> +	pair->back = back;
> +	pair->buffer = buffer;
> +
> +	return devm_add_action_or_reset(dev, iio_backend_free_buffer, pair);
> +}

...

> +static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back)
> +{
> +	struct device_link *link;
> +	int ret;
> +
> +	/*
> +	 * Make sure the provider cannot be unloaded before the consumer module.
> +	 * Note that device_links would still guarantee that nothing is
> +	 * accessible (and breaks) but this makes it explicit that the consumer
> +	 * module must be also unloaded.
> +	 */
> +	if (!try_module_get(back->owner)) {
> +		pr_err("%s: Cannot get module reference\n", dev_name(dev));

NIH dev_err(). If you want the prefix, define dev_fmt() (or how is it called?)
as well.

> +		return -ENODEV;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, iio_backend_release, back);
> +	if (ret)
> +		return ret;
> +
> +	link = device_link_add(dev, back->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +	if (!link) {
> +		pr_err("%s: Could not link to supplier(%s)\n", dev_name(dev),
> +		       dev_name(back->dev));

Ditto.

> +		return -EINVAL;
> +	}
> +
> +	pr_debug("%s: Found backend(%s) device\n", dev_name(dev),
> +		 dev_name(back->dev));

Ditto (dev_dbg() here).

> +	return 0;
> +}

...

> +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)

Same comments regarding pr_*() vs. dev_*().

> +	struct fwnode_handle *fwnode;
> +	struct iio_backend *back;

> +	int index = 0, ret;

Wouldn't be better to have it done differently and actually using int is not
okay strictly speaking? It's unsigned in your case.

	unsigned int index;
	int ret;


> +	if (name) {
> +		index = device_property_match_string(dev, "io-backends-names",
> +						     name);
> +		if (index < 0)
> +			return ERR_PTR(index);
> +	}

	if (name) {
		ret = device_property_match_string(dev, "io-backends-names", name);
		if (ret < 0)
			return ERR_PTR(ret);
		index = ret;
	} else {
		index = 0;
	}

> +	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
> +	if (IS_ERR(fwnode)) {
> +		/* not an error if optional */
> +		pr_debug("%s: Cannot get Firmware reference\n", dev_name(dev));
> +		return ERR_CAST(fwnode);
> +	}
> +
> +	guard(mutex)(&iio_back_lock);
> +	list_for_each_entry(back, &iio_back_list, entry) {
> +		if (!device_match_fwnode(back->dev, fwnode))
> +			continue;
> +
> +		fwnode_handle_put(fwnode);
> +		ret = __devm_iio_backend_get(dev, back);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		return back;
> +	}
> +
> +	fwnode_handle_put(fwnode);
> +	return ERR_PTR(-EPROBE_DEFER);
> +}

...

> +static void iio_backend_unregister(void *arg)
> +{
> +	struct iio_backend *back = arg;

No guard() here, why?

> +	mutex_lock(&iio_back_lock);
> +	list_del(&back->entry);
> +	mutex_unlock(&iio_back_lock);
> +}

> +int devm_iio_backend_register(struct device *dev,
> +			      const struct iio_backend_ops *ops, void *priv)

Use dev_err() et al.

...

> +	mutex_lock(&iio_back_lock);
> +	list_add(&back->entry, &iio_back_list);
> +	mutex_unlock(&iio_back_lock);

scoped_guard()?

-- 
With Best Regards,
Andy Shevchenko