lkml.org 
[lkml]   [2018]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4] Staging: iio: ade7758: Expand buf_lock to cover both buffer and state protection
From


On 4 February 2018 21:03:05 GMT, Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
>On Sun, 2018-02-04 at 11:10 +0000, Jonathan Cameron wrote:
>> On Sat,  3 Feb 2018 21:01:32 +0530
>> Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
>>
>> >
>> > iio_dev->mlock is to be used only by the IIO core for protecting
>> > device mode changes between INDIO_DIRECT and INDIO_BUFFER.
>> >
>> > This patch replaces the use of mlock with the already established
>> > buf_lock mutex.
>> >
>> > Introducing 'unlocked' forms of read and write registers. The
>> > read/write frequency functions now require buf_lock to be held.
>> > That's not obvious so avoid this but moving the locking inside
>> > the functions where it is then clear that they are taking the
>> > unlocked forms of the register read/write.
>> >
>> > It isn't readily apparent that write frequency function requires
>> > the locks to be taken, so move it inside the function to where it
>> > is required to protect.
>> >
>> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
>> Hi Shreeya,
>>
>Hello sir,
>
>> Unfortunately this introduces a new bug - you end up unlocking
>> a mutex that you never locked in one of the error paths.
>> See inline.
>I'll make this change.
>>
>> We are also still taking the mlock around the read of the
>> frequency which doesn't make any sense given there is
>> no reason to protect that against state changes.
>> Arguably fixing that could be a separate patch as it never
>> made much sense, but it should probably be in this same series
>> at least.  I would have no real problem with it being in it this
>> same patch as long as the description above mentions it.
>>
>> Thanks,
>>
>> Jonathan
>>
>> >
>> > ---
>> >
>> > Changes in v2
>> >   -Add static keyword to newly introduced functions and remove some
>> > added comments which are not required.
>> >
>> > Changes in v3
>> >   -Remove some useless mlocks and send it as another patch.
>> > Also make the necessary change in the current patch associated
>> > with 
>> > the new patch with commit id 88eba33. Make commit message more 
>> > appropriate.
>> >
>> > Changes in v4
>> >   -Write frequency function do not require lock so move it inside
>> > the function to where it is required to protect.
>> >
>> >
>> >  drivers/staging/iio/meter/ade7758.h      |  2 +-
>> >  drivers/staging/iio/meter/ade7758_core.c | 49
>> > ++++++++++++++++++++++++--------
>> >  2 files changed, 38 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/staging/iio/meter/ade7758.h
>> > b/drivers/staging/iio/meter/ade7758.h
>> > index 6ae78d8..2de81b5 100644
>> > --- a/drivers/staging/iio/meter/ade7758.h
>> > +++ b/drivers/staging/iio/meter/ade7758.h
>> > @@ -111,7 +111,7 @@
>> >   * @trig: data ready trigger registered with iio
>> >   * @tx: transmit buffer
>> >   * @rx: receive buffer
>> > - * @buf_lock: mutex to protect tx and rx
>> > + * @buf_lock: mutex to protect tx, rx, read and
>> > write frequency
>> >   **/
>> >  struct ade7758_state {
>> >   struct spi_device *us;
>> > diff --git a/drivers/staging/iio/meter/ade7758_core.c
>> > b/drivers/staging/iio/meter/ade7758_core.c
>> > index 227dbfc..ff19d46 100644
>> > --- a/drivers/staging/iio/meter/ade7758_core.c
>> > +++ b/drivers/staging/iio/meter/ade7758_core.c
>> > @@ -24,17 +24,25 @@
>> >  #include "meter.h"
>> >  #include "ade7758.h"
>> >  
>> > -int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
>> > val)
>> > +static int __ade7758_spi_write_reg_8(struct device *dev, u8
>> > reg_address, u8 val)
>> >  {
>> > - int ret;
>> >   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> >   struct ade7758_state *st = iio_priv(indio_dev);
>> >  
>> > - mutex_lock(&st->buf_lock);
>> >   st->tx[0] = ADE7758_WRITE_REG(reg_address);
>> >   st->tx[1] = val;
>> >  
>> > - ret = spi_write(st->us, st->tx, 2);
>> > + return spi_write(st->us, st->tx, 2);
>> > +}
>> > +
>> > +int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
>> > val)
>> > +{
>> > + int ret;
>> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > + struct ade7758_state *st = iio_priv(indio_dev);
>> > +
>> > + mutex_lock(&st->buf_lock);
>> > + ret = __ade7758_spi_write_reg_8(dev, reg_address, val);
>> >   mutex_unlock(&st->buf_lock);
>> >  
>> >   return ret;
>> > @@ -91,7 +99,7 @@ static int ade7758_spi_write_reg_24(struct device
>> > *dev, u8 reg_address,
>> >   return ret;
>> >  }
>> >  
>> > -int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
>> > *val)
>> > +static int __ade7758_spi_read_reg_8(struct device *dev, u8
>> > reg_address, u8 *val)
>> >  {
>> >   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> >   struct ade7758_state *st = iio_priv(indio_dev);
>> > @@ -111,7 +119,6 @@ int ade7758_spi_read_reg_8(struct device *dev,
>> > u8 reg_address, u8 *val)
>> >   },
>> >   };
>> >  
>> > - mutex_lock(&st->buf_lock);
>> >   st->tx[0] = ADE7758_READ_REG(reg_address);
>> >   st->tx[1] = 0;
>> >  
>> > @@ -124,7 +131,19 @@ int ade7758_spi_read_reg_8(struct device *dev,
>> > u8 reg_address, u8 *val)
>> >   *val = st->rx[0];
>> >  
>> >  error_ret:
>> > + return ret;
>> > +}
>> > +
>> > +int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
>> > *val)
>> > +{
>> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > + struct ade7758_state *st = iio_priv(indio_dev);
>> > + int ret;
>> > +
>> > + mutex_lock(&st->buf_lock);
>> > + ret = __ade7758_spi_read_reg_8(dev, reg_address, val);
>> >   mutex_unlock(&st->buf_lock);
>> > +
>> >   return ret;
>> >  }
>> >  
>> > @@ -480,10 +499,12 @@ static int ade7758_read_samp_freq(struct
>> > device *dev, int *val)
>> >   return 0;
>> >  }
>> >  
>> > -static int ade7758_write_samp_freq(struct device *dev, int val)
>> > +static int ade7758_write_samp_freq(struct iio_dev *indio_dev, int
>> > val)
>> >  {
>> >   int ret;
>> >   u8 reg, t;
>> > + struct ade7758_state *st = iio_priv(indio_dev);
>> > + struct device *dev = &indio_dev->dev;
>> >  
>> >   switch (val) {
>> >   case 26040:
>> > @@ -503,16 +524,20 @@ static int ade7758_write_samp_freq(struct
>> > device *dev, int val)
>> >   goto out;
>> This goto out results in an unlock but the lock hasn't been locked
>> for a few more lines...
>>
>> Change this to a direct return here rather than a goto to fix this.
>> return -EINVAL;
>>
>> >
>> >   }
>> >  
>> > - ret = ade7758_spi_read_reg_8(dev, ADE7758_WAVMODE, &reg);
>> > + mutex_lock(&st->buf_lock);
>> > +
>> > + ret = __ade7758_spi_read_reg_8(dev, ADE7758_WAVMODE,
>> > &reg);
>> >   if (ret)
>> >   goto out;
>
>Here, can I move the above lock after the calling of the read register
>but before the if(ret) statement?
>With this we can avoid locks over the read cases.
>Mostly, this shouldn't create any problem but yet I thought of first
>confirming it from you.

Don't do that. We need to protect against another caller doing that read before the write is done.
>
>Thank you 
>
>> >  
>> >   reg &= ~(5 << 3);
>> >   reg |= t << 5;
>> >  
>> > - ret = ade7758_spi_write_reg_8(dev, ADE7758_WAVMODE, reg);
>> > + ret = __ade7758_spi_write_reg_8(dev, ADE7758_WAVMODE,
>> > reg);
>> >  
>> >  out:
>> > + mutex_unlock(&st->buf_lock);
>> > +
>> >   return ret;
>> >  }
>> >  
>> > @@ -545,9 +570,9 @@ static int ade7758_write_raw(struct iio_dev
>> > *indio_dev,
>> >   case IIO_CHAN_INFO_SAMP_FREQ:
>> >   if (val2)
>> >   return -EINVAL;
>> > - mutex_lock(&indio_dev->mlock);
>> > - ret = ade7758_write_samp_freq(&indio_dev->dev,
>> > val);
>> > - mutex_unlock(&indio_dev->mlock);
>> > +
>> > + ret = ade7758_write_samp_freq(indio_dev, val);
>> > +
>> >   return ret;
>> >   default:
>> >   return -EINVAL;
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

\
 
 \ /
  Last update: 2018-02-04 22:23    [W:0.082 / U:2.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site