Message-ID: <6c6cqaxmsy7miesel4ghdeiea6nrpe4gti4xf5enfyg4uqro5u@vpmtd2t7gydi>
Date: Fri, 28 Feb 2025 17:44:27 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: David Jander <david@...tonic.nl>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org, 
	Jonathan Corbet <corbet@....net>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org, 
	linux-doc@...r.kernel.org, Nuno Sa <nuno.sa@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [RFC PATCH 1/7] drivers: Add motion control subsystem

Hello David,

just a few highlevel review comments inline.

On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote:
> diff --git a/drivers/motion/motion-core.c b/drivers/motion/motion-core.c
> new file mode 100644
> index 000000000000..2963f1859e8b
> --- /dev/null
> +++ b/drivers/motion/motion-core.c
> @@ -0,0 +1,823 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Motion Control Subsystem - Core
> + *
> + * Copyright (C) 2024 Protonic Holland
> + *                    David Jander <david@...tonic.nl>
> + */
> +
> +#include <asm-generic/bitops/builtin-fls.h>
> +#include <asm-generic/errno-base.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqreturn.h>
> +#include <linux/container_of.h>
> +#include <linux/hrtimer_types.h>
> +#include <linux/gfp_types.h>
> +#include <linux/module.h>
> +
> +#include <linux/fs.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/major.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/kmod.h>
> +#include <linux/motion.h>
> +#include <linux/poll.h>
> +#include <linux/ptrace.h>
> +#include <linux/ktime.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/gpio/consumer.h>
> +
> +#include "motion-core.h"
> +#include "motion-helpers.h"
> +#include <linux/time.h>
> +#include <linux/uaccess.h>
> +#include <linux/string.h>
> +#include <linux/math64.h>
> +#include <linux/mutex.h>
> +#include <linux/math.h>
> +#include <linux/math64.h>

Order all <...> includes over the "..." ones.

> +#define MOTION_PROFILE_VALID BIT(31)
> +
> +static LIST_HEAD(motion_list);
> +static DEFINE_MUTEX(motion_mtx);
> +static int motion_major;
> +static DEFINE_IDA(motion_minors_ida);
> +
> +struct iio_motion_trigger_info {
> +	unsigned int minor;
> +};
> +
> +static int motion_minor_alloc(void)
> +{
> +	int ret;
> +
> +	ret = ida_alloc_range(&motion_minors_ida, 0, MINORMASK, GFP_KERNEL);
> +	return ret;

This could be a one-liner.

> +}
> +
> +static void motion_minor_free(int minor)
> +{
> +	ida_free(&motion_minors_ida, minor);
> +}
> +
> +static int motion_open(struct inode *inode, struct file *file)
> +{
> +	int minor = iminor(inode);
> +	struct motion_device *mdev = NULL, *iter;
> +	int err;
> +
> +	mutex_lock(&motion_mtx);

If you use guard(), error handling gets a bit easier.

> +	list_for_each_entry(iter, &motion_list, list) {
> +		if (iter->minor != minor)
> +			continue;
> +		mdev = iter;
> +		break;
> +	}

This should be easier. If you use a cdev you can just do
container_of(inode->i_cdev, ...);

> +	if (!mdev) {
> +		err = -ENODEV;
> +		goto fail;
> +	}
> +
> +	dev_info(mdev->dev, "MOTION: open %d\n", mdev->minor);

degrade to dev_dbg.

> +	file->private_data = mdev;
> +
> +	if (mdev->ops.device_open)
> +		err = mdev->ops.device_open(mdev);
> +	else
> +		err = 0;
> +fail:
> +	mutex_unlock(&motion_mtx);
> +	return err;
> +}
> +
> +static int motion_release(struct inode *inode, struct file *file)
> +{
> +	struct motion_device *mdev = file->private_data;
> +	int i;
> +
> +	if (mdev->ops.device_release)
> +		mdev->ops.device_release(mdev);
> +
> +	for (i = 0; i < mdev->num_gpios; i++) {
> +		int irq;
> +		struct motion_gpio_input *gpio = &mdev->gpios[i];
> +
> +		if (gpio->function == MOT_INP_FUNC_NONE)
> +			continue;
> +		irq = gpiod_to_irq(gpio->gpio);
> +		devm_free_irq(mdev->dev, irq, gpio);

It seems devm is just overhead here if you release by hand anyhow.

> +		gpio->function = MOT_INP_FUNC_NONE;
> +	}
> +
> +	if (!kfifo_is_empty(&mdev->events))
> +		kfifo_reset(&mdev->events);
> +
> +	/* FIXME: Stop running motions? Probably not... */
> +
> +	return 0;
> +}
> +
> +static ssize_t motion_read(struct file *file, char __user *buffer,
> +			  size_t count, loff_t *ppos)
> +{
> +	struct motion_device *mdev = file->private_data;
> +	unsigned int copied = 0L;
> +	int ret;
> +
> +	if (!mdev->dev)
> +		return -ENODEV;
> +
> +	if (count < sizeof(struct mot_event))
> +		return -EINVAL;
> +
> +	do {
> +		if (kfifo_is_empty(&mdev->events)) {
> +			if (file->f_flags & O_NONBLOCK)
> +				return -EAGAIN;
> +
> +			ret = wait_event_interruptible(mdev->wait,
> +					!kfifo_is_empty(&mdev->events) ||
> +					mdev->dev == NULL);
> +			if (ret)
> +				return ret;
> +			if (mdev->dev == NULL)
> +				return -ENODEV;
> +		}
> +
> +		if (mutex_lock_interruptible(&mdev->read_mutex))
> +			return -ERESTARTSYS;
> +		ret = kfifo_to_user(&mdev->events, buffer, count, &copied);
> +		mutex_unlock(&mdev->read_mutex);
> +
> +		if (ret)
> +			return ret;
> +	} while (!copied);
> +
> +	return copied;
> +}
> +
> +static __poll_t motion_poll(struct file *file, poll_table *wait)
> +{
> +	struct motion_device *mdev = file->private_data;
> +	__poll_t mask = 0;
> +
> +	poll_wait(file, &mdev->wait, wait);
> +	if (!kfifo_is_empty(&mdev->events))
> +		mask = EPOLLIN | EPOLLRDNORM;
> +	dev_info(mdev->dev, "Obtained POLL events: 0x%08x\n", mask);

dev_dbg

> +
> +	return mask;
> +}
> +
> [...]
> +
> +static long motion_start_locked(struct motion_device *mdev, struct mot_start *s)
> +{
> +	long ret = 0L;
> +	mot_time_t conv_duration;
> +
> +	lockdep_assert_held(&mdev->mutex);
> +
> +	if (s->reserved1 || s->reserved2)
> +		return -EINVAL;
> +	if (s->channel >= mdev->capabilities.num_channels)
> +		return -EINVAL;
> +	if ((s->index >= MOT_MAX_PROFILES) || (s->direction > MOT_DIRECTION_RIGHT))
> +		return -EINVAL;
> +	if (!(mdev->profiles[s->index].index & MOTION_PROFILE_VALID))
> +		return -EINVAL;
> +	if (s->when >= MOT_WHEN_NUM_WHENS)
> +		return -EINVAL;
> +	if (s->duration && s->distance)
> +		return -EINVAL;
> +	if (!mdev->ops.motion_distance && !mdev->ops.motion_timed)
> +		return -EOPNOTSUPP;

I would add empty lines between these ifs to improve readability. Maybe
thats subjective though.

> +	if (s->duration) {
> +		if (!mdev->ops.motion_timed)
> +			return -EOPNOTSUPP;
> +		/* FIXME: Implement time to distance conversion? */
> +		return mdev->ops.motion_timed(mdev, s->channel, s->index,
> +				s->direction, s->duration, s->when);
> +	}
> +	if (!mdev->ops.motion_distance) {
> +		ret = motion_distance_to_time(mdev, s->index, s->distance,
> +				&conv_duration);
> +		if (ret)
> +			return ret;
> +		return mdev->ops.motion_timed(mdev, s->channel, s->index,
> +				s->direction, conv_duration, s->when);
> +	}
> +	ret = mdev->ops.motion_distance(mdev, s->channel, s->index,
> +			s->distance, s->when);
> +
> +	return ret;
> +}
> [...]
> +
> +static long motion_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct motion_device *mdev = file->private_data;
> +	void __user *argp = (void __user *)arg;
> +	long ret;
> +
> +	switch (cmd) {
> +	case MOT_IOCTL_APIVER:
> +		force_successful_syscall_return();
> +		return MOT_UAPI_VERSION;

force_successful_syscall_return() is only needed if the return value is
negative but no error.

> +	case MOT_IOCTL_BASIC_RUN: {
> +		struct mot_speed_duration spd;
> +
> +		if (copy_from_user(&spd, argp, sizeof(spd)))
> +			return -EFAULT;
> +		if (!mdev->ops.basic_run)
> +			return -EINVAL;
> [...]
> +
> +static const struct class motion_class = {
> +	.name		= "motion",
> +	.devnode	= motion_devnode,

IIRC it's recommended to not create new classes, but a bus.

> +};
> +
> +static const struct file_operations motion_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= motion_read,
> +	.poll		= motion_poll,
> +	.unlocked_ioctl = motion_ioctl,
> +	.open		= motion_open,
> +	.llseek		= noop_llseek,
> +	.release	= motion_release,
> +};
> +
> +static int motion_of_parse_gpios(struct motion_device *mdev)
> +{
> +	int ngpio, i;
> +
> +	ngpio = gpiod_count(mdev->parent, "motion,input");
> +	if (ngpio < 0) {
> +		if (ngpio == -ENOENT)
> +			return 0;
> +		return ngpio;
> +	}
> +
> +	if (ngpio >= MOT_MAX_INPUTS)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ngpio; i++) {
> +		mdev->gpios[i].gpio = devm_gpiod_get_index(mdev->parent,
> +				"motion,input", i, GPIOD_IN);
> +		if (IS_ERR(mdev->gpios[i].gpio))
> +			return PTR_ERR(mdev->gpios[i].gpio);
> +		mdev->gpios[i].function = MOT_INP_FUNC_NONE;
> +		mdev->gpios[i].chmask = 0;
> +		mdev->gpios[i].index = i;
> +	}
> +
> +	mdev->num_gpios = ngpio;
> +	mdev->capabilities.num_ext_triggers += ngpio;
> +
> +	return 0;
> +}
> +
> +static void motion_trigger_work(struct irq_work *work)
> +{
> +	struct motion_device *mdev = container_of(work, struct motion_device,
> +							iiowork);
> +	iio_trigger_poll(mdev->iiotrig);
> +}
> +
> +/**
> + * motion_register_device - Register a new Motion Device
> + * @mdev: description and handle of the motion device
> + *
> + * Register a new motion device with the motion subsystem core.
> + * It also handles OF parsing of external trigger GPIOs and registers an IIO
> + * trigger device if IIO support is configured.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int motion_register_device(struct motion_device *mdev)
> +{
> +	dev_t devt;
> +	int err = 0;
> +	struct iio_motion_trigger_info *trig_info;
> +
> +	if (!mdev->capabilities.num_channels)
> +		mdev->capabilities.num_channels = 1;
> +	if (mdev->capabilities.features | MOT_FEATURE_PROFILE)
> +		mdev->capabilities.max_profiles = MOT_MAX_PROFILES;
> +	if (!mdev->capabilities.speed_conv_mul)
> +		mdev->capabilities.speed_conv_mul = 1;
> +	if (!mdev->capabilities.speed_conv_div)
> +		mdev->capabilities.speed_conv_div = 1;
> +	if (!mdev->capabilities.accel_conv_mul)
> +		mdev->capabilities.accel_conv_mul = 1;
> +	if (!mdev->capabilities.accel_conv_div)
> +		mdev->capabilities.accel_conv_div = 1;
> +
> +	mutex_init(&mdev->mutex);
> +	mutex_init(&mdev->read_mutex);
> +	INIT_KFIFO(mdev->events);
> +	init_waitqueue_head(&mdev->wait);
> +
> +	err = motion_of_parse_gpios(mdev);
> +	if (err)
> +		return err;
> +
> +	mdev->minor = motion_minor_alloc();
> +
> +	mdev->iiotrig = iio_trigger_alloc(NULL, "mottrig%d", mdev->minor);
> +	if (!mdev->iiotrig) {
> +		err = -ENOMEM;
> +		goto error_free_minor;
> +	}
> +
> +	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
> +	if (!trig_info) {
> +		err = -ENOMEM;
> +		goto error_free_trigger;
> +	}
> +
> +	iio_trigger_set_drvdata(mdev->iiotrig, trig_info);
> +
> +	trig_info->minor = mdev->minor;
> +	err = iio_trigger_register(mdev->iiotrig);
> +	if (err)
> +		goto error_free_trig_info;
> +
> +	mdev->iiowork = IRQ_WORK_INIT_HARD(motion_trigger_work);
> +
> +	INIT_LIST_HEAD(&mdev->list);
> +
> +	mutex_lock(&motion_mtx);
> +
> +	devt = MKDEV(motion_major, mdev->minor);
> +	mdev->dev = device_create_with_groups(&motion_class, mdev->parent,
> +				devt, mdev, mdev->groups, "motion%d", mdev->minor);

What makes sure that mdev doesn't go away while one of the attributes is
accessed?

> +	if (IS_ERR(mdev->dev)) {
> +		dev_err(mdev->parent, "Error creating motion device %d\n",
> +				mdev->minor);
> +		mutex_unlock(&motion_mtx);
> +		goto error_free_trig_info;
> +	}
> +	list_add_tail(&mdev->list, &motion_list);
> +	mutex_unlock(&motion_mtx);
> +
> +	return 0;
> +
> +error_free_trig_info:
> +	kfree(trig_info);
> +error_free_trigger:
> +	iio_trigger_free(mdev->iiotrig);
> +error_free_minor:
> +	motion_minor_free(mdev->minor);
> +	dev_info(mdev->parent, "Registering motion device err=%d\n", err);
> +	return err;
> +}
> +EXPORT_SYMBOL(motion_register_device);
> [...]
> +struct mot_capabilities {
> +	__u32 features;
> +	__u8 type;
> +	__u8 num_channels;
> +	__u8 num_int_triggers;
> +	__u8 num_ext_triggers;
> +	__u8 max_profiles;
> +	__u8 max_vpoints;
> +	__u8 max_apoints;
> +	__u8 reserved1;
> +	__u32 subdiv; /* Position unit sub-divisions, microsteps, etc... */
> +	/*
> +	 * Coefficients for converting to/from controller time <--> seconds.
> +	 * Speed[1/s] = Speed[controller_units] * conv_mul / conv_div
> +	 * Accel[1/s^2] = Accel[controller_units] * conv_mul / conv_div
> +	 */
> +	__u32 speed_conv_mul;
> +	__u32 speed_conv_div;
> +	__u32 accel_conv_mul;
> +	__u32 accel_conv_div;
> +	__u32 reserved2;
> +};

https://docs.kernel.org/gpu/imagination/uapi.html (which has some
generic bits that apply here, too) has: "The overall struct must be
padded to 64-bit alignment." If you drop reserved2 the struct is
properly sized (or I counted wrongly).

> +struct mot_speed_duration {
> +	__u32 channel;
> +	speed_raw_t speed;

What is the unit here?

> +	mot_time_t duration;

duration_ns? That makes usage much more ideomatic and there should be no
doubts what the unit is.

> +	pos_raw_t distance;

What is the unit here?

> +	__u32 reserved[3];

Again the padding is wrong here.

> +};

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists