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