| 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: <swswxbwejqyrekr2bvjf4p5lglodg3hlgl5ckiluxpazzl3chn@ga3uriqvmv7b>
Date: Sat, 26 Apr 2025 21:19:13 -0300
From: Gustavo Silva <gustavograzs@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alex Lanzano <lanzano.alex@...il.com>,
David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] iio: imu: bmi270: add channel for step counter
On Sat, Apr 26, 2025 at 02:40:20PM +0100, Jonathan Cameron wrote:
> On Thu, 24 Apr 2025 21:14:50 -0300
> Gustavo Silva <gustavograzs@...il.com> wrote:
>
> > Add a channel for enabling/disabling the step counter, reading the
> > number of steps and resetting the counter.
> >
> > Signed-off-by: Gustavo Silva <gustavograzs@...il.com>
> Hi Gustavo,
>
> This is tripping over the somewhat theoretical requirement for
> regmap to be provided with DMA safe buffers for bulk accesses.
>
> Jonathan
>
Hi Jonathan,
Thanks for the review. I've got a few questions inline.
> > ---
> > drivers/iio/imu/bmi270/bmi270_core.c | 127 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> >
> > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> > index a86be5af5ccb1f010f2282ee31c47f284c1bcc86..f09d8dead9df63df5ae8550cf473b5573374955b 100644
> > --- a/drivers/iio/imu/bmi270/bmi270_core.c
> > +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> > @@ -31,6 +31,8 @@
>
> > /* See datasheet section 4.6.14, Temperature Sensor */
> > #define BMI270_TEMP_OFFSET 11776
> > #define BMI270_TEMP_SCALE 1953125
> > @@ -111,6 +118,7 @@ struct bmi270_data {
> > struct iio_trigger *trig;
> > /* Protect device's private data from concurrent access */
> > struct mutex mutex;
> > + int steps_enabled;
>
> Seems a little odd to have a thing called _enabled as an integer.
> Probably better as a bool even though that will require slightly more
> code to handle read / write.
>
I agree that a bool might be more appropriate in this case. I decided to
use an int to keep consistency with other drivers, specifically bma400
and the iio dummy driver.
If you prefer, I'll use a bool here and after this series submit a patch
updating those drivers as well.
>
> >
> > /*
> > * Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
> > @@ -282,6 +290,99 @@ static const struct bmi270_odr_item bmi270_odr_table[] = {
> > },
> > };
> >
> > +enum bmi270_feature_reg_id {
> > + BMI270_SC_26_REG,
> > +};
> > +
> > +struct bmi270_feature_reg {
> > + u8 page;
> > + u8 addr;
> > +};
> > +
> > +static const struct bmi270_feature_reg bmi270_feature_regs[] = {
> > + [BMI270_SC_26_REG] = {
> > + .page = 6,
> > + .addr = 0x32,
> > + },
> > +};
> > +
> > +static int bmi270_write_feature_reg(struct bmi270_data *data,
> > + enum bmi270_feature_reg_id id,
> > + u16 val)
> > +{
> > + const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> > + int ret;
> > +
> > + ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_bulk_write(data->regmap, reg->addr, &val, sizeof(val));
>
> For a regmap on top of an SPI bus. I think we are still requiring DMA safe
> buffers until we can get confirmation that the API guarantees that isn't
> needed. (there is another thread going on where we are trying to get that
> confirmation).
>
> That is a pain here because this can run concurrently with buffered
> capture and as such I think we can't just put a additional element after
> data->data but instead need to mark that new element __aligned(IIO_DMA_MINALIGN)
> as well (and add a comment that it can be used concurrently with data->data).
>
Just to clarify, when you say data->data, are you referring to the
bmi270_data::buffer variable? That used to be called 'data' but it was
changed to 'buffer' in commit 16c94de2a.
> This hole thing is a mess because in reality I think the regmap core is always
> bouncing data today. In theory it could sometimes be avoiding copies
> and the underlying regmap_spi does require DMA safe buffers. This all relies
> on an old discussion where Mark Brown said that we should not assume any
> different requirements from the the underlying bus.
>
> > +}
> > +
> > +static int bmi270_read_feature_reg(struct bmi270_data *data,
> > + enum bmi270_feature_reg_id id,
> > + u16 *val)
> > +{
> > + const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> > + int ret;
> > +
> > + ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_bulk_read(data->regmap, reg->addr, val, sizeof(*val));
> > +}
> > +
> > @@ -551,6 +652,8 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
> > struct bmi270_data *data = iio_priv(indio_dev);
> >
> > switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + return bmi270_read_steps(data, val);
> > case IIO_CHAN_INFO_RAW:
> > if (!iio_device_claim_direct(indio_dev))
> > return -EBUSY;
> > @@ -571,6 +674,10 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > ret = bmi270_get_odr(data, chan->type, val, val2);
> > return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_ENABLE:
> > + scoped_guard(mutex, &data->mutex)
> > + *val = data->steps_enabled;
>
> What race is this protecting against? Protecting the write is fine because it
> is about ensuring we don't race an enable against a clear of the counter.
> A race here would I think just give the same as either the race to take the lock
> being won by this or not (so not a race as such, just ordering of calls)
> So I don't think you need the lock here.
>
Understood. I'll fix it in v2.
> > + return IIO_VAL_INT;
>
>
Powered by blists - more mailing lists