Re: [PATCH v4] Staging: iio: ade7758: Expand buf_lock to cover both buffer and state protection

[Jonathan Cameron]

On 4 February 2018 21:03:05 GMT, Shreeya Patel <shreeya.patel23498@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>> 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@xxxxxxxxxxxxxxx
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

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