Re: [PATCH v7 3/5] iio: core: Introduce IIO software triggers

[Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>]

On 08/11/2015 12:42 AM, Daniel Baluta wrote:
> A software trigger associates an IIO device trigger with a software
> interrupt source (e.g: timer, sysfs). This patch adds the generic
> infrastructure for handling software triggers.
> 
> Software interrupts sources are kept in a iio_trigger_types_list and
> registered separately when the associated kernel module is loaded.
> 
> Software triggers can be created directly from drivers or from user
> space via configfs interface.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Sorry for the delay.

[...]
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index dbf5f9a..31aead3 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_IIO) += industrialio.o
>  industrialio-y := industrialio-core.o industrialio-event.o inkern.o
>  industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> +industrialio-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
>  industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o
>  
>  obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
> index 3edee90..ced7115 100644
> --- a/drivers/iio/industrialio-configfs.c
> +++ b/drivers/iio/industrialio-configfs.c
> @@ -15,6 +15,40 @@
>  #include <linux/slab.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sw_trigger.h>
> +
> +static struct config_group *trigger_make_group(struct config_group *group,
> +					       const char *name)
> +{
> +	struct iio_sw_trigger *t;
> +
> +	t = iio_sw_trigger_create(group->cg_item.ci_name, name);
> +	if (IS_ERR(t))
> +		return ERR_CAST(t);
> +
> +	config_item_set_name(&t->group.cg_item, name);

config_item_set_name() takes a format string, since name is user supplied
this should be

	config_item_set_name(&t->group.cg_item, "%s", name);

> +
> +	return &t->group;
> +}
> +
> +static void trigger_drop_group(struct config_group *group,
> +			       struct config_item *item)
> +{
> +	struct iio_sw_trigger *t = to_iio_sw_trigger(item);
> +
> +	iio_sw_trigger_destroy(t);
> +	config_item_put(item);
> +}

I get compile errors when I build this without software trigger support enabled:

drivers/built-in.o: In function `trigger_drop_group':
/opt/linux/drivers/iio/industrialio-configfs.c:39: undefined reference to
`iio_sw_trigger_destroy'
drivers/built-in.o: In function `trigger_make_group':
/opt/linux/drivers/iio/industrialio-configfs.c:25: undefined reference to
`iio_sw_trigger_create'

Since it is bool ifdefing it out should work. But I wonder if it shouldn't
be the other way around and this should go into the sw-trigger module?

[...]
> diff --git a/drivers/iio/industrialio-sw-trigger.c b/drivers/iio/industrialio-sw-trigger.c
> new file mode 100644
> index 0000000..761418e
> --- /dev/null
> +++ b/drivers/iio/industrialio-sw-trigger.c
> @@ -0,0 +1,119 @@
> +/*
> + * The Industrial I/O core, software trigger functions
> + *
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kmod.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/sw_trigger.h>
> +
> +static LIST_HEAD(iio_trigger_types_list);
> +static DEFINE_MUTEX(iio_trigger_types_lock);
> +
> +static
> +struct iio_sw_trigger_type *__iio_find_sw_trigger_type(const char *name,
> +						       unsigned len)
> +{
> +	struct iio_sw_trigger_type *t = NULL, *iter;

Maybe add a lockdep_assert() here to make it explicit that this requires the
types_lock.

> +
> +	list_for_each_entry(iter, &iio_trigger_types_list, list)
> +		if (!strncmp(iter->name, name, len)) {

What's the reason for doing a prefix match? E.g. if name is "h" this will
return the "hrtimer" trigger.

> +			t = iter;
> +			break;
> +		}
> +
> +	return t;
> +}
> +
> +int iio_register_sw_trigger_type(struct iio_sw_trigger_type *t)
> +{
> +	struct iio_sw_trigger_type *iter;
> +	int ret = 0;
> +
> +	mutex_lock(&iio_trigger_types_lock);
> +	iter = __iio_find_sw_trigger_type(t->name, strlen(t->name));
> +	if (iter) {
> +		ret = -EBUSY;
> +	} else {
> +		list_add_tail(&t->list, &iio_trigger_types_list);
> +		iio_sw_trigger_type_configfs_register(t);
> +	}
> +	mutex_unlock(&iio_trigger_types_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(iio_register_sw_trigger_type);
> +
> +int iio_unregister_sw_trigger_type(struct iio_sw_trigger_type *t)

unregister should be void. There isn't too much that can go wrong here. If
the type is registered it will be removed, if it isn't registered there is
nothing to remove and jobs already done.

The problem with having a return value of unregister function is always
where to propagate the value to. And what to do if unregister fails of one
of many in a sequence of unregister calls in case the module registered
multiple things.


> +{
> +	struct iio_sw_trigger_type *iter;
> +	int ret = 0;
> +
> +	mutex_lock(&iio_trigger_types_lock);
> +	iter = __iio_find_sw_trigger_type(t->name, strlen(t->name));
> +	if (!iter) {
> +		ret = -EINVAL;
> +	} else {
> +		iio_sw_trigger_type_configfs_unregister(t);
> +		list_del(&t->list);
> +	}
> +	mutex_unlock(&iio_trigger_types_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(iio_unregister_sw_trigger_type);
> +
> +static
> +struct iio_sw_trigger_type *iio_get_sw_trigger_type(const char *name)
> +{
> +	struct iio_sw_trigger_type *t;
> +
> +	mutex_lock(&iio_trigger_types_lock);
> +	t = __iio_find_sw_trigger_type(name, strlen(name));
> +	if (t && !try_module_get(t->owner))
> +		t = NULL;
> +	mutex_unlock(&iio_trigger_types_lock);
> +
> +	return t;
> +}
> +
> +struct iio_sw_trigger *iio_sw_trigger_create(const char *type, const char *name)
> +{
> +	struct iio_sw_trigger *t;
> +	struct iio_sw_trigger_type *tt;
> +
> +	tt = iio_get_sw_trigger_type(type);
> +	if (!tt) {
> +		pr_err("Invalid trigger type: %s\n", type);
> +		return ERR_PTR(-EINVAL);
> +	}
> +	t = tt->ops->probe(name);
> +	if (IS_ERR(t))
> +		goto out_module_put;
> +
> +	t->trigger_type = tt;
> +
> +	return t;
> +out_module_put:
> +	module_put(tt->owner);
> +	return t;
> +}
> +EXPORT_SYMBOL(iio_sw_trigger_create);
> +
> +void iio_sw_trigger_destroy(struct iio_sw_trigger *t)
> +{
> +	struct iio_sw_trigger_type *tt = t->trigger_type;
> +
> +	tt->ops->remove(t);
> +	module_put(tt->owner);
> +}
> +EXPORT_SYMBOL(iio_sw_trigger_destroy);