linux-kernel - Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250713144214.6ee02f59@jic23-huawei>
Date: Sun, 13 Jul 2025 14:42:14 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@...bosch.com>
Cc: "lars@...afoo.de" <lars@...afoo.de>, "robh@...nel.org"
 <robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>, "dima.fedrau@...il.com"
 <dima.fedrau@...il.com>, "marcelo.schmitt1@...il.com"
 <marcelo.schmitt1@...il.com>, "linux-iio@...r.kernel.org"
 <linux-iio@...r.kernel.org>, "devicetree@...r.kernel.org"
 <devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "Lorenz Christian (ME-SE/EAD2)"
 <Christian.Lorenz3@...bosch.com>, "Frauendorf Ulrike (ME/PJ-SW3)"
 <Ulrike.Frauendorf@...bosch.com>, "Dolde Kai (ME-SE/PAE-A3)"
 <Kai.Dolde@...bosch.com>
Subject: Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

On Wed, 9 Jul 2025 19:38:18 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@...bosch.com> wrote:

> Hi Jonathan,
> 
> "available_scan_masks" works not as expected.  We test it using kernel version v6.16. see the test result inline.
> 
> Best Regards 
> Jianping 
> 
> >> +
> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct smi330_data *data = iio_priv(indio_dev);
> >> +	int ret, chan;
> >> +	int i = 0;
> >> +
> >> +	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data-
> >>buf,
> >> +			       ARRAY_SIZE(smi330_channels));
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
> >> +		iio_for_each_active_channel(indio_dev, chan)
> >> +			data->buf[i++] = data->buf[chan];  
> >
> >If I follow this correctly you are reading all the channels and just copying out the
> >ones you want.  Just let the IIO core do that for you by setting iio_dev-  
> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the whole  
> >buffer every time.  
> 
> For the most frequent use cases, we define available_scan_masks = { SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK, SMI330_GYRO_XYZ_MSK, 0 }; and push the whole buffer every time.
> From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z. Then we enable buffer and expect that only the gyro values and timestamp in iio_buffer. Nevertheless, we have 3 accelerometer values and the timestamp in iio_buffer.

> It seems that the iio core does not take care which channel is enabled,  just copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code still needs to take care and just copy the enabled channel value to buffer.

Look again at how it works.  If you provide ACC_XYZ_MSK, then your driver has to handle it.
available_scan_masks is saying what your driver supports. The driver can check active_scan_mask
to find out what is enabled.  So right option here is only
{ SMI330_ALL_CHAN_MSK, 0, }  In that case the driver never needs to check as there is only
one option.

Then if any subset of channels is enabled the IIO core copy out just the data that
is relevant.


> 
> Another side effect after using available_scan_masks is that the active_scan_masks sometimes does not reflect current channel activation status.
> 
> Is some step missing to properly use available_scan_masks ?  How can a user find out from user space which channel combination is defined in available_scan_masks ?

Why would userspace want to?  Userspace requested a subset of channels
and it gets that subset.  So it if asks for the channels that make up
SMI330_ACC_XYZ_MSK, if available_scan_mask == { SMI330_ALL_CHAN_MSK, 0 } then
the IIO core handling selects SMI330_ALL_CHAN_MSK (smallest available mask that
is superset of what we asked for) and sets active_scan_mask to that.  The driver
follows what active_scan_mask specifies and passes all channel data via
the iio_push_to_buffers*() call. The demux in the IIO core than takes that
'scan' and repacks it so that userspace receives just the data it asked for
formatting exactly as the driver would have done it if you had handled
each channels separately in the driver.

So the aim is that userspace never knows anything about this.  Just set
what channels you want and get that data. 

Jonathan


> 
> >
> >The handling the core code is reasonably sophisticated and will use bulk
> >copying where appropriate.
> >
> >If there is a strong reason to not use that, add a comment here so we don't
> >have anyone 'fix' this code in future.
> >  
> >> +	}
> >> +
> >> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
> >> +pf->timestamp);
> >> +
> >> +out:
> >> +	iio_trigger_notify_done(indio_dev->trig);
> >> +
> >> +	return IRQ_HANDLED;
> >> +}  
> 
>