| 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
| ||
|
Message-ID: <fc9af95b-abbf-454c-97e1-b884baa5317c@protonmail.com>
Date: Thu, 10 Apr 2025 12:31:20 +0000
From: Yassine Oudjana <y.oudjana@...tonmail.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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>, Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas.schier@...ux.dev>, Alexander Sverdlin <alexander.sverdlin@...il.com>, Sean Nyekjaer <sean@...nix.com>, Javier Carrasco <javier.carrasco.cruz@...il.com>, Matti Vaittinen <mazziesaccount@...il.com>, Antoniu Miclaus <antoniu.miclaus@...log.com>, Ramona Gradinariu <ramona.gradinariu@...log.com>, "Yo-Jung (Leo) Lin" <0xff07@...il.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Neil Armstrong <neil.armstrong@...aro.org>, Barnabás Czémán <barnabas.czeman@...nlining.org>, Danila Tikhonov
<danila@...xyga.com>, Antoni Pokusinski <apokusinski01@...il.com>, Vasileios Amoiridis <vassilisamir@...il.com>, Petar Stoykov <pd.pstoykov@...il.com>, shuaijie wang <wangshuaijie@...nic.com>, Yasin Lee <yasin.lee.x@...il.com>, "Borislav Petkov (AMD)" <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, Tony Luck <tony.luck@...el.com>, Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>, Ingo Molnar <mingo@...nel.org>, Yassine Oudjana <yassine.oudjana@...il.com>, linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org, linux-arm-msm@...r.kernel.org, netdev@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: Add Qualcomm Sensor Manager drivers
On 06/04/2025 7:29 pm, Jonathan Cameron wrote:
> On Sun, 06 Apr 2025 14:08:03 +0000
> Yassine Oudjana <y.oudjana@...tonmail.com> wrote:
>
>> Add drivers for sensors exposed by the Qualcomm Sensor Manager service,
>> which is provided by SLPI or ADSP on Qualcomm SoCs. Supported sensors
>> include accelerometers, gyroscopes, pressure sensors, proximity sensors
>> and magnetometers.
>>
>> Signed-off-by: Yassine Oudjana <y.oudjana@...tonmail.com>
>> ---
>> MAINTAINERS | 18 +
>> drivers/iio/accel/Kconfig | 10 +
>> drivers/iio/accel/Makefile | 2 +
>> drivers/iio/accel/qcom_smgr_accel.c | 138 ++++
>> drivers/iio/common/Kconfig | 1 +
>> drivers/iio/common/Makefile | 1 +
>> drivers/iio/common/qcom_smgr/Kconfig | 16 +
>> drivers/iio/common/qcom_smgr/Makefile | 8 +
>> drivers/iio/common/qcom_smgr/qcom_smgr.c | 589 ++++++++++++++++
>> drivers/iio/common/qcom_smgr/qmi/Makefile | 3 +
>> drivers/iio/common/qcom_smgr/qmi/sns_smgr.c | 711 ++++++++++++++++++++
>> drivers/iio/common/qcom_smgr/qmi/sns_smgr.h | 163 +++++
>> drivers/iio/gyro/Kconfig | 10 +
>> drivers/iio/gyro/Makefile | 2 +
>> drivers/iio/gyro/qcom_smgr_gyro.c | 138 ++++
>> drivers/iio/magnetometer/Kconfig | 9 +
>> drivers/iio/magnetometer/Makefile | 2 +
>> drivers/iio/magnetometer/qcom_smgr_mag.c | 138 ++++
>> drivers/iio/pressure/Kconfig | 10 +
>> drivers/iio/pressure/Makefile | 1 +
>> drivers/iio/pressure/qcom_smgr_pressure.c | 106 +++
>> drivers/iio/proximity/Kconfig | 10 +
>> drivers/iio/proximity/Makefile | 1 +
>> drivers/iio/proximity/qcom_smgr_prox.c | 106 +++
>> include/linux/iio/common/qcom_smgr.h | 64 ++
>> 25 files changed, 2257 insertions(+)
> Split this up. Common library code first, then
> individual drivers making use of it.
Ack.
>
>
>> create mode 100644 drivers/iio/accel/qcom_smgr_accel.c
>> create mode 100644 drivers/iio/common/qcom_smgr/Kconfig
>> create mode 100644 drivers/iio/common/qcom_smgr/Makefile
>> create mode 100644 drivers/iio/common/qcom_smgr/qcom_smgr.c
>> create mode 100644 drivers/iio/common/qcom_smgr/qmi/Makefile
>> create mode 100644 drivers/iio/common/qcom_smgr/qmi/sns_smgr.c
>> create mode 100644 drivers/iio/common/qcom_smgr/qmi/sns_smgr.h
>> create mode 100644 drivers/iio/gyro/qcom_smgr_gyro.c
>> create mode 100644 drivers/iio/magnetometer/qcom_smgr_mag.c
>> create mode 100644 drivers/iio/pressure/qcom_smgr_pressure.c
>> create mode 100644 drivers/iio/proximity/qcom_smgr_prox.c
>> create mode 100644 include/linux/iio/common/qcom_smgr.h
>
>
>> diff --git a/drivers/iio/accel/qcom_smgr_accel.c b/drivers/iio/accel/qcom_smgr_accel.c
>> new file mode 100644
>> index 000000000000..ce854312d1d9
>> --- /dev/null
>> +++ b/drivers/iio/accel/qcom_smgr_accel.c
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Qualcomm Sensor Manager accelerometer driver
>> + *
>> + * Copyright (c) 2022, Yassine Oudjana <y.oudjana@...tonmail.com>
> Given ongoing work, a range on that date to go up to this year
> probably makes sense!
Yeah
>
>> + */
>> +
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/common/qcom_smgr.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +
>> +static const struct iio_chan_spec qcom_smgr_accel_iio_channels[] = {
>> + {
>> + .type = IIO_ACCEL,
>> + .modified = true,
>> + .channel2 = IIO_MOD_X,
>> + .scan_index = 0,
>> + .scan_type = {
>> + .sign = 's',
>> + .realbits = 32,
>> + .storagebits = 32,
>> + .endianness = IIO_LE,
>> + },
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> + .ext_info = qcom_smgr_iio_ext_info
>> + },
>> + {
>> + .type = IIO_ACCEL,
>> + .modified = true,
>> + .channel2 = IIO_MOD_Y,
>> + .scan_index = 1,
>> + .scan_type = {
>> + .sign = 's',
>> + .realbits = 32,
>> + .storagebits = 32,
>> + .endianness = IIO_LE,
>> + },
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> + .ext_info = qcom_smgr_iio_ext_info
>> + },
>> + {
>> + .type = IIO_ACCEL,
>> + .modified = true,
>> + .channel2 = IIO_MOD_Z,
>> + .scan_index = 2,
>> + .scan_type = {
>> + .sign = 's',
>> + .realbits = 32,
>> + .storagebits = 32,
>> + .endianness = IIO_LE,
>> + },
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> + .ext_info = qcom_smgr_iio_ext_info
>> + },
>> + {
>> + .type = IIO_TIMESTAMP,
>> + .channel = -1,
>> + .scan_index = 3,
>> + .scan_type = {
>> + .sign = 'u',
>> + .realbits = 32,
>> + .storagebits = 64,
>> + .endianness = IIO_LE,
>> + },
>> + },
>> +};
>> +
>> +static int qcom_smgr_accel_probe(struct platform_device *pdev)
>> +{
>> + struct iio_dev *iio_dev;
>> + struct qcom_smgr_iio_priv *priv;
>> + int ret;
>> +
>> + iio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
>> + if (!iio_dev)
>> + return -ENOMEM;
>> +
>> + priv = iio_priv(iio_dev);
>> + priv->sensor = *(struct qcom_smgr_sensor **)pdev->dev.platform_data;
>> + priv->sensor->iio_dev = iio_dev;
>> +
>> + iio_dev->name = "qcom-smgr-accel";
>> + iio_dev->info = &qcom_smgr_iio_info;
>> + iio_dev->channels = qcom_smgr_accel_iio_channels;
>> + iio_dev->num_channels = ARRAY_SIZE(qcom_smgr_accel_iio_channels);
>> +
>> + ret = devm_iio_kfifo_buffer_setup(&pdev->dev, iio_dev,
>> + &qcom_smgr_buffer_ops);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to setup buffer: %pe\n",
>> + ERR_PTR(ret));
> For all error message in probe() use
> return dev_err_probe(&pdev->dev, ERR_PTR(ret), "Failed to setup buffer\n")
> etc.
Ack.
>
>> + return ret;
>> + }
>> +
>> + ret = devm_iio_device_register(&pdev->dev, iio_dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to register IIO device: %pe\n",
>> + ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, priv->sensor);
>> +
>> + return 0;
>> +}
>> +
>> +static void qcom_smgr_accel_remove(struct platform_device *pdev)
>
> I'm surprised to see a platform device here - will read on but I
> doubt that is the way to go. Maybe an auxbus or similar or
> just squashing this all down to be registered directly by
> the parent driver.
I got the idea from cros_ec_sensors which also deals with a similar
sensor hub paradigm.
>
>
>> +{
>> + struct qcom_smgr_sensor *sensor = platform_get_drvdata(pdev);
>> +
>> + sensor->iio_dev = NULL;
>> +}
>> +
>> +static const struct platform_device_id qcom_smgr_accel_ids[] = {
>> + { .name = "qcom-smgr-accel" },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(platform, qcom_smgr_accel_ids);
>> +
>> +static struct platform_driver qcom_smgr_accel_driver = {
>> + .probe = qcom_smgr_accel_probe,
>> + .remove = qcom_smgr_accel_remove,
>> + .driver = {
>> + .name = "qcom_smgr_accel",
>> + },
>> + .id_table = qcom_smgr_accel_ids,
>> +};
>> +module_platform_driver(qcom_smgr_accel_driver);
>> +
>> +MODULE_AUTHOR("Yassine Oudjana <y.oudjana@...tonmail.com>");
>> +MODULE_DESCRIPTION("Qualcomm Sensor Manager accelerometer driver");
>> +MODULE_LICENSE("GPL");
>
>> diff --git a/drivers/iio/common/qcom_smgr/qcom_smgr.c b/drivers/iio/common/qcom_smgr/qcom_smgr.c
>> new file mode 100644
>> index 000000000000..8d46be11d5b6
>> --- /dev/null
>> +++ b/drivers/iio/common/qcom_smgr/qcom_smgr.c
>> @@ -0,0 +1,589 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Qualcomm Sensor Manager core driver
>> + *
>> + * Copyright (c) 2021, Yassine Oudjana <y.oudjana@...tonmail.com>
>
> As above, I'd add a date range to reflect that this is ongoing.
Ack.
>
>> + */
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/common/qcom_smgr.h>
>> +#include <linux/iio/iio.h>
>
> Be consistent with ordering. Above you have iio as a separate block.
> Either option is fine, but not a mixture.
Ack.
>
>> +#include <linux/math64.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
> Unless there are very strong reasons for of specific code please
> use property.h and the generic firmware accessors.
Ack.
>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc/qcom_rproc.h>
>> +#include <linux/soc/qcom/qmi.h>
>> +#include <linux/soc/qcom/qrtr.h>
>> +#include <linux/types.h>
>> +#include <net/sock.h>
>
>
>> +static void qcom_smgr_buffering_report_handler(struct qmi_handle *hdl,
>> + struct sockaddr_qrtr *sq,
>> + struct qmi_txn *txn,
>> + const void *data)
>> +{
>> + struct qcom_smgr *smgr =
>> + container_of(hdl, struct qcom_smgr, sns_smgr_hdl);
>> + struct sns_smgr_buffering_report_ind *ind =
>> + (struct sns_smgr_buffering_report_ind *)data;
>
> Casting away a const isn't a good sign. Why do you need to do that?
> const struct sns_smg_buffer_repor_ind *ind = data;
> should be fine I think.
The casted struct was previously not const so I was only casting from
void *. I made it const lately but didn't notice this cast. Will change it.
>
>
>> + struct qcom_smgr_sensor *sensor;
>> + u8 i;
>> +
>> + for (i = 0; i < smgr->sensor_count; ++i) {
>> + sensor = &smgr->sensors[i];
>> +
>> + /* Find sensor matching report */
>> + if (sensor->id != ind->report_id)
>> + continue;
>> +
>> + if (!sensor->iio_dev)
>> + /* Corresponding driver was unloaded. Ignore remaining reports. */
>> + return;
>> +
>> + /*
>> + * Since we are matching report rate with sample rate, we only
>> + * get a single sample in every report.
>> + */
>> + iio_push_to_buffers_with_timestamp(sensor->iio_dev,
>> + ind->samples[0].values,
>> + ind->metadata.timestamp);
> You are using a 64 bit timestamp writer that doesn't know about the endianness of
> that timestamp. I'd not do this. Just write the timestamp in like any normal
> channel and call iio_push_to_buffers().
Will look into it.
>
>> +
>> + break;
>
> return;
>
>> + }
>> +}
>> +
>> +static const struct qmi_msg_handler qcom_smgr_msg_handlers[] = {
>> + {
>> + .type = QMI_INDICATION,
>> + .msg_id = SNS_SMGR_BUFFERING_REPORT_MSG_ID,
>> + .ei = sns_smgr_buffering_report_ind_ei,
>> + .decoded_size = sizeof(struct sns_smgr_buffering_report_ind),
>> + .fn = qcom_smgr_buffering_report_handler,
>> + },
>> + {}
> { }
>
> given it's in IIO and I get to pick silly formatting rules.
> More seriously I wanted this consistent so picked a choice mostly at random.
No objections on my side.
>
>> +};
>
>> +
>> +static int qcom_smgr_iio_write_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) {
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + priv->sensor->data_types[0].cur_sample_rate = val;
>> +
>> + /*
>> + * Send new SMGR buffering request with updated rates
>> + * if buffer is enabled
>> + */
>> + if (iio_buffer_enabled(iio_dev))
>> + return iio_dev->setup_ops->postenable(iio_dev);
>
> Generally we'd just refuse to set the sampling frequency.
> This is racy as nothing holds the buffer enabled.
> So I'd do
> if (!iio_device_claim_direct(iio_dev);
> return -EBUSY;
>
> priv->sensor->data_types[0].cur_sample_rate = val;
> iio_device_release_diect(iio_dev);
>
> Change sampling frequency when doing buffered capture is really confusing
> anyway for userspace software as it has no way to know when the
> change occurred so just don't bother supporting that.
Makes sense.
>
>> +
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
> Put this in a default in the switch. That makes it clear we don't
> expect to see anything else.
> Same for other similar cases above.
Ack.
>
>> +}
>> +
>> +static int qcom_smgr_iio_read_avail(struct iio_dev *iio_dev,
>> + struct iio_chan_spec const *chan,
>> + const int **vals, int *type, int *length,
>> + long mask)
>> +{
>> + struct qcom_smgr_iio_priv *priv = iio_priv(iio_dev);
>> + const int samp_freq_vals[3] = {
>> + 1, 1, priv->sensor->data_types[0].cur_sample_rate
>> + };
>
> Lifetime of this needs to last beyond the end of this call as some users
> or read_avail hang on to it. Embed the storage in your priv
> structure rather than local in this function.
Ack.
> I'm also a little confused how the maximum comes from something called cur_sample_rate.
This was max_sample_rate previously but at some point while trying to
understand how the sampling rate works on the SMGR side I removed
max_sample_rate and replaced it here with cur_sample_rate without
understanding what I was changing. When I brought back max_sample_rate I
missed this change. Will change it back.
>
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + *type = IIO_VAL_INT;
>> + *vals = samp_freq_vals;
>> + *length = ARRAY_SIZE(samp_freq_vals);
>> + return IIO_AVAIL_RANGE;
>> + }
>> +
>> + return -EINVAL;
>> +}
>
> ...
>
>> +const struct iio_chan_spec_ext_info qcom_smgr_iio_ext_info[] = {
>> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, qcom_smgr_iio_get_mount_matrix),
>> + {}
> trivial but I'm trying to standardize on
> { }
> for this in IIO.
Ack.
>
>> +};
>> +EXPORT_SYMBOL_GPL(qcom_smgr_iio_ext_info);
>> +
>> +static int qcom_smgr_probe(struct qrtr_device *qdev)
>> +{
>> + struct qcom_smgr *smgr;
>> + int i, j;
>> + int ret;
>> +
>> + smgr = devm_kzalloc(&qdev->dev, sizeof(*smgr), GFP_KERNEL);
>> + if (!smgr)
>> + return -ENOMEM;
>> +
>> + smgr->dev = &qdev->dev;
>> +
>> + smgr->sns_smgr_info.sq_family = AF_QIPCRTR;
>> + smgr->sns_smgr_info.sq_node = qdev->node;
>> + smgr->sns_smgr_info.sq_port = qdev->port;
>> +
>> + dev_set_drvdata(&qdev->dev, smgr);
>> +
>> + ret = qmi_handle_init(&smgr->sns_smgr_hdl,
>> + SNS_SMGR_SINGLE_SENSOR_INFO_RESP_MAX_LEN, NULL,
>> + qcom_smgr_msg_handlers);
> On error this handle doesn't seem to be released.
Will fix.
>
>> + if (ret < 0) {
>> + dev_err(smgr->dev,
>> + "Failed to initialize sensor manager handle: %d\n",
>> + ret);
>> + return ret;
>
> return dev_err_probe()
> Same in all other similar cases that are only called from probe.
Ack.
>
>> + }
>> +
>> + ret = qcom_smgr_request_all_sensor_info(smgr, &smgr->sensors);
>> + if (ret < 0) {
>> + dev_err(smgr->dev, "Failed to get available sensors: %pe\n",
>> + ERR_PTR(ret));
>> + return ret;
>> + }
>> + smgr->sensor_count = ret;
>> +
>> + /* Get primary and secondary sensors from each sensor ID */
>> + for (i = 0; i < smgr->sensor_count; i++) {
>> + ret = qcom_smgr_request_single_sensor_info(smgr,
>> + &smgr->sensors[i]);
>> + if (ret < 0) {
>> + dev_err(smgr->dev,
>> + "Failed to get sensors from ID 0x%02x: %pe\n",
>> + smgr->sensors[i].id, ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + for (j = 0; j < smgr->sensors[i].data_type_count; j++) {
>> + /* Default to maximum sample rate */
>> + smgr->sensors[i].data_types->cur_sample_rate =
>> + smgr->sensors[i].data_types->max_sample_rate;
>> +
>> + dev_dbg(smgr->dev, "0x%02x,%d: %s %s\n",
>> + smgr->sensors[i].id, j,
>> + smgr->sensors[i].data_types[j].vendor,
>> + smgr->sensors[i].data_types[j].name);
>> + }
>> +
>> + qcom_smgr_register_sensor(smgr, &smgr->sensors[i]);
> Above I suggest that maybe you should just skip the platform devices and register
> directly with IIO as you find the sensors. So have the struct iio_dev->device
> parent directly off this one.
As I said previously I followed the model used in cros_ec_sensors, and
it made sense to me since I always see platform devices used to
represent firmware-backed devices like this.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void qcom_smgr_remove(struct qrtr_device *qdev)
>> +{
>> + struct qcom_smgr *smgr = dev_get_drvdata(&qdev->dev);
>> +
>> + qmi_handle_release(&smgr->sns_smgr_hdl);
> If that is all you have, use a devm_add_action_or_reset() to
> register a handler and drop this remove entirely.
Ack.
>
>> +}
>> +
>> +static const struct qrtr_device_id qcom_smgr_qrtr_match[] = {
>> + {
>> + .service = SNS_SMGR_QMI_SVC_ID,
>> + .instance = QRTR_INSTANCE(SNS_SMGR_QMI_SVC_V1,
>> + SNS_SMGR_QMI_INS_ID)
>> + },
>> + {},
> { }
>
> for IIO terminating entries like this.
Ack.
>
>> +};
>> +MODULE_DEVICE_TABLE(qrtr, qcom_smgr_qrtr_match);
Powered by blists - more mailing lists