| 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: <CAFXKEHaRupFmFQ9ixTT_3p_XaoorJP=y4asYjw3dSMpxXhbOwQ@mail.gmail.com>
Date: Wed, 11 Jun 2025 17:06:16 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, corbet@....net,
lucas.p.stankus@...il.com, lars@...afoo.de, Michael.Hennerich@...log.com,
bagasdotme@...il.com, linux-iio@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing
On Sun, Jun 8, 2025 at 6:08 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sun, 1 Jun 2025 17:21:35 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Add possibilities to set a threshold for activity sensing. Extend the
> > interrupt handler to process activity interrupts. Provide functions to set
> > the activity threshold and to enable/disable activity sensing. Further add
> > a fake channel for having x, y and z axis anded on the iio channel.
> >
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
>
> Hi Lothar,
>
> My main question from this read through is whether we need to be quite
> so careful on disabling measurement when configuring events. It is rather
> unusual if that is necessary and I'm not sure that's what the datasheet
> is implying with the vague bit of advice.
>
> > static const unsigned long adxl313_scan_masks[] = {
> > @@ -297,6 +331,68 @@ static int adxl313_read_freq_avail(struct iio_dev *indio_dev,
> > }
> > }
> >
> > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > + enum adxl313_activity_type type)
> > +{
> > + unsigned int axis_ctrl;
> > + unsigned int regval;
> > + int axis_en, int_en, ret;
> > +
> > + ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > + if (ret)
> > + return ret;
> > +
> > + /* Check if axis for activity are enabled */
> > + if (type != ADXL313_ACTIVITY)
>
> As below - only one value possible, so don't check it.
>
> > + return 0;
> > +
> > + axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > +
> > + /* The axis are enabled, now check if specific interrupt is enabled */
> > + ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, ®val);
> > + if (ret)
> > + return ret;
> > +
> > + int_en = adxl313_act_int_reg[type] & regval;
> > +
> > + return axis_en && int_en;
> > +}
> > +
> > +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> > + enum adxl313_activity_type type,
> > + bool cmd_en)
> > +{
> > + unsigned int axis_ctrl;
> > + unsigned int threshold;
> > + int ret;
> > +
> > + if (type != ADXL313_ACTIVITY)
>
> As the enum only has one value you can drop this check.
> Obviously it's dropped in next patch anyway but better to never
> introduce it.
>
> > + return 0;
> > +
> > + axis_ctrl = ADXL313_ACT_XYZ_EN;
> > +
> > + ret = adxl313_set_measure_en(data, false);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_assign_bits(data->regmap, ADXL313_REG_ACT_INACT_CTL,
> > + axis_ctrl, cmd_en);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > + adxl313_act_int_reg[type],
> > + cmd_en && threshold);
> > + if (ret)
> > + return ret;
> > +
> > + return adxl313_set_measure_en(data, true);
> > +}
> > +
> > static int adxl313_read_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *chan,
> > int *val, int *val2, long mask)
> > @@ -370,6 +466,113 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
> > }
> > }
>
> > +
> > +static int adxl313_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct adxl313_data *data = iio_priv(indio_dev);
> > + unsigned int act_threshold;
> > + int ret;
> > +
> > + /* Measurement stays enabled, reading from regmap cache */
>
> If it isn't safe to read whilst measurements are in progress (as opposed
> to maybe getting a small variation in timing) then this seems more
> fragile than I'd like (to future code changes for example).
>
> Might need an explicit check on it being cached regcache_reg_cached()
> for example though that is very rarely used which makes me dubious
> about using it here.
>
>
> > +
> > + if (type != IIO_EV_TYPE_MAG)
> > + return -EINVAL;
> > +
> > + if (info != IIO_EV_INFO_VALUE)
> > + return -EINVAL;
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + ret = regmap_read(data->regmap,
> > + adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > + &act_threshold);
> > + if (ret)
> > + return ret;
> > + *val = act_threshold * 15625;
> > + *val2 = MICRO;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct adxl313_data *data = iio_priv(indio_dev);
> > + unsigned int regval;
> > + int ret;
> > +
> > + ret = adxl313_set_measure_en(data, false);
> > + if (ret)
> > + return ret;
> > +
> > + if (type != IIO_EV_TYPE_MAG)
> > + return -EINVAL;
> > +
> > + if (info != IIO_EV_INFO_VALUE)
> > + return -EINVAL;
> > +
> > + /* Scale factor 15.625 mg/LSB */
> > + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + ret = regmap_write(data->regmap,
> > + adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > + regval);
>
> I'm surprised this can only be set with measurement disabled.
> Maybe a spec reference. It's common to tweak event values as events
> come in and we generally don't have to stop data flow whilst we do.
>
> There are a few specific bits where the datasheet suggests updating
> them has unwanted side effects in measurement mode. + there is a general
> suggestion to do configuration before enabling measurement mode.
> I don't see anything saying it is a problem for this register.
>
AFAIK there is no issue, nor a big side effect. Changing config
registers might lead to initially wrong measurements. Just the first
measurements might be wrong. I guess this could be a problem if the
sensor had more features and, say, any kind of threshold for some
event then triggered an event wrongly. In case of the ADXL313, there
should not be such a risk. It's then rather about initial wrong
measurements. (Exception: changing the FIFO modes, where turning to
standby is explicitely recommended).
Unfortunately, I could not recall the exact page in the datasheet, but
it matched pretty much my observation also with the ADXL345. So, I'll
give it a try and remove turning off measurement for the
write_event_config()
> > + if (ret)
> > + return ret;
> > + return adxl313_set_measure_en(data, true);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > {
> > struct adxl313_data *data = iio_priv(indio_dev);
> > @@ -502,19 +705,32 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
> >
> > static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
>
> Ah. This does not also have events. Still it's a mix, so maybe
> adxl313_handle_interrupts() or something like that.
I also could break it up into:
- handle interrupt source register events
- drain fifo watermark samples
?
> > {
> > + s64 ts = iio_get_time_ns(indio_dev);
> > struct adxl313_data *data = iio_priv(indio_dev);
> > int samples;
> > + int ret = -ENOENT;
> > +
> > + if (FIELD_GET(ADXL313_INT_ACTIVITY, int_stat)) {
> > + ret = iio_push_event(indio_dev,
> > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > + IIO_MOD_X_OR_Y_OR_Z,
> > + IIO_EV_TYPE_MAG,
> > + IIO_EV_DIR_RISING),
> > + ts);
> > + if (ret)
> > + return ret;
> > + }
> >
> > if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > samples = adxl313_get_samples(data);
> > if (samples < 0)
> > return samples;
> >
> > - return adxl313_fifo_push(indio_dev, samples);
> > + ret = adxl313_fifo_push(indio_dev, samples);
> > }
> >
> > /* Return error if no event data was pushed to the IIO channel. */
> > - return -ENOENT;
> > + return ret;
> This handling works, but as Andy observed maybe the comment is now confusing
> given ret is mostly not an error. Perhaps put that where ret is declared
> instead, or use a separate mask check at the start to quickly
> error out if no bits that we handle are set.
> > }
Yes. Andy also pointed out here. I already developed a feeling for
"something's smelly" with this code, but cannot really think of a
better approach. Actually it works, and for me it is somehow logical.
Probably there are better ways to solve this situation in a cleaner
way?
Powered by blists - more mailing lists