Re: [RFC PATCH 05/14] iio/accel: add driver for apple SMS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iio: accel: macsmc-accel: unindexed event channels cause -EBUSY probe failure
- [Medium] iio: accel: macsmc-accel: missing event config callbacks in macsmc_accel_info
- [Low] iio: accel: macsmc-accel: missing pm_sleep_ptr() wrapper on macsmc_accel_pm_ops
--

commit ee4b509c282b1a026627accf04e0b08185034ca5
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

iio/accel: add driver for apple SMS

This commit adds an IIO accelerometer driver for the Apple Sudden Motion
Sensor (SMS) to report X, Y, and Z axes along with drop, orientation, and
shock events, and registers the macsmc-accel MFD cell in macsmc-core when the
MOCN key is present.

> diff --git a/drivers/iio/accel/macsmc-accel.c b/drivers/iio/accel/macsmc-accel.c
> new file mode 100644
> index 0000000000000..0abf81e7bd334
> --- /dev/null
> +++ b/drivers/iio/accel/macsmc-accel.c

[ ... ]

> +static const struct iio_event_spec macsmc_accel_event_spec = {
> +	.type = IIO_EV_TYPE_CHANGE,
> +	.dir = IIO_EV_DIR_NONE,
> +	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +};
> +
> +static const struct iio_chan_spec macsmc_accel_channels[] = {
> +	{
> +		.type = IIO_ACCEL,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_X,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_Y,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_Z,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.channel = SMS_DROP,
> +		.event_spec = &macsmc_accel_event_spec,
> +		.num_event_specs = 1,
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.channel = SMS_ORIENTATION,
> +		.event_spec = &macsmc_accel_event_spec,
> +		.num_event_specs = 1,
> +	},
> +	{
> +		.type = IIO_ACCEL,
> +		.channel = SMS_SHOCK,
> +		.event_spec = &macsmc_accel_event_spec,
> +		.num_event_specs = 1,
> +	},
> +};

[Severity: High]
Will macsmc_accel_probe() always fail with -EBUSY when registering these three
event channels?

In macsmc_accel_channels[], the SMS_DROP, SMS_ORIENTATION, and SMS_SHOCK
channels set .channel and .event_spec = &macsmc_accel_event_spec (which sets
.mask_separate = BIT(IIO_EV_INFO_ENABLE)), but they leave .indexed as 0.

When devm_iio_device_register() registers the event attributes:

  macsmc_accel_probe()
    -> devm_iio_device_register()
      -> __iio_device_register()
        -> iio_device_register_eventset()
          -> __iio_add_event_config_attrs()
            -> iio_device_add_event_sysfs()
              -> iio_device_add_event()
                -> __iio_add_chan_devattr()
                  -> __iio_device_attr_init()
                    -> __iio_chan_prefix_emit()

drivers/iio/industrialio-core.c:__iio_chan_prefix_emit() only includes
chan->channel in the sysfs attribute name when chan->indexed is set:

	seq_buf_printf(&s, "%s_%s", dir, type);

	if (chan->indexed)
		seq_buf_printf(&s, "%d", chan->channel);

Because chan->indexed is 0 on all three event channels, each one generates the
same separate sysfs attribute name "in_accel_change_en". When the second event
channel (SMS_ORIENTATION) is processed in __iio_add_chan_devattr(), it detects
the duplicate IIO_SEPARATE attribute name in ev_int->dev_attr_list and returns
-EBUSY:

	list_for_each_entry(t, attr_list, l)
		if (strcmp(t->dev_attr.attr.name,
			   iio_attr->dev_attr.attr.name) == 0) {
			if (shared_by == IIO_SEPARATE)
				dev_err(dev, "tried to double register : %s\n",
					t->dev_attr.attr.name);
			ret = -EBUSY;
			goto error_device_attr_deinit;
		}

Should these three event channels set .indexed = 1 (and should
macsmc_accel_event() use IIO_EVENT_CODE() or otherwise match the channel
configuration) so that their sysfs attribute names do not collide?

> +
> +static const struct iio_info macsmc_accel_info = {
> +	.read_raw = macsmc_accel_read_raw
> +};

[Severity: Medium]
Should macsmc_accel_info also provide .read_event_config and
.write_event_config callbacks, or should BIT(IIO_EV_INFO_ENABLE) be omitted
from macsmc_accel_event_spec?

Because macsmc_accel_event_spec sets .mask_separate = BIT(IIO_EV_INFO_ENABLE),
iio_device_add_event() creates sysfs event enable attributes backed by
iio_ev_state_show() and iio_ev_state_store() in
drivers/iio/industrialio-event.c:

	if (!indio_dev->info->read_event_config)
		return -EINVAL;

and:

	if (!indio_dev->info->write_event_config)
		return -EINVAL;

With both callbacks left NULL in macsmc_accel_info, any userspace read or
write on the exported *_change_en sysfs attributes will fail with -EINVAL.

> +
> +static int macsmc_accel_restore(struct device *dev)
> +{
> +	struct macsmc_accel *drv = dev_get_drvdata(dev);
> +
> +	return macsmc_accel_dev_init(drv);
> +}
> +
> +static const struct dev_pm_ops macsmc_accel_pm_ops = {
> +	.restore = macsmc_accel_restore,
> +};

[ ... ]

> +static struct platform_driver macsmc_accel_driver = {
> +	.probe = macsmc_accel_probe,
> +	.driver = {
> +		.name = "macsmc-accel",
> +		.pm = &macsmc_accel_pm_ops,
> +	},
> +};

[Severity: Low]
Could macsmc_accel_pm_ops wrap macsmc_accel_restore with pm_sleep_ptr(), and
macsmc_accel_driver wrap &macsmc_accel_pm_ops with pm_sleep_ptr(), so the PM
callback and ops structure are compiled out when CONFIG_PM_SLEEP is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=5