Message-ID: <mzxammninwmak5ti4c6is4pbdx3xzzziiwbxiwrldjyxgae4ok@ocec24vu4txa>
Date: Thu, 6 Mar 2025 00:21:22 +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>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [RFC PATCH 1/7] drivers: Add motion control subsystem

Hello David,

On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote:
> On Fri, 28 Feb 2025 17:44:27 +0100
> Uwe Kleine-König <u.kleine-koenig@...libre.com> wrote:
> > 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.

I agree that guard() makes it harder for non-trivial functions to spot
the critical section. In my eyes this is outweight by not having to
unlock in all exit paths, but that might be subjective. Annother
downside of guard is that sparse doesn't understand it and reports
unbalanced locking.
 
> > > +	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.

The code that I'm convinced is correct is
https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/

This isn't in mainline because there is some feedback I still have to
address, but I think it might serve as an example anyhow.

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

Half knowledge on my end at best. I would hope that Greg knows some
details (which might even be "no, classes are fine"). I added him to Cc:

> > [...]
> > > +	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'm not sure there is an issue, but when I developed the above mentioned
patch it helped me to test these possible races. Just open the sysfs
file, unbind the device (or unload the module) and only then start
reading (or writing).

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

Note that you don't need reserved fields at the end because in the
ioctl handler you know the size of the passed struct. So if the need to
add members to the struct arise, you can do that by checking for the
size. This is even more flexible because otherwise you can only add
fields that must be 0 when the old behaviour is intended. Most of the
time this is no problem. But only most.
 
> > > +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.

Sad, so a userspace process still has to know some internal things about
the motor it drives. :-\

Best regards
Uwe

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

Powered by blists - more mailing lists