linux-kernel - Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231204152120.71455f40@jic23-huawei>
Date:   Mon, 4 Dec 2023 15:21:20 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Nuno Sá <noname.nuno@...il.com>
Cc:     David Lechner <dlechner@...libre.com>, nuno.sa@...log.com,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-iio@...r.kernel.org,
        Olivier MOYSAN <olivier.moysan@...s.st.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>
Subject: Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct
 ad9467_state

On Fri, 01 Dec 2023 09:49:38 +0100
Nuno Sá <noname.nuno@...il.com> wrote:

> On Thu, 2023-11-30 at 15:50 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@...nel.org> wrote:  
> > > 
> > > From: Nuno Sa <nuno.sa@...log.com>
> > > 
> > > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > > are done which means we need to properly protect the whole operation so
> > > we are sure we will be in a sane state if two concurrent calls occur.
> > > 
> > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > > Signed-off-by: Nuno Sa <nuno.sa@...log.com>
> > > ---
> > >  drivers/iio/adc/ad9467.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 04474dbfa631..91821dee03b7 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c
> > > @@ -4,7 +4,7 @@
> > >   *
> > >   * Copyright 2012-2020 Analog Devices Inc.
> > >   */
> > > -
> > > +#include <linux/cleanup.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>  
> > 
> > Ah, the case of the misplaced header from the previous patch is solved. :-)
> >   
> 
> Yeps, it needs to be in this patch :)
> 
> > >  #include <linux/device.h>
> > > @@ -122,6 +122,8 @@ struct ad9467_state {
> > >         unsigned int                    output_mode;
> > > 
> > >         struct gpio_desc                *pwrdown_gpio;
> > > +       /* protect against concurrent accesses to the device */
> > > +       struct mutex                    lock;
> > >  };
> > > 
> > >  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > > @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv,
> > > unsigned int reg,
> > >         int ret;
> > > 
> > >         if (!readval) {
> > > +               guard(mutex)(&st->lock);
> > >                 ret = ad9467_spi_write(spi, reg, writeval);
> > >                 if (ret)
> > >                         return ret;
> > > @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv,
> > > int val, int val2)
> > >                 if (scale_val[0] != val || scale_val[1] != val2)
> > >                         continue;
> > > 
> > > +               guard(mutex)(&st->lock);
> > >                 ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> > >                                        info->scale_table[i][1]);
> > >                 if (ret < 0)
> > > 
> > > --
> > > 2.42.1
> > > 
> > >   
> > 
> > Alternately, this could probably be solved with spi_bus_lock/unlock
> > and spi_sync_locked rather than introducing a new mutex.  
> 
> Hmm, typically you just have your own lock. No reason to lock the spi bus. And I also
> have some plans to eventually change this to regmap :)

Bus lock typically implies that we can't let other users grab the bus in between
for reasons like the chip select needing to be held. I'm not keen on it being
used if the locking is just needed for a specific driver to deal with its
associated device and driver state.

Jonathan

> 
> - Nuno Sá
>