linux-kernel - Re: [PATCH v2 4/4] iio: Add Qualcomm Sensor Manager 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250719181915.499d5c4d@jic23-huawei>
Date: Sat, 19 Jul 2025 18:19:15 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Yassine Oudjana <y.oudjana@...tonmail.com>
Cc: Yassine Oudjana via B4 Relay
 <devnull+y.oudjana.protonmail.com@...nel.org>, Manivannan Sadhasivam
 <mani@...nel.org>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Bjorn Andersson
 <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Masahiro
 Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>,
 Nicolas Schier <nicolas.schier@...ux.dev>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, Luca Weiss <luca@...aweiss.eu>,
 linux-arm-msm@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
 linux-iio@...r.kernel.org
Subject: Re: [PATCH v2 4/4] iio: Add Qualcomm Sensor Manager driver

> > > +static int qcom_smgr_iio_read_raw(struct iio_dev *iio_dev,
> > > + struct iio_chan_spec const *chan, int *val,
> > > + int *val2, long mask)
> > > +{
> > > + struct qcom_smgr_iio_priv *priv = iio_priv(iio_dev);
> > > +
> > > + switch (mask) {  
> > 
> > 
> > No sysfs access at all to data is unusual but not completely unheard of.  
> 
> There is no (known) method to request a single reading from the QMI
> service. The only known way to get sensor data is to send a buffering
> request to initiate sending data, then the remoteproc sends QMI
> indications at a regular interval carrying sensor data which I am
> pushing to the IIO buffers. The only way to implement direct sysfs
> access would be to store the last received value somewhere then pass
> it to sysfs when requested. This will also require enabling buffering
> if disabled at the time of reading, then waiting until new data is
> received. I didn't like this solution so I skipped direct sysfs access
> altogether. Buffer access is enough for the current use case with
> iio-sensor-proxy in userspace.

This is absolutely fine.   I have mulled in the past implementing core
code to deal with cases where we are in buffered mode but want to still
provide sysfs access.  That applies for cases like ADCs where a couple
of channels are used for a touchscreen but where there is a hardware
restriction on accessing other channels on a oneshot basis whilst streaming
data on the others.  Maybe one day we'll have that support and it will
also help here, but it's not a high priority thing.

> > > +static const struct iio_chan_spec qcom_smgr_pressure_iio_channels[] = {
> > > + {
> > > + .type = IIO_PRESSURE,
> > > + .scan_index = 0,
> > > + .scan_type = {
> > > + .sign = 'u',
> > > + .realbits = 32,
> > > + .storagebits = 32,
> > > + .endianness = IIO_LE,
> > > + },
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) |
> > > + BIT(IIO_CHAN_INFO_SAMP_FREQ)
> > > + },
> > > + {
> > > + .type = IIO_TIMESTAMP,
> > > + .channel = -1,
> > > + .scan_index = 3,  
> > 
> > 
> > Why 3?  
> 
> Because the same struct is used for this and 3-axis sensors, so we should
> skip the unused values.

I'm not sure how that is related to this value.  These are effectively monotonic
but shouldn't be used to index anything driver side.  So there is nothing
wrong with the value 3, it's just a bit odd.

> 
> >   
> > > + .scan_type = {
> > > + .sign = 'u',
> > > + .realbits = 32,  
> > 
> > 
> > If it's realbits 32 and no shift, why not store it in a 32 bit value?
> > I assume this is a hardware provided timestamp rather than typical software
> > filled in one? Anyhow, I'm not immediately spotting it being used yet
> > so for now perhaps best to drop the channel descriptions.  
> 
> The hardware (or firmware rather) passes an unsigned 32-bit timestamp
> value in a 64-bit QMI field. I was previously passing it as-is to IIO
> but now since I introduced a new struct I can make it 32-bit storagebits.
> 
> But below you said s64 for timestamp so which is it going to be?

I wasn't sure if it was a software or hardware timestamp. Given it's coming
from the QMI thing it's 'hardware' so 32 bit is correct here.

> 
> > > + {
> > > + .service = SNS_SMGR_QMI_SVC_ID,
> > > + / Found on MSM8996 and SDM660 */
> > > + .instance = QRTR_INSTANCE_CONST(1, 50)
> > > + },
> > > + { },  
> > 
> > 
> > No comma on a terminating entry like this.  
> 
> Ok. Gotta keep track of all the conventions used in different subsystems.
I'm curious - have you ever had anyone request the comma?

I know some don't care, but it seems like an odd thing to insist on.

> 
> >   
> > > +};
> > > +MODULE_DEVICE_TABLE(qrtr, qcom_smgr_qrtr_match);  

Jonathan