Hi Uwe,
On Fri, 28 Feb 2025 17:44:27 +0100
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> 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
