| Message ID | 20170918065952.GA5386@himanshi-Inspiron-5558 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 18, 2017 at 12:29:52PM +0530, Himanshi Jain wrote: > Replace driver usage of mlock with driver private lock to meet the new > model where usage of iio_dev->mlock is being redefined as protecting > operating mode changes(changes between BUFFER* and DIRECT modes). This is more complex than a "normal" outreachy coding style cleanup, so I'll leave it to the IIO developers/maintainer to review/apply it. thanks, greg k-h -- 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
On Mon, 18 Sep 2017 12:29:52 +0530 Himanshi Jain <himshijain.hj@gmail.com> wrote: > Replace driver usage of mlock with driver private lock to meet the new > model where usage of iio_dev->mlock is being redefined as protecting > operating mode changes(changes between BUFFER* and DIRECT modes). > > Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> A well presented patch making a sensible change. My only thought here is that ultimately we could cover bother the buffer protection and the state protection with a single lock. Hmm. The new lock (as was the old mlock code) is providing protection to ensure we end up with consistent state when changing the sampling frequency between the sampling frequency and the bus clock speed (which has different maximum values depending on the current sampling frequency). In similar cases we have expanded the meaning on the buffer lock to fulfil both roles - that could be done here as well, although you have to be careful about deadlocks. On reflection I think this patch is worthwhile applying even if this new lock gets dropped again in some later rework of this driver. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > drivers/staging/iio/meter/ade7753.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c > index ce26abdea..745c5a6 100644 > --- a/drivers/staging/iio/meter/ade7753.c > +++ b/drivers/staging/iio/meter/ade7753.c > @@ -81,10 +81,12 @@ > * @tx: transmit buffer > * @rx: receive buffer > * @buf_lock: mutex to protect tx and rx > + * @lock: protect sensor data > **/ > struct ade7753_state { > struct spi_device *us; > struct mutex buf_lock; > + struct mutex lock; /* protect sensor data */ > u8 tx[ADE7753_MAX_TX] ____cacheline_aligned; > u8 rx[ADE7753_MAX_RX]; > }; > @@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, > if (!val) > return -EINVAL; > > - mutex_lock(&indio_dev->mlock); > + mutex_lock(&st->lock); > > t = 27900 / val; > if (t > 0) > @@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, > ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg); > > out: > - mutex_unlock(&indio_dev->mlock); > + mutex_unlock(&st->lock); > > return ret ? ret : len; > } > @@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi) > st = iio_priv(indio_dev); > st->us = spi; > mutex_init(&st->buf_lock); > + mutex_init(&st->lock); > > indio_dev->name = spi->dev.driver->name; > indio_dev->dev.parent = &spi->dev; -- 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
On Sun, Sep 24, 2017 at 7:47 PM, Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 18 Sep 2017 12:29:52 +0530 > Himanshi Jain <himshijain.hj@gmail.com> wrote: > >> Replace driver usage of mlock with driver private lock to meet the new >> model where usage of iio_dev->mlock is being redefined as protecting >> operating mode changes(changes between BUFFER* and DIRECT modes). >> >> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > > A well presented patch making a sensible change. My only thought here > is that ultimately we could cover bother the buffer protection and > the state protection with a single lock. > > Hmm. The new lock (as was the old mlock code) is providing protection > to ensure we end up with consistent state when changing the > sampling frequency between the sampling frequency and the > bus clock speed (which has different maximum values depending > on the current sampling frequency). In similar cases we have > expanded the meaning on the buffer lock to fulfil both roles > - that could be done here as well, although you have to be careful > about deadlocks. > I have understood how to use a single lock for both the purposes as you have quoted in Katie's patch too i.e. by making an unlocked version of the function ade7753_spi_write_reg_16(). And since the function is not publicly available to be used, an unlocked version will not create any issue(which is why I am assuming, we are not using nested locks - Please correct me if I am wrong.) I will submit a new patch with a single lock. > On reflection I think this patch is worthwhile applying even > if this new lock gets dropped again in some later rework of this > driver. > > Applied to the togreg branch of iio.git and pushed out as testing for > the autobuilders to play with it. > Thank you for reviewing and applying the patch and guiding for a better logical solution. > Thanks, > > Jonathan >> --- >> drivers/staging/iio/meter/ade7753.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c >> index ce26abdea..745c5a6 100644 >> --- a/drivers/staging/iio/meter/ade7753.c >> +++ b/drivers/staging/iio/meter/ade7753.c >> @@ -81,10 +81,12 @@ >> * @tx: transmit buffer >> * @rx: receive buffer >> * @buf_lock: mutex to protect tx and rx >> + * @lock: protect sensor data >> **/ >> struct ade7753_state { >> struct spi_device *us; >> struct mutex buf_lock; >> + struct mutex lock; /* protect sensor data */ >> u8 tx[ADE7753_MAX_TX] ____cacheline_aligned; >> u8 rx[ADE7753_MAX_RX]; >> }; >> @@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, >> if (!val) >> return -EINVAL; >> >> - mutex_lock(&indio_dev->mlock); >> + mutex_lock(&st->lock); >> >> t = 27900 / val; >> if (t > 0) >> @@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, >> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg); >> >> out: >> - mutex_unlock(&indio_dev->mlock); >> + mutex_unlock(&st->lock); >> >> return ret ? ret : len; >> } >> @@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi) >> st = iio_priv(indio_dev); >> st->us = spi; >> mutex_init(&st->buf_lock); >> + mutex_init(&st->lock); >> >> indio_dev->name = spi->dev.driver->name; >> indio_dev->dev.parent = &spi->dev; > -- 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
On Wed, 27 Sep 2017 20:27:09 +0530 Himanshi Jain <himshijain.hj@gmail.com> wrote: > On Sun, Sep 24, 2017 at 7:47 PM, Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 18 Sep 2017 12:29:52 +0530 > > Himanshi Jain <himshijain.hj@gmail.com> wrote: > > > >> Replace driver usage of mlock with driver private lock to meet the new > >> model where usage of iio_dev->mlock is being redefined as protecting > >> operating mode changes(changes between BUFFER* and DIRECT modes). > >> > >> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > > > > A well presented patch making a sensible change. My only thought here > > is that ultimately we could cover bother the buffer protection and > > the state protection with a single lock. > > > > Hmm. The new lock (as was the old mlock code) is providing protection > > to ensure we end up with consistent state when changing the > > sampling frequency between the sampling frequency and the > > bus clock speed (which has different maximum values depending > > on the current sampling frequency). In similar cases we have > > expanded the meaning on the buffer lock to fulfil both roles > > - that could be done here as well, although you have to be careful > > about deadlocks. > > > > I have understood how to use a single lock for both the purposes as you have > quoted in Katie's patch too i.e. by making an unlocked version of the function > ade7753_spi_write_reg_16(). And since the function is not publicly available > to be used, an unlocked version will not create any issue(which is why I am > assuming, we are not using nested locks - Please correct me if I am wrong.) > > I will submit a new patch with a single lock. That's great, but please do it on top of your previous patch as that is now in a non rebasing tree. Jonathan > > > On reflection I think this patch is worthwhile applying even > > if this new lock gets dropped again in some later rework of this > > driver. > > > > Applied to the togreg branch of iio.git and pushed out as testing for > > the autobuilders to play with it. > > > > Thank you for reviewing and applying the patch and guiding for a better logical > solution. > > > Thanks, > > > > Jonathan > >> --- > >> drivers/staging/iio/meter/ade7753.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c > >> index ce26abdea..745c5a6 100644 > >> --- a/drivers/staging/iio/meter/ade7753.c > >> +++ b/drivers/staging/iio/meter/ade7753.c > >> @@ -81,10 +81,12 @@ > >> * @tx: transmit buffer > >> * @rx: receive buffer > >> * @buf_lock: mutex to protect tx and rx > >> + * @lock: protect sensor data > >> **/ > >> struct ade7753_state { > >> struct spi_device *us; > >> struct mutex buf_lock; > >> + struct mutex lock; /* protect sensor data */ > >> u8 tx[ADE7753_MAX_TX] ____cacheline_aligned; > >> u8 rx[ADE7753_MAX_RX]; > >> }; > >> @@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, > >> if (!val) > >> return -EINVAL; > >> > >> - mutex_lock(&indio_dev->mlock); > >> + mutex_lock(&st->lock); > >> > >> t = 27900 / val; > >> if (t > 0) > >> @@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, > >> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg); > >> > >> out: > >> - mutex_unlock(&indio_dev->mlock); > >> + mutex_unlock(&st->lock); > >> > >> return ret ? ret : len; > >> } > >> @@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi) > >> st = iio_priv(indio_dev); > >> st->us = spi; > >> mutex_init(&st->buf_lock); > >> + mutex_init(&st->lock); > >> > >> indio_dev->name = spi->dev.driver->name; > >> indio_dev->dev.parent = &spi->dev; > > > -- > 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 -- 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
diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c index ce26abdea..745c5a6 100644 --- a/drivers/staging/iio/meter/ade7753.c +++ b/drivers/staging/iio/meter/ade7753.c @@ -81,10 +81,12 @@ * @tx: transmit buffer * @rx: receive buffer * @buf_lock: mutex to protect tx and rx + * @lock: protect sensor data **/ struct ade7753_state { struct spi_device *us; struct mutex buf_lock; + struct mutex lock; /* protect sensor data */ u8 tx[ADE7753_MAX_TX] ____cacheline_aligned; u8 rx[ADE7753_MAX_RX]; }; @@ -483,7 +485,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, if (!val) return -EINVAL; - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->lock); t = 27900 / val; if (t > 0) @@ -504,7 +506,7 @@ static ssize_t ade7753_write_frequency(struct device *dev, ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg); out: - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->lock); return ret ? ret : len; } @@ -580,6 +582,7 @@ static int ade7753_probe(struct spi_device *spi) st = iio_priv(indio_dev); st->us = spi; mutex_init(&st->buf_lock); + mutex_init(&st->lock); indio_dev->name = spi->dev.driver->name; indio_dev->dev.parent = &spi->dev;
Replace driver usage of mlock with driver private lock to meet the new model where usage of iio_dev->mlock is being redefined as protecting operating mode changes(changes between BUFFER* and DIRECT modes). Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> --- drivers/staging/iio/meter/ade7753.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)