Re: [PATCH] drivers: iio: accel: fix ADX355 startup race condition

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 16 Sep 2025 09:07:18 +0200
Andrej Valek <andrej.v@skyrain.eu> wrote:

> On 15.09.2025 17:53, Jonathan Cameron wrote:
> > On Mon, 15 Sep 2025 14:03:47 +0200
> > Andrej Valek <andrej.v@skyrain.eu> wrote:
> >  
> >> Hi Jonathan,
> >>
> >> I submitted the version 2 with applied your suggestions.
> >>
> >> On 12.09.2025 16:12, Jonathan Cameron wrote:  
> >>> On Thu, 11 Sep 2025 11:33:49 +0200
> >>> Andrej Valek <andrej.v@skyrain.eu> wrote:
> >>>     
> >>>> Hi Jonathan,
> >>>>
> >>>> First, I would like to thanks for your feedback.
> >>>>
> >>>> On 10.09.2025 20:30, Jonathan Cameron wrote:  
> >>>>> On Tue,  9 Sep 2025 10:55:28 +0200
> >>>>> Andrej Valek <andrej.v@skyrain.eu> wrote:
> >>>>>        
> >>>>>> From: Valek Andrej <andrej.v@skyrain.eu>  
> >>>>> Hi Valek,
> >>>>>
> >>>>> Thanks for the patch. Small thing on patch title, don't include drivers.
> >>>>> It's a pain but you need to look at other patches to a given subsystem
> >>>>> to find out the preferred style.  
> >>>> Valek is my surname 🙂.  
> >>> Oops. Sorry!
> >>>
> >>> Hi Andrej,
> >>>     
> >>>>>> Signed-off-by: Valek Andrej <andrej.v@skyrain.eu>
> >>>>>> Signed-off-by: Kessler Markus <markus.kessler@hilti.com>
> >>>>>> ---
> >>>>>>     drivers/iio/accel/adxl355_core.c | 48 ++++++++++++++++++++++++++++----
> >>>>>>     1 file changed, 43 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> >>>>>> index 2e00fd51b4d51..5386cd4766def 100644
> >>>>>> --- a/drivers/iio/accel/adxl355_core.c
> >>>>>> +++ b/drivers/iio/accel/adxl355_core.c
> >>>>>> @@ -56,6 +56,8 @@
> >>>>>>     #define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
> >>>>>>     #define ADXL355_SELF_TEST_REG		0x2E
> >>>>>>     #define ADXL355_RESET_REG		0x2F
> >>>>>> +#define ADXL355_BASE_ADDR_SHADOW_REG	0x50
> >>>>>> +#define ADXL355_SHADOW_REG_COUNT	5
> >>>>>>     
> >>>>>>     #define ADXL355_DEVID_AD_VAL		0xAD
> >>>>>>     #define ADXL355_DEVID_MST_VAL		0x1D
> >>>>>> @@ -294,6 +296,9 @@ static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
> >>>>>>     static int adxl355_setup(struct adxl355_data *data)
> >>>>>>     {
> >>>>>>     	unsigned int regval;
> >>>>>> +	u8 shadow_regs[ADXL355_SHADOW_REG_COUNT];  
> >>>>> Needs to be a DMA safe buffer.  We can't assume that regmap will always
> >>>>> bounce the data through one before passing it to the SPI controllers
> >>>>> that do sometimes require DMA safe buffers.    Add a buffer to end of
> >>>>> struct adxl355_data where you can take advantage of the forcing of appropriate
> >>>>> padding that is already going on there.  
> >>>> I see, but I don't like extending the adxl355_data just for one time
> >>>> usage. The suggested approach is more similar to what is done with the
> >>>> ID checking.
> >>>> | if (regval != ADXL355_DEVID_MST_VAL) {
> >>>> |  dev_err(data->dev, "Invalid MEMS ID 0x%02x\n", regval);
> >>>> |  return -ENODEV;
> >>>> | }  
> >>> That's a single read.  Regmap always bounces those so a local variable should
> >>> be fine. Not so for a bulk read (or at least no one guarantees it)
> >>>
> >>> https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
> >>> Slide 11 (in general this slide deck is a good introduction to the fun of DMA safety).
> >>>
> >>> The path for a single read goes through:
> >>> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L2779
> >>> _regmap_bus_read() which uses map->work_buf which is DMA safe.  
> >> regmap_bulk_read is using the same _regmap_read
> >> https://elixir.bootlin.com/linux/v6.16.7/source/drivers/base/regmap/regmap.c#L3137
> >> so it should be safe, too I guess.  
> > Whilst true, the repeated statement from the regmap maintainer that
> > is mentioned in the talk is that there is no guarantee that will continue
> > to be the case :(  Whereas the single reads are fine and will remain so.
> > I guess one reason for that is that regmap_read() take an unsigned int *val
> > so the type is almost always going to be wrong and a copy necessary.
> >
> > Anyhow, the whole thing is a bit silly but upshot is dma safe buffers only to bulk
> > accessors unless the ABI is ever documented as not needing them.
> >
> > Jonathan  
> Ok, I see. So can we live with the variant I did?
> >>> Jonathan
> >>>
> >>>     
> >> Andy
> >>  
> >  
> In other words, is there anything pending from my side?

Use a dma safe buffer for the bulk read. It's not a bug today
but it is relying on undocumented behaviour of the regmap
implementation that may change.

Jonathan

> 
> Andy