On Tue, Mar 21, 2017 at 11:42 PM, Alison Schofield <amsfield22@xxxxxxxxx> wrote:
> On Tue, Mar 21, 2017 at 11:18:23PM +0530, Gargi Sharma wrote:
>> On Tue, Mar 21, 2017 at 10:54 PM, Alison Schofield <amsfield22@xxxxxxxxx> wrote:
>> > On Mon, Mar 20, 2017 at 01:09:21PM +0530, Gargi Sharma wrote:
>> >> The IIO subsystem is redefining iio_dev->mlock to be used by
>> >> the IIO core only for protecting device operating mode changes.
>> >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>> >>
>> >> In this driver, mlock was being used to protect hardware state
>> >> changes. Replace it with buf_lock in the devices global data.
>> >>
>> >> As buf_lock already protects operations in ade7754_write_frequency,
>> >> there isn't a need to acquire the lock inside ade7754_spi_write_reg_8
>> >> when writing to the register.
>> >
>> > Hi Gargi,
>> >
>> > Looks like something went wrong in your patch below. It doesn't do what
>> > you say it'll do...Instead of removing the lock from _write_reg_8()
>> > it inserts a bunch of code. Anyway, it seems that w_rite_reg_8() is used
>> > in multiple places, so removing that lock doesn't appear to be an
>> > option.
>> >
>> > See below...
>> >
>> > alisons
>> >
>> >>
>> >> Signed-off-by: Gargi Sharma <gs051095@xxxxxxxxx>
>> >> ---
>> >> drivers/staging/iio/meter/ade7754.c | 13 +++++++++++--
>> >> 1 file changed, 11 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
>> >> index 024463a..eb03469 100644
>> >> --- a/drivers/staging/iio/meter/ade7754.c
>> >> +++ b/drivers/staging/iio/meter/ade7754.c
>> >> @@ -29,6 +29,15 @@ static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>> >> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> >> struct ade7754_state *st = iio_priv(indio_dev);
>> >>
>> >> + if (reg_address == ADE7754_WAVMODE) {
>> >> + st->tx[0] = ADE7754_WRITE_REG(reg_address);
>> >> + st->tx[1] = val;
>> >> +
>> >> + ret = spi_write(st->us, st->tx, 2);
>> >> +
>> >> + return ret;
>> >> + }
>> >> +
>> > What's this?
>>
>> When the function ade_spi_write_reg_8() is called inside
>> ade7754_write_frequency(), we are writing to this( ADE7754_WAVMODE)
>> register. When writing to this register we don't need to hold the
>> buf_lock since ade7754_write_frequency() already takes care of that.
>
> Oh! I see it now. You created a special 'no lock needed' case
> inside of --write_reg_8 for writing frequency. That works,
> but it's...ummm...sneaky ;) Let's see if there's another way.
>
> Look back at Lars suggestion on a similar patch. Maybe that
> will apply here.
> http://marc.info/?l=linux-kernel&m=148940648615743&w=2
>
I did look at the patch you suggested for inspiration :) What I could
not understand was the part where Lars wrote about "doing a
read-modify-write cycle in a protected section." I can write a
separate function for --write_reg_8 that does not take the lock, but
do not know how to do a "read-modify-write cycle in a protected
section" :(
Gargi
> alisons
>
>
>>
>> >
>> >> mutex_lock(&st->buf_lock);
>> >> st->tx[0] = ADE7754_WRITE_REG(reg_address);
>> >> st->tx[1] = val;
>> >> @@ -430,7 +439,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>> >> if (!val)
>> >> return -EINVAL;
>> >>
>> >> - mutex_lock(&indio_dev->mlock);
>> >> + mutex_lock(&st->buf_lock);
>> >>
>> >> t = 26000 / val;
>> >> if (t > 0)
>> >> @@ -451,7 +460,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>> >> ret = ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>> >>
>> >> out:
>> >> - mutex_unlock(&indio_dev->mlock);
>> >> + mutex_unlock(&st->buf_lock);
>> >>
>> >> return ret ? ret : len;
>> >> }
>>
>> The buf_lock inside ade7754_write_frequency() takes into account that
>> when using the function ade7754_spi_write_reg_8, lock is already held
>> and locking is no longer required inside the ade7754_spi_write_reg_8()
>> function.
>>
>> Let me know if this sounds okay, I can perhaps edit the commit log to
>> make this clearer.
>>
>> Thanks,
>> Gargi
>>
>> >> --
>> >> 2.7.4
>> >>
>> >> --
>> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@xxxxxxxxxxxxxxxx.
>> >> To post to this group, send email to outreachy-kernel@xxxxxxxxxxxxxxxx.
>> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1489995561-6988-1-git-send-email-gs051095%40gmail.com.
>> >> For more options, visit https://groups.google.com/d/optout.
>> >
>> > --
>> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@xxxxxxxxxxxxxxxx.
>> > To post to this group, send email to outreachy-kernel@xxxxxxxxxxxxxxxx.
>> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170321172438.GC2793%40d830.WORKGROUP.
>> > For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@xxxxxxxxxxxxxxxx.
> To post to this group, send email to outreachy-kernel@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170321181207.GA10699%40d830.WORKGROUP.
> For more options, visit https://groups.google.com/d/optout.
--
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
