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

["Nuno Sá" <noname.nuno@gmail.com>]

On Sun, 2024-02-04 at 17:52 +0200, andy.shevchenko@gmail.com wrote:
> 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?

Yep.

> 
> ...
> 
> > +#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.)

ack..

> 
> ...
> 
> > +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.

Oh that's right. Good catch!

> 
> > +	/* 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.

Hmm, initially I was using dev() stuff but then it felt we could easily get
unconsistent. We have two devices (supplier and consumer) and I guess we can easily
start to be unconsistent in which device we use as argument so I just went with pr_. 

I would say if the call is done by the supplier we use that one, if it's the
consumer, then the consumer. I can do that but again, not sure if it's the best thing
long run.

> 
> > +		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.

Well, I think you're being a bit pedantic... I do cover the index < 0. But no strong
opinion, so I'll do as you suggest and don't wast your (and my) time with this :)

> 
> 	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);

But in the end, nice you mentioned this because it caught my attention for a bug!
io-backends-names > io-backend-names.

> 		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?

Because you're not really gaining much in using it in here. There's no early return
path or goto in here. As I say below, you can argue about consistency but meh...

> 
> > +	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()?
> 

Don't really see the point. In the end we'll even have the same LOC.

But yeah, the only reason I could think off is consistency but OTOH I think it makes
sense to use these were it makes sense.

- Nuno Sá