Re: [Outreachy kernel] [PATCH v2] staging: iio: ade7753: replace mlock with driver private lock

[Jonathan Cameron]

On 18/03/17 18:46, SIMRAN SINGHAL wrote:
> On Sat, Mar 18, 2017 at 11:51 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> On 18/03/17 17:34, SIMRAN SINGHAL wrote:
>>> On Thu, Mar 16, 2017 at 3:23 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>>>> On 13/03/17 19:53, Alison Schofield wrote:
>>>>> On Mon, Mar 13, 2017 at 10:01:07PM +0530, simran singhal 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 a lock in the devices global data.
>>>>>>
>>>>>> Fix some coding style issues related to white space also.
>>>>>>
>>>>>> Signed-off-by: simran singhal <singhalsimran0@xxxxxxxxx>
>>>>>> ---
>>>>>>
>>>>>>  v2:
>>>>>>    -Removed new lock to reuse the existing lock
>>>>> Simran,
>>>>>
>>>>> The good news is that you have 2 patches that have similar
>>>>> challenges!  I'll suggest picking one, drive it to completion,
>>>>> then do the other.
>>>>>
>>>>> This has the nested locking issue that Lars warned about.
>>>>> Need to refactor to avoid.  Check back on his review comments.
>>>>>
>>>>> I suggest dropping those whitespace changes - they appear
>>>>> out of place in this patch since you are no longer actually
>>>>> touching those lines of code.
>>>>>
>>>>> alisons
>>>> Also missing a mutex_init, though that may well become irrelevant
>>>> with refactoring as suggested.
>>>
>>> Jonathon, what I found by looking at the codes of other drivers is that
>>> we have to use mutex_init in probe function only.
>>>
>>> Is this correct?
>> Yes, it only needs initializing once.
>>>
> So, no need of using mutex_init here as the function is not a
> probe function.
Sorry, my confusion was that I thought you were creating a new
mutex. This was kind of implied by the description and I lost
the fact it was already there in the reformatting of the
private structure.

Please could you fix that alignment first in a precursor patch and
follow it up in the same series with the change of mutex.

Will make it easier to see what is going on.
Which would in turn highlight that you have a deadlock here.

Key rule of mutexes, you can't lock them twice in a row.
Here you take the lock where mlock was previous taken and again
in the call to ade7753_spi_write_reg_16.

The second will hang for ever as it is waiting for the lock to
be released which obviously can't happen if it's held by the
same thread. 

mlock here is doing a lot more than protecting the buffer.
It is ensuring the spi bus frequency and sampling frequency
of the device are changed in an atomic fashion.

I think this one does need a new local lock alongside buf lock.
The purposes are completely different so it would be cleaner to
just have two locks.

Thanks,

Jonathan
> 
> Thanks
> Simran
> 
>>> And this patch also suggest the same:-
>>> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=388c8f18ff29fe95dbf72cb0a1bd8fbcd6f52f8f
>>>
>>>>>
>>>>>
>>>>>>
>>>>>>  drivers/staging/iio/meter/ade7753.c | 12 ++++++------
>>>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
>>>>>> index dfd8b71..d88eaa3 100644
>>>>>> --- a/drivers/staging/iio/meter/ade7753.c
>>>>>> +++ b/drivers/staging/iio/meter/ade7753.c
>>>>>> @@ -83,10 +83,10 @@
>>>>>>   * @buf_lock:       mutex to protect tx and rx
>>>>>>   **/
>>>>>>  struct ade7753_state {
>>>>>> -        struct spi_device   *us;
>>>>>> -                struct mutex        buf_lock;
>>>>>> -                        u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>>>>>> -                                u8          rx[ADE7753_MAX_RX];
>>>>>> +    struct spi_device   *us;
>>>>>> +    struct mutex        buf_lock;
>>>>>> +    u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
>>>>>> +    u8          rx[ADE7753_MAX_RX];
>>>>>>  };
>>>>>>
>>>>>>  static int ade7753_spi_write_reg_8(struct device *dev,
>>>>>> @@ -484,7 +484,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>>>>>>      if (!val)
>>>>>>              return -EINVAL;
>>>>>>
>>>>>> -    mutex_lock(&indio_dev->mlock);
>>>>>> +    mutex_lock(&st->buf_lock);
>>>>>>
>>>>>>      t = 27900 / val;
>>>>>>      if (t > 0)
>>>>>> @@ -505,7 +505,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->buf_lock);
>>>>>>
>>>>>>      return ret ? ret : len;
>>>>>>  }
>>>>>> --
>>>>>> 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@xxxxxxxxxxxxxxxxx
>>>>>> To post to this group, send email to outreachy-kernel@xxxxxxxxxxxxxxxxx
>>>>>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170313163107.GA31496%40singhal-Inspiron-5558.
>>>>>> 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
>>>>>
>>>>
>>
> --
> 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
>