| 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: <CAFXKEHYXnN9ddSM3wzgRTCZDu3JiaBJ6n8htQEBCiS52G+QzQQ@mail.gmail.com>
Date: Mon, 14 Apr 2025 15:19:52 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v5 09/11] iio: accel: adxl345: add inactivity feature
On Mon, Mar 31, 2025 at 12:47 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Tue, 18 Mar 2025 23:08:41 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Add the inactivity feature of the sensor. When activity and inactivity
> > are enabled, a link bit will be set linking activity and inactivity
> > handling. Additionally, the auto-sleep mode will be enabled. Due to the
> > link bit the sensor is going to auto-sleep when inactivity was
> > detected.
> >
> > Inactivity detection needs a threshold to be configured, and a time
> > after which it will go into inactivity state if measurements under
> > threshold.
> >
> > When a ODR is configured this time for inactivity is adjusted with a
> > corresponding reasonable default value, in order to have higher
> > frequencies and lower inactivity times, and lower sample frequency but
> > give more time until inactivity. Both with reasonable upper and lower
> > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > need to operate beween 12.5 Hz and 400 Hz. This is a default setting
> > when actively changing sample frequency, explicitly setting the time
> > until inactivity will overwrite the default.
> >
> > Similarly, setting the g-range will provide a default value for the
> > activity and inactivity thresholds. Both are implicit defaults, but
> > equally can be overwritten to be explicitly configured.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
>
> A few comments inline. The magic handling of the value 0 is
> a bit of unexpected ABI.
>
> Jonathan
>
> > @@ -327,6 +358,7 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > bool cmd_en)
> > {
> > bool axis_en, en;
> > + unsigned int inact_time_s;
> > unsigned int threshold;
> > u32 axis_ctrl = 0;
> > int ret;
> > @@ -345,6 +377,20 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > default:
> > return -EINVAL;
> > }
> > + } else {
> > + switch (axis) {
> > + case IIO_MOD_X:
> > + axis_ctrl = ADXL345_INACT_X_EN;
> > + break;
> > + case IIO_MOD_Y:
> > + axis_ctrl = ADXL345_INACT_Y_EN;
> > + break;
> > + case IIO_MOD_Z:
> > + axis_ctrl = ADXL345_INACT_Z_EN;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > }
> >
> > if (cmd_en)
> > @@ -365,11 +411,67 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > if (type == ADXL345_ACTIVITY) {
> > axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) > 0;
> > en = axis_en && threshold > 0;
> > + } else {
>
> So previous suggestion on setting en doesn't work but you can still combine
> the bits other than the type match to simplify code and get rid of axis_en
> in both paths.
>
> > + ret = regmap_read(st->regmap, ADXL345_REG_TIME_INACT, &inact_time_s);
> > + if (ret)
> > + return ret;
> > +
> > + axis_en = FIELD_GET(ADXL345_REG_INACT_AXIS_MSK, axis_ctrl) > 0;
> > + en = axis_en && threshold > 0 && inact_time_s > 0;
> > }
>
> > +/**
> > + * adxl345_set_inact_time_s - Configure inactivity time explicitly or by ODR.
> > + * @st: The sensor state instance.
> > + * @val_s: A desired time value, between 0 and 255.
> > + *
> > + * If val_s is 0, a default inactivity time will be computed. It should take
> > + * power consumption into consideration. Thus it shall be shorter for higher
> > + * frequencies and longer for lower frequencies. Hence, frequencies above 255 Hz
> > + * shall default to 10 s and frequencies below 10 Hz shall result in 255 s to
> > + * detect inactivity.
>
> I'd missed this previously. I've no problem with a default time being set
> on driver load, but a later write of 0 should not result in something very different
> as that's not standard use of the ABI. If a user wants to go back to a sensible
> default then they should have stored out what was set initially.
>
> I don't mind if you update the default until the point where they first override
> it, but from there on we should obey what they request or error out if the
> value requested is not possible.
>
Hm, I'm unsure if I got this wrong. It is not supposed to be an
automatic feature to kick in and change user configured values,
actually. Let me try to explain it differently:
Setting a threshold for an inactivity time in [s] is always applied as
a user wishes. Setting 0s for inactivity time IMHO does not make much
sense, where one could also simply disable the sensor event. So, what
I did now is I implemented when 0s was set by a user for inactivity
time, it will result in an automatic adjustment of inactivity time,
depending on range and odr.
In v6 I will try to refrase the text, and double-check it's contained
in documentation, too. Pls, let me know what you think.
> > + *
> > + * The approach simply subtracts the pre-decimal figure of the configured
> > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > + * ignored in this estimation. The recommended ODRs for various features
> > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > + * defaults or need to be explicitly specified via val_s.
> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_set_inact_time_s(struct adxl345_state *st, u32 val_s)
> > +{
> > + unsigned int max_boundary = 255;
> > + unsigned int min_boundary = 10;
> > + unsigned int val = min(val_s, max_boundary);
> > + enum adxl345_odr odr;
> > + unsigned int regval;
> > + int ret;
> > +
> > + if (val == 0) {
> > + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > + if (ret)
> > + return ret;
> > + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > +
> > + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
> > + }
> > +
> > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > }
> >
>
> > @@ -1546,10 +1697,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > if (ret)
> > return ret;
> >
> > + ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, 3);
> > + if (ret)
> > + return ret;
> > +
> > ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
> > if (ret)
> > return ret;
> >
> > + ret = regmap_write(st->regmap, ADXL345_REG_THRESH_INACT, 4);
>
> Comments on defaults are good.
>
> > + if (ret)
> > + return ret;
> > +
> > ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
> > if (ret)
> > return ret;
>
Powered by blists - more mailing lists