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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

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

> >
> > Jonathan
> >
> >  
> Andy
>