by Lars-Peter Clausen

[permalink] [raw]

Subject: Re: [PATCH v7 4/5] iio: trigger: Introduce IIO hrtimer based trigger

On 08/11/2015 12:42 AM, Daniel Baluta wrote:
[...]
> +
> +static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char *name)
> +{
> + struct iio_hrtimer_info *trig_info;
> + int ret;
> +
> + trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
> + if (!trig_info)
> + return ERR_PTR(-ENOMEM);
> +
> + trig_info->swt.trigger = iio_trigger_alloc("%s", name);
> + if (!trig_info->swt.trigger) {
> + ret = -ENOMEM;
> + goto err_free_trig_info;
> + }
> +
> + iio_trigger_set_drvdata(trig_info->swt.trigger, trig_info);
> + trig_info->swt.trigger->ops = &iio_hrtimer_trigger_ops;
> + trig_info->swt.trigger->dev.groups = iio_hrtimer_attr_groups;
> +
> + hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + trig_info->timer.function = iio_hrtimer_trig_handler;
> +
> + trig_info->sampling_frequency = HRTIMER_DEFAULT_SAMPLING_FREQUENCY;
> + trig_info->period = ktime_set(0, NSEC_PER_SEC /
> + trig_info->sampling_frequency);
> +
> + ret = iio_trigger_register(trig_info->swt.trigger);

I think it makes sense to move the register and unregister to the swt core.
This is something you'd always want to do as the last step in the probe and
the first step in the remove function.

> + if (ret)
> + goto err_free_trigger;
> +
> + iio_swt_group_init_type_name(&trig_info->swt, name, &iio_hrtimer_type);
> + return &trig_info->swt;
> +err_free_trigger:
> + iio_trigger_free(trig_info->swt.trigger);
> +err_free_trig_info:
> + kfree(trig_info);
> +
> + return ERR_PTR(ret);
> +}
> +
> +static int iio_trig_hrtimer_remove(struct iio_sw_trigger *swt)
> +{
> + struct iio_hrtimer_info *trig_info;
> +
> + trig_info = iio_trigger_get_drvdata(swt->trigger);
> +
> + iio_trigger_unregister(swt->trigger);
> +
> + /* cancel the timer after unreg to make sure no one rearms it */
> + hrtimer_cancel(&trig_info->timer);
> + iio_trigger_free(swt->trigger);
> + kfree(trig_info);
> +
> + return 0;
> +}
> +
> +const struct iio_sw_trigger_ops iio_trig_hrtimer_ops = {

static

> + .probe = iio_trig_hrtimer_probe,
> + .remove = iio_trig_hrtimer_remove,
> +};
> +
> +struct iio_sw_trigger_type iio_trig_hrtimer = {

static

> + .name = "hrtimer",
> + .owner = THIS_MODULE,
> + .ops = &iio_trig_hrtimer_ops,
> +};
> +
> +module_iio_sw_trigger_driver(iio_trig_hrtimer);
> +
> +MODULE_AUTHOR("Marten Svanfeldt <[email protected]>");
> +MODULE_AUTHOR("Daniel Baluta <[email protected]>");
> +MODULE_DESCRIPTION("Periodic hrtimer trigger for the IIO subsystem");
> +MODULE_LICENSE("GPL v2");
>

2015-08-31 16:02:52

by Lars-Peter Clausen

[permalink] [raw]

Subject: Re: [PATCH v7 1/5] configfs: Allow dynamic group (un)registration

[...]
> +int configfs_register_group(struct config_group *parent_group,
> + struct config_group *group)
> +{
> + struct dentry *dentry;
> + int ret;
> +
> + link_group(parent_group, group);

This needs to hold subsys->su_mutex

I think from here to before the return we need to hold the parent dentry
mutex with I_MUTEX_PARENT

> +
> + ret = create_default_group(parent_group, group, &dentry);
> + if (ret == 0)
> + configfs_dir_set_ready(dentry->d_fsdata);

This needs to hold configfs_dirent_lock

> +
> + return ret;
> +}
> +EXPORT_SYMBOL(configfs_register_group);
> +
> +void configfs_unregister_group(struct config_group *group)
> +{
> + struct dentry *dentry = group->cg_item.ci_dentry;
> +

We probably need to do the detach_prep stuff somewhere in here.

I think this needs to hold the parent dentry mutex with I_MUTEX_PARENT from
here to ...

> + mutex_lock(&d_inode(dentry)->i_mutex);
> + configfs_detach_group(&group->cg_item);
> + d_inode(dentry)->i_flags |= S_DEAD;
> + dont_mount(dentry);
> + mutex_unlock(&d_inode(dentry)->i_mutex);
> +
> + d_delete(dentry);

... here

> + dput(dentry);
> + unlink_group(group);

needs subsys->su_mutex

> +}
> +EXPORT_SYMBOL(configfs_unregister_group);
> +
> int configfs_register_subsystem(struct configfs_subsystem *subsys)
> {
> int err;
> diff --git a/include/linux/configfs.h b/include/linux/configfs.h
> index c9e5c57..726980e 100644
> --- a/include/linux/configfs.h
> +++ b/include/linux/configfs.h
> @@ -251,6 +251,10 @@ static inline struct configfs_subsystem *to_configfs_subsystem(struct config_gro
> int configfs_register_subsystem(struct configfs_subsystem *subsys);
> void configfs_unregister_subsystem(struct configfs_subsystem *subsys);
>
> +int configfs_register_group(struct config_group *parent_group,
> + struct config_group *group);
> +void configfs_unregister_group(struct config_group *group);
> +
> /* These functions can sleep and can alloc with GFP_KERNEL */
> /* WARNING: These cannot be called underneath configfs callbacks!! */
> int configfs_depend_item(struct configfs_subsystem *subsys, struct config_item *target);
>