Message-ID: <20250305164046.4de5b6ef@erd003.prtnl>
Date: Wed, 5 Mar 2025 16:40:45 +0100
From: David Jander <david@...tonic.nl>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
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


Hi Uwe,

On Fri, 28 Feb 2025 17:44:27 +0100
Uwe Kleine-König <u.kleine-koenig@...libre.com> wrote:

> Hello David,
> 
> just a few highlevel review comments inline.

Thanks...

> On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote:
> [...]
> > +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.

This looks interesting. I didn't know about guard(). Thanks. I see the
benefits, but in some cases it also makes the locked region less clearly
visible. While I agree that guard() in this particular place is nice,
I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard().
Let me know if my assessment of the intended use of guard() is incorrect.

> > +	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, ...);

Hmm... I don't yet really understand what you mean. I will have to study the
involved code a bit more.

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

Ack. This looks indeed unnecessary... I'll try to use non-devres calls here.

> > [...]
> > +
> > +static const struct class motion_class = {
> > +	.name		= "motion",
> > +	.devnode	= motion_devnode,  
> 
> IIRC it's recommended to not create new classes, but a bus.

Interesting. I did some searching, and all I could find was that the chapter
in driver-api/driver-model about classes magically vanished between versions
5.12 and 5.13. Does anyone know where I can find some information about this?
Sorry if I'm being blind...

> [...]
> > +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?

Good question. I suppose you mean that since mdev is devres-managed and
device_create_with_groups() apparently isn't aware of that, so there is no
internal lock somewhere that prevents read() or ioctl() being called while the
devres code is freeing the memory of mdev?

I will try to search for some example code to see how something like this is
handled in other places. I assume I'd need to add a per-mdev lock or use the
big motion_mtx everywhere... which sounds like a performance penalty that
should be avoidable. If you know of a good example to learn from, I'd be
grateful to know.

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

Oh, thanks for pointing that out... I wouldn't have searched for that
information in that particular place tbh. ;-)

I am tempted to add another __u32 reserved3 though instead. Better to have
some leeway if something needs to be added in a backwards-compatible way later.

> > +struct mot_speed_duration {
> > +	__u32 channel;
> > +	speed_raw_t speed;  
> 
> What is the unit here?

Speed doesn't have a fixed unit in this case. Or rather, the unit is
device-dependent. For a motor it could be rotations per second, micro-steps per
second, etc... while for a linear actuator, it could be micrometers per second.

Why no fixed unit? That's because in practice many devices (controllers) have
their inherent base-unit, and it would get overly complicated if one needed to
convert back and forth between that and some universal unit just for the sake
of uniformity, and user-space most certainly expects the same unit as the
hardware device it was initially designed for. So in this case it is a design
decision to make user-space deal with unit-conversion if it is necessary to do
so.

> > +	mot_time_t duration;  
> 
> duration_ns? That makes usage much more ideomatic and there should be no
> doubts what the unit is.

Yes, mot_time_t is defined as nanoseconds, so I'll add the _ns suffix here.

> > +	pos_raw_t distance;  
> 
> What is the unit here?

Again this unit can have different meanings: micrometers, micro-steps,
angle-degrees, etc... so what suffix to use?

> > +	__u32 reserved[3];  
> 
> Again the padding is wrong here.

Will fix. thanks.
 
Best regards,

-- 
David Jander

Powered by blists - more mailing lists