Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1603969 > unrolled thread
| Started by | simran singhal <singhalsimran0@gmail.com> |
|---|---|
| First post | 2017-03-19 14:00 +0100 |
| Last post | 2017-03-21 20:40 +0100 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v5] staging: Use buf_lock instead of mlock and Refactor code simran singhal <singhalsimran0@gmail.com> - 2017-03-19 14:00 +0100
Re: [Outreachy kernel] [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code Gargi Sharma <gs051095@gmail.com> - 2017-03-19 18:20 +0100
Re: [Outreachy kernel] [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code Jonathan Cameron <jic23@kernel.org> - 2017-03-19 21:50 +0100
Re: [Outreachy kernel] [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code SIMRAN SINGHAL <singhalsimran0@gmail.com> - 2017-03-19 22:20 +0100
Re: [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code SIMRAN SINGHAL <singhalsimran0@gmail.com> - 2017-03-21 16:10 +0100
Re: [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code Jonathan Cameron <jic23@kernel.org> - 2017-03-21 20:40 +0100
| From | simran singhal <singhalsimran0@gmail.com> |
|---|---|
| Date | 2017-03-19 14:00 +0100 |
| Subject | [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code |
| Message-ID | <tmIBc-5Kw-15@gated-at.bofh.it> |
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 protects both the adis16060_spi_write() and
adis16060_spi_read() functions and both are always called in
pair. First write, then read. Thus, refactor the code to have
one single function adis16060_spi_write_than_read() which is
protected by the existing buf_lock.
Signed-off-by: simran singhal <singhalsimran0@gmail.com>
---
v5:
-Rename val in adis16060_spi_write_than_read() to conf.
-Rename val2 in adis16060_spi_write_than_read() to val.
-Corrected Checkpatch issues.
-Removed goto from adis16060_read_raw().
drivers/staging/iio/gyro/adis16060_core.c | 42 ++++++++++++-------------------
1 file changed, 16 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
index c9d46e7..0f12492 100644
--- a/drivers/staging/iio/gyro/adis16060_core.c
+++ b/drivers/staging/iio/gyro/adis16060_core.c
@@ -40,25 +40,20 @@ struct adis16060_state {
static struct iio_dev *adis16060_iio_dev;
-static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
+static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
+ u8 conf, u16 *val)
{
int ret;
struct adis16060_state *st = iio_priv(indio_dev);
mutex_lock(&st->buf_lock);
- st->buf[2] = val; /* The last 8 bits clocked in are latched */
+ st->buf[2] = conf; /* The last 8 bits clocked in are latched */
ret = spi_write(st->us_w, st->buf, 3);
- mutex_unlock(&st->buf_lock);
- return ret;
-}
-
-static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
-{
- int ret;
- struct adis16060_state *st = iio_priv(indio_dev);
-
- mutex_lock(&st->buf_lock);
+ if (ret < 0) {
+ mutex_unlock(&st->buf_lock);
+ return ret;
+ }
ret = spi_read(st->us_r, st->buf, 3);
@@ -69,8 +64,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
*/
if (!ret)
*val = ((st->buf[0] & 0x3) << 12) |
- (st->buf[1] << 4) |
- ((st->buf[2] >> 4) & 0xF);
+ (st->buf[1] << 4) |
+ ((st->buf[2] >> 4) & 0xF);
mutex_unlock(&st->buf_lock);
return ret;
@@ -83,20 +78,19 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
{
u16 tval = 0;
int ret;
+ struct adis16060_state *st = iio_priv(indio_dev);
switch (mask) {
case IIO_CHAN_INFO_RAW:
/* Take the iio_dev status lock */
- mutex_lock(&indio_dev->mlock);
- ret = adis16060_spi_write(indio_dev, chan->address);
+ mutex_lock(&st->buf_lock);
+ ret = adis16060_spi_write_than_read(indio_dev,
+ chan->address, &tval);
if (ret < 0)
- goto out_unlock;
+ mutex_unlock(&st->buf_lock);
+ return ret;
- ret = adis16060_spi_read(indio_dev, &tval);
- if (ret < 0)
- goto out_unlock;
-
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->buf_lock);
*val = tval;
return IIO_VAL_INT;
case IIO_CHAN_INFO_OFFSET:
@@ -110,10 +104,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
}
return -EINVAL;
-
-out_unlock:
- mutex_unlock(&indio_dev->mlock);
- return ret;
}
static const struct iio_info adis16060_info = {
--
2.7.4
[toc] | [next] | [standalone]
| From | Gargi Sharma <gs051095@gmail.com> |
|---|---|
| Date | 2017-03-19 18:20 +0100 |
| Subject | Re: [Outreachy kernel] [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code |
| Message-ID | <tmMEO-tl-23@gated-at.bofh.it> |
| In reply to | #1603969 |
On Sun, Mar 19, 2017 at 6:20 PM, simran singhal
<singhalsimran0@gmail.com> 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 protects both the adis16060_spi_write() and
> adis16060_spi_read() functions and both are always called in
> pair. First write, then read. Thus, refactor the code to have
> one single function adis16060_spi_write_than_read() which is
> protected by the existing buf_lock.
>
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> ---
>
> v5:
> -Rename val in adis16060_spi_write_than_read() to conf.
> -Rename val2 in adis16060_spi_write_than_read() to val.
> -Corrected Checkpatch issues.
> -Removed goto from adis16060_read_raw().
>
>
> drivers/staging/iio/gyro/adis16060_core.c | 42 ++++++++++++-------------------
> 1 file changed, 16 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..0f12492 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -40,25 +40,20 @@ struct adis16060_state {
>
> static struct iio_dev *adis16060_iio_dev;
>
> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
> + u8 conf, u16 *val)
> {
> int ret;
> struct adis16060_state *st = iio_priv(indio_dev);
>
> mutex_lock(&st->buf_lock);
> - st->buf[2] = val; /* The last 8 bits clocked in are latched */
> + st->buf[2] = conf; /* The last 8 bits clocked in are latched */
> ret = spi_write(st->us_w, st->buf, 3);
> - mutex_unlock(&st->buf_lock);
>
> - return ret;
> -}
> -
> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> -{
> - int ret;
> - struct adis16060_state *st = iio_priv(indio_dev);
> -
> - mutex_lock(&st->buf_lock);
> + if (ret < 0) {
> + mutex_unlock(&st->buf_lock);
> + return ret;
> + }
>
> ret = spi_read(st->us_r, st->buf, 3);
>
> @@ -69,8 +64,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> */
> if (!ret)
> *val = ((st->buf[0] & 0x3) << 12) |
> - (st->buf[1] << 4) |
> - ((st->buf[2] >> 4) & 0xF);
> + (st->buf[1] << 4) |
> + ((st->buf[2] >> 4) & 0xF);
> mutex_unlock(&st->buf_lock);
>
> return ret;
> @@ -83,20 +78,19 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> {
> u16 tval = 0;
> int ret;
> + struct adis16060_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> - ret = adis16060_spi_write(indio_dev, chan->address);
> + mutex_lock(&st->buf_lock);
> + ret = adis16060_spi_write_than_read(indio_dev,
> + chan->address, &tval);
> if (ret < 0)
> - goto out_unlock;
> + mutex_unlock(&st->buf_lock);
> + return ret;
>
> - ret = adis16060_spi_read(indio_dev, &tval);
> - if (ret < 0)
> - goto out_unlock;
> -
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->buf_lock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -110,10 +104,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> }
>
> return -EINVAL;
> -
> -out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> - return ret;
> }
>
Hey Simran,
I'm another Outreachy aspirant and I'm trying to work through a
similar patch in another driver. Can you please explain to me how you
are avoiding nested locks here? From what I understand, the function
adis16060_read_raw call a lock on &st->buf_lock and then you call the
function adis16060_spi_write_than_read which again tries to get hold
of the same lock. Isn't this a deadlock situation? Please let me know
if my understanding is incorrect.
Thank you!
Gargi
> static const struct iio_info adis16060_info = {
> --
> 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@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170319125039.GA23385%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2017-03-19 21:50 +0100 |
| Subject | Re: [Outreachy kernel] [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code |
| Message-ID | <tmPW2-2E6-13@gated-at.bofh.it> |
| In reply to | #1604053 |
On 19/03/17 17:14, Gargi Sharma wrote:
> On Sun, Mar 19, 2017 at 6:20 PM, simran singhal
> <singhalsimran0@gmail.com> 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 protects both the adis16060_spi_write() and
>> adis16060_spi_read() functions and both are always called in
>> pair. First write, then read. Thus, refactor the code to have
>> one single function adis16060_spi_write_than_read() which is
>> protected by the existing buf_lock.
>>
>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> ---
>>
>> v5:
>> -Rename val in adis16060_spi_write_than_read() to conf.
>> -Rename val2 in adis16060_spi_write_than_read() to val.
>> -Corrected Checkpatch issues.
>> -Removed goto from adis16060_read_raw().
>>
>>
>> drivers/staging/iio/gyro/adis16060_core.c | 42 ++++++++++++-------------------
>> 1 file changed, 16 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
>> index c9d46e7..0f12492 100644
>> --- a/drivers/staging/iio/gyro/adis16060_core.c
>> +++ b/drivers/staging/iio/gyro/adis16060_core.c
>> @@ -40,25 +40,20 @@ struct adis16060_state {
>>
>> static struct iio_dev *adis16060_iio_dev;
>>
>> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
>> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
>> + u8 conf, u16 *val)
>> {
>> int ret;
>> struct adis16060_state *st = iio_priv(indio_dev);
>>
>> mutex_lock(&st->buf_lock);
>> - st->buf[2] = val; /* The last 8 bits clocked in are latched */
>> + st->buf[2] = conf; /* The last 8 bits clocked in are latched */
>> ret = spi_write(st->us_w, st->buf, 3);
>> - mutex_unlock(&st->buf_lock);
>>
>> - return ret;
>> -}
>> -
>> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
>> -{
>> - int ret;
>> - struct adis16060_state *st = iio_priv(indio_dev);
>> -
>> - mutex_lock(&st->buf_lock);
>> + if (ret < 0) {
>> + mutex_unlock(&st->buf_lock);
>> + return ret;
>> + }
>>
>> ret = spi_read(st->us_r, st->buf, 3);
>>
>> @@ -69,8 +64,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
>> */
>> if (!ret)
>> *val = ((st->buf[0] & 0x3) << 12) |
>> - (st->buf[1] << 4) |
>> - ((st->buf[2] >> 4) & 0xF);
>> + (st->buf[1] << 4) |
>> + ((st->buf[2] >> 4) & 0xF);
>> mutex_unlock(&st->buf_lock);
>>
>> return ret;
>> @@ -83,20 +78,19 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>> {
>> u16 tval = 0;
>> int ret;
>> + struct adis16060_state *st = iio_priv(indio_dev);
>>
>> switch (mask) {
>> case IIO_CHAN_INFO_RAW:
>> /* Take the iio_dev status lock */
>> - mutex_lock(&indio_dev->mlock);
>> - ret = adis16060_spi_write(indio_dev, chan->address);
>> + mutex_lock(&st->buf_lock);
>> + ret = adis16060_spi_write_than_read(indio_dev,
>> + chan->address, &tval);
>> if (ret < 0)
>> - goto out_unlock;
>> + mutex_unlock(&st->buf_lock);
>> + return ret;
>>
>> - ret = adis16060_spi_read(indio_dev, &tval);
>> - if (ret < 0)
>> - goto out_unlock;
>> -
>> - mutex_unlock(&indio_dev->mlock);
>> + mutex_unlock(&st->buf_lock);
>> *val = tval;
>> return IIO_VAL_INT;
>> case IIO_CHAN_INFO_OFFSET:
>> @@ -110,10 +104,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>> }
>>
>> return -EINVAL;
>> -
>> -out_unlock:
>> - mutex_unlock(&indio_dev->mlock);
>> - return ret;
>> }
>>
>
> Hey Simran,
>
> I'm another Outreachy aspirant and I'm trying to work through a
> similar patch in another driver. Can you please explain to me how you
> are avoiding nested locks here? From what I understand, the function
> adis16060_read_raw call a lock on &st->buf_lock and then you call the
> function adis16060_spi_write_than_read which again tries to get hold
> of the same lock. Isn't this a deadlock situation? Please let me know
> if my understanding is incorrect.
Well spotted. That is indeed the case. Just goes to show how more
eyes on code is always a good thing!
The locks in read_raw itself should be dropped as we now have a single
safe function with the locks inside it being called.
Jonathan
>
> Thank you!
> Gargi
>
>> static const struct iio_info adis16060_info = {
>> --
>> 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@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170319125039.GA23385%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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[toc] | [prev] | [next] | [standalone]
| From | SIMRAN SINGHAL <singhalsimran0@gmail.com> |
|---|---|
| Date | 2017-03-19 22:20 +0100 |
| Subject | Re: [Outreachy kernel] [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code |
| Message-ID | <tmQp4-34f-5@gated-at.bofh.it> |
| In reply to | #1604085 |
On Mon, Mar 20, 2017 at 2:13 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 19/03/17 17:14, Gargi Sharma wrote:
>> On Sun, Mar 19, 2017 at 6:20 PM, simran singhal
>> <singhalsimran0@gmail.com> 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 protects both the adis16060_spi_write() and
>>> adis16060_spi_read() functions and both are always called in
>>> pair. First write, then read. Thus, refactor the code to have
>>> one single function adis16060_spi_write_than_read() which is
>>> protected by the existing buf_lock.
>>>
>>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>>> ---
>>>
>>> v5:
>>> -Rename val in adis16060_spi_write_than_read() to conf.
>>> -Rename val2 in adis16060_spi_write_than_read() to val.
>>> -Corrected Checkpatch issues.
>>> -Removed goto from adis16060_read_raw().
>>>
>>>
>>> drivers/staging/iio/gyro/adis16060_core.c | 42 ++++++++++++-------------------
>>> 1 file changed, 16 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
>>> index c9d46e7..0f12492 100644
>>> --- a/drivers/staging/iio/gyro/adis16060_core.c
>>> +++ b/drivers/staging/iio/gyro/adis16060_core.c
>>> @@ -40,25 +40,20 @@ struct adis16060_state {
>>>
>>> static struct iio_dev *adis16060_iio_dev;
>>>
>>> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
>>> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
>>> + u8 conf, u16 *val)
>>> {
>>> int ret;
>>> struct adis16060_state *st = iio_priv(indio_dev);
>>>
>>> mutex_lock(&st->buf_lock);
>>> - st->buf[2] = val; /* The last 8 bits clocked in are latched */
>>> + st->buf[2] = conf; /* The last 8 bits clocked in are latched */
>>> ret = spi_write(st->us_w, st->buf, 3);
>>> - mutex_unlock(&st->buf_lock);
>>>
>>> - return ret;
>>> -}
>>> -
>>> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
>>> -{
>>> - int ret;
>>> - struct adis16060_state *st = iio_priv(indio_dev);
>>> -
>>> - mutex_lock(&st->buf_lock);
>>> + if (ret < 0) {
>>> + mutex_unlock(&st->buf_lock);
>>> + return ret;
>>> + }
>>>
>>> ret = spi_read(st->us_r, st->buf, 3);
>>>
>>> @@ -69,8 +64,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
>>> */
>>> if (!ret)
>>> *val = ((st->buf[0] & 0x3) << 12) |
>>> - (st->buf[1] << 4) |
>>> - ((st->buf[2] >> 4) & 0xF);
>>> + (st->buf[1] << 4) |
>>> + ((st->buf[2] >> 4) & 0xF);
>>> mutex_unlock(&st->buf_lock);
>>>
>>> return ret;
>>> @@ -83,20 +78,19 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>>> {
>>> u16 tval = 0;
>>> int ret;
>>> + struct adis16060_state *st = iio_priv(indio_dev);
>>>
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> /* Take the iio_dev status lock */
>>> - mutex_lock(&indio_dev->mlock);
>>> - ret = adis16060_spi_write(indio_dev, chan->address);
>>> + mutex_lock(&st->buf_lock);
>>> + ret = adis16060_spi_write_than_read(indio_dev,
>>> + chan->address, &tval);
>>> if (ret < 0)
>>> - goto out_unlock;
>>> + mutex_unlock(&st->buf_lock);
>>> + return ret;
>>>
>>> - ret = adis16060_spi_read(indio_dev, &tval);
>>> - if (ret < 0)
>>> - goto out_unlock;
>>> -
>>> - mutex_unlock(&indio_dev->mlock);
>>> + mutex_unlock(&st->buf_lock);
>>> *val = tval;
>>> return IIO_VAL_INT;
>>> case IIO_CHAN_INFO_OFFSET:
>>> @@ -110,10 +104,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>>> }
>>>
>>> return -EINVAL;
>>> -
>>> -out_unlock:
>>> - mutex_unlock(&indio_dev->mlock);
>>> - return ret;
>>> }
>>>
>>
>> Hey Simran,
>>
>> I'm another Outreachy aspirant and I'm trying to work through a
>> similar patch in another driver. Can you please explain to me how you
>> are avoiding nested locks here? From what I understand, the function
>> adis16060_read_raw call a lock on &st->buf_lock and then you call the
>> function adis16060_spi_write_than_read which again tries to get hold
>> of the same lock. Isn't this a deadlock situation? Please let me know
>> if my understanding is incorrect.
> Well spotted. That is indeed the case. Just goes to show how more
> eyes on code is always a good thing!
>
Jonathan, I have already sent the version 6 of this patch in which I
have dropped the
locks in the function adis16060_spi_write_than_read and keep the locks
of function
read_raw as it is.
> The locks in read_raw itself should be dropped as we now have a single
> safe function with the locks inside it being called.
I keep the locks inside read_raw as it is because it will be more
safe, if we see in terms of
security. If I am wrong here, please correct me.
>
> Jonathan
>>
>> Thank you!
>> Gargi
>>
>>> static const struct iio_info adis16060_info = {
>>> --
>>> 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@googlegroups.com.
>>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170319125039.GA23385%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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
[toc] | [prev] | [next] | [standalone]
| From | SIMRAN SINGHAL <singhalsimran0@gmail.com> |
|---|---|
| Date | 2017-03-21 16:10 +0100 |
| Message-ID | <tntA7-4ZS-33@gated-at.bofh.it> |
| In reply to | #1603969 |
On Tue, Mar 21, 2017 at 8:01 PM, kbuild test robot <lkp@intel.com> wrote:
> Hi simran,
>
> [auto build test WARNING on iio/togreg]
> [also build test WARNING on v4.11-rc3 next-20170321]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/simran-singhal/staging-Use-buf_lock-instead-of-mlock-and-Refactor-code/20170321-213956
> base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: x86_64-randconfig-x016-201712 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All warnings (new ones prefixed by >>):
>
I am not getting any error, and I have already sent the v7 of this patch
please check out that:
https://groups.google.com/forum/#!topic/outreachy-kernel/5uz6IpcOpMA
> In file included from include/uapi/linux/stddef.h:1:0,
> from include/linux/stddef.h:4,
> from include/uapi/linux/posix_types.h:4,
> from include/uapi/linux/types.h:13,
> from include/linux/types.h:5,
> from include/linux/list.h:4,
> from include/linux/module.h:9,
> from drivers/staging/iio/gyro/adis16060_core.c:9:
> drivers/staging/iio/gyro/adis16060_core.c: In function 'adis16060_read_raw':
> include/linux/compiler.h:149:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
> if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
> ^
> include/linux/compiler.h:147:23: note: in expansion of macro '__trace_if'
> #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
> ^~~~~~~~~~
>>> drivers/staging/iio/gyro/adis16060_core.c:89:3: note: in expansion of macro 'if'
> if (ret < 0)
> ^~
> drivers/staging/iio/gyro/adis16060_core.c:91:4: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
> return ret;
> ^~~~~~
>
> vim +/if +89 drivers/staging/iio/gyro/adis16060_core.c
>
> e071f6b8 Barry Song 2010-10-27 3 *
> e071f6b8 Barry Song 2010-10-27 4 * Copyright 2010 Analog Devices Inc.
> e071f6b8 Barry Song 2010-10-27 5 *
> e071f6b8 Barry Song 2010-10-27 6 * Licensed under the GPL-2 or later.
> e071f6b8 Barry Song 2010-10-27 7 */
> e071f6b8 Barry Song 2010-10-27 8
> 45296236 Paul Gortmaker 2011-08-30 @9 #include <linux/module.h>
> e071f6b8 Barry Song 2010-10-27 10 #include <linux/delay.h>
> e071f6b8 Barry Song 2010-10-27 11 #include <linux/mutex.h>
> e071f6b8 Barry Song 2010-10-27 12 #include <linux/device.h>
> e071f6b8 Barry Song 2010-10-27 13 #include <linux/kernel.h>
> e071f6b8 Barry Song 2010-10-27 14 #include <linux/spi/spi.h>
> e071f6b8 Barry Song 2010-10-27 15 #include <linux/slab.h>
> e071f6b8 Barry Song 2010-10-27 16 #include <linux/sysfs.h>
> 14f98326 Jonathan Cameron 2011-02-28 17
> 06458e27 Jonathan Cameron 2012-04-25 18 #include <linux/iio/iio.h>
> 06458e27 Jonathan Cameron 2012-04-25 19 #include <linux/iio/sysfs.h>
> e071f6b8 Barry Song 2010-10-27 20
> 14f98326 Jonathan Cameron 2011-02-28 21 #define ADIS16060_GYRO 0x20 /* Measure Angular Rate (Gyro) */
> 14f98326 Jonathan Cameron 2011-02-28 22 #define ADIS16060_TEMP_OUT 0x10 /* Measure Temperature */
> 14f98326 Jonathan Cameron 2011-02-28 23 #define ADIS16060_AIN2 0x80 /* Measure AIN2 */
> 14f98326 Jonathan Cameron 2011-02-28 24 #define ADIS16060_AIN1 0x40 /* Measure AIN1 */
> 14f98326 Jonathan Cameron 2011-02-28 25
> 14f98326 Jonathan Cameron 2011-02-28 26 /**
> 14f98326 Jonathan Cameron 2011-02-28 27 * struct adis16060_state - device instance specific data
> 14f98326 Jonathan Cameron 2011-02-28 28 * @us_w: actual spi_device to write config
> 14f98326 Jonathan Cameron 2011-02-28 29 * @us_r: actual spi_device to read back data
> 25985edc Lucas De Marchi 2011-03-30 30 * @buf: transmit or receive buffer
> 14f98326 Jonathan Cameron 2011-02-28 31 * @buf_lock: mutex to protect tx and rx
> 14f98326 Jonathan Cameron 2011-02-28 32 **/
> 14f98326 Jonathan Cameron 2011-02-28 33 struct adis16060_state {
> 14f98326 Jonathan Cameron 2011-02-28 34 struct spi_device *us_w;
> 14f98326 Jonathan Cameron 2011-02-28 35 struct spi_device *us_r;
> 14f98326 Jonathan Cameron 2011-02-28 36 struct mutex buf_lock;
> 14f98326 Jonathan Cameron 2011-02-28 37
> 14f98326 Jonathan Cameron 2011-02-28 38 u8 buf[3] ____cacheline_aligned;
> 14f98326 Jonathan Cameron 2011-02-28 39 };
> e071f6b8 Barry Song 2010-10-27 40
> 3a5952f9 Jonathan Cameron 2011-06-27 41 static struct iio_dev *adis16060_iio_dev;
> e071f6b8 Barry Song 2010-10-27 42
> 71db16e5 simran singhal 2017-03-19 43 static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
> 71db16e5 simran singhal 2017-03-19 44 u8 conf, u16 *val)
> e071f6b8 Barry Song 2010-10-27 45 {
> e071f6b8 Barry Song 2010-10-27 46 int ret;
> 3a5952f9 Jonathan Cameron 2011-06-27 47 struct adis16060_state *st = iio_priv(indio_dev);
> e071f6b8 Barry Song 2010-10-27 48
> e071f6b8 Barry Song 2010-10-27 49 mutex_lock(&st->buf_lock);
> 71db16e5 simran singhal 2017-03-19 50 st->buf[2] = conf; /* The last 8 bits clocked in are latched */
> 14f98326 Jonathan Cameron 2011-02-28 51 ret = spi_write(st->us_w, st->buf, 3);
> e071f6b8 Barry Song 2010-10-27 52
> 71db16e5 simran singhal 2017-03-19 53 if (ret < 0) {
> 71db16e5 simran singhal 2017-03-19 54 mutex_unlock(&st->buf_lock);
> e071f6b8 Barry Song 2010-10-27 55 return ret;
> e071f6b8 Barry Song 2010-10-27 56 }
> e071f6b8 Barry Song 2010-10-27 57
> 14f98326 Jonathan Cameron 2011-02-28 58 ret = spi_read(st->us_r, st->buf, 3);
> e071f6b8 Barry Song 2010-10-27 59
> d14ae859 Jonathan Cameron 2011-02-11 60 /* The internal successive approximation ADC begins the
> d14ae859 Jonathan Cameron 2011-02-11 61 * conversion process on the falling edge of MSEL1 and
> d14ae859 Jonathan Cameron 2011-02-11 62 * starts to place data MSB first on the DOUT line at
> d14ae859 Jonathan Cameron 2011-02-11 63 * the 6th falling edge of SCLK
> e071f6b8 Barry Song 2010-10-27 64 */
> 05824120 Cristina Moraru 2015-10-20 65 if (!ret)
> 14f98326 Jonathan Cameron 2011-02-28 66 *val = ((st->buf[0] & 0x3) << 12) |
> 14f98326 Jonathan Cameron 2011-02-28 67 (st->buf[1] << 4) |
> 14f98326 Jonathan Cameron 2011-02-28 68 ((st->buf[2] >> 4) & 0xF);
> e071f6b8 Barry Song 2010-10-27 69 mutex_unlock(&st->buf_lock);
> e071f6b8 Barry Song 2010-10-27 70
> e071f6b8 Barry Song 2010-10-27 71 return ret;
> e071f6b8 Barry Song 2010-10-27 72 }
> e071f6b8 Barry Song 2010-10-27 73
> 4f2ca080 Jonathan Cameron 2011-08-12 74 static int adis16060_read_raw(struct iio_dev *indio_dev,
> 4f2ca080 Jonathan Cameron 2011-08-12 75 struct iio_chan_spec const *chan,
> 4f2ca080 Jonathan Cameron 2011-08-12 76 int *val, int *val2,
> 4f2ca080 Jonathan Cameron 2011-08-12 77 long mask)
> e071f6b8 Barry Song 2010-10-27 78 {
> 4f2ca080 Jonathan Cameron 2011-08-12 79 u16 tval = 0;
> 4f2ca080 Jonathan Cameron 2011-08-12 80 int ret;
> 71db16e5 simran singhal 2017-03-19 81 struct adis16060_state *st = iio_priv(indio_dev);
> e071f6b8 Barry Song 2010-10-27 82
> 4f2ca080 Jonathan Cameron 2011-08-12 83 switch (mask) {
> fbaff213 Jonathan Cameron 2012-04-15 84 case IIO_CHAN_INFO_RAW:
> e071f6b8 Barry Song 2010-10-27 85 /* Take the iio_dev status lock */
> 71db16e5 simran singhal 2017-03-19 86 mutex_lock(&st->buf_lock);
> 71db16e5 simran singhal 2017-03-19 87 ret = adis16060_spi_write_than_read(indio_dev,
> 71db16e5 simran singhal 2017-03-19 88 chan->address, &tval);
> 31a99443 Cristina Moraru 2015-10-25 @89 if (ret < 0)
> 71db16e5 simran singhal 2017-03-19 90 mutex_unlock(&st->buf_lock);
> 71db16e5 simran singhal 2017-03-19 91 return ret;
> 31a99443 Cristina Moraru 2015-10-25 92
>
> :::::: The code at line 89 was first introduced by commit
> :::::: 31a99443a53aa65cdaaaaa3bcd5ed685eec87419 staging: iio: adis16060_core: Fix error handling
>
> :::::: TO: Cristina Moraru <cristina.moraru09@gmail.com>
> :::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2017-03-21 20:40 +0100 |
| Subject | Re: [PATCH v5] staging: Use buf_lock instead of mlock and Refactor code |
| Message-ID | <tnxNo-7LG-23@gated-at.bofh.it> |
| In reply to | #1605696 |
On 21/03/17 15:05, SIMRAN SINGHAL wrote:
> On Tue, Mar 21, 2017 at 8:01 PM, kbuild test robot <lkp@intel.com> wrote:
>> Hi simran,
>>
>> [auto build test WARNING on iio/togreg]
>> [also build test WARNING on v4.11-rc3 next-20170321]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url: https://github.com/0day-ci/linux/commits/simran-singhal/staging-Use-buf_lock-instead-of-mlock-and-Refactor-code/20170321-213956
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
>> config: x86_64-randconfig-x016-201712 (attached as .config)
>> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
>> reproduce:
>> # save the attached .config to linux build tree
>> make ARCH=x86_64
>>
>> All warnings (new ones prefixed by >>):
>>
> I am not getting any error, and I have already sent the v7 of this patch
> please check out that:
> https://groups.google.com/forum/#!topic/outreachy-kernel/5uz6IpcOpMA
>
>
>> In file included from include/uapi/linux/stddef.h:1:0,
>> from include/linux/stddef.h:4,
>> from include/uapi/linux/posix_types.h:4,
>> from include/uapi/linux/types.h:13,
>> from include/linux/types.h:5,
>> from include/linux/list.h:4,
>> from include/linux/module.h:9,
>> from drivers/staging/iio/gyro/adis16060_core.c:9:
>> drivers/staging/iio/gyro/adis16060_core.c: In function 'adis16060_read_raw':
>> include/linux/compiler.h:149:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
>> if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
>> ^
>> include/linux/compiler.h:147:23: note: in expansion of macro '__trace_if'
>> #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
>> ^~~~~~~~~~
>>>> drivers/staging/iio/gyro/adis16060_core.c:89:3: note: in expansion of macro 'if'
>> if (ret < 0)
>> ^~
>> drivers/staging/iio/gyro/adis16060_core.c:91:4: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
>> return ret;
>> ^~~~~~
All this rather complex bit of magic is saying, is that the indenting on the return
is probably wrong. Which it isn't, but there is something else wrong that
would make the autobuilder think so. No brackets around the two lines that
should run if the if statement is true!
This is the autobuilder magic running directly on patches on the mailing list.
I force the equivalent tests by pushing patches out first in a testing branch,
so that this stuff can be fixed before patches go into a non rebasing branch
(fixing them there makes for a very messy history).
If the autobuilder bots have spare cycles, they tend to start running patches
off the mailing list - hence magic like this one, though the results can be
hard to read sometimes!
Jonathan
>>
>> vim +/if +89 drivers/staging/iio/gyro/adis16060_core.c
>>
>> e071f6b8 Barry Song 2010-10-27 3 *
>> e071f6b8 Barry Song 2010-10-27 4 * Copyright 2010 Analog Devices Inc.
>> e071f6b8 Barry Song 2010-10-27 5 *
>> e071f6b8 Barry Song 2010-10-27 6 * Licensed under the GPL-2 or later.
>> e071f6b8 Barry Song 2010-10-27 7 */
>> e071f6b8 Barry Song 2010-10-27 8
>> 45296236 Paul Gortmaker 2011-08-30 @9 #include <linux/module.h>
>> e071f6b8 Barry Song 2010-10-27 10 #include <linux/delay.h>
>> e071f6b8 Barry Song 2010-10-27 11 #include <linux/mutex.h>
>> e071f6b8 Barry Song 2010-10-27 12 #include <linux/device.h>
>> e071f6b8 Barry Song 2010-10-27 13 #include <linux/kernel.h>
>> e071f6b8 Barry Song 2010-10-27 14 #include <linux/spi/spi.h>
>> e071f6b8 Barry Song 2010-10-27 15 #include <linux/slab.h>
>> e071f6b8 Barry Song 2010-10-27 16 #include <linux/sysfs.h>
>> 14f98326 Jonathan Cameron 2011-02-28 17
>> 06458e27 Jonathan Cameron 2012-04-25 18 #include <linux/iio/iio.h>
>> 06458e27 Jonathan Cameron 2012-04-25 19 #include <linux/iio/sysfs.h>
>> e071f6b8 Barry Song 2010-10-27 20
>> 14f98326 Jonathan Cameron 2011-02-28 21 #define ADIS16060_GYRO 0x20 /* Measure Angular Rate (Gyro) */
>> 14f98326 Jonathan Cameron 2011-02-28 22 #define ADIS16060_TEMP_OUT 0x10 /* Measure Temperature */
>> 14f98326 Jonathan Cameron 2011-02-28 23 #define ADIS16060_AIN2 0x80 /* Measure AIN2 */
>> 14f98326 Jonathan Cameron 2011-02-28 24 #define ADIS16060_AIN1 0x40 /* Measure AIN1 */
>> 14f98326 Jonathan Cameron 2011-02-28 25
>> 14f98326 Jonathan Cameron 2011-02-28 26 /**
>> 14f98326 Jonathan Cameron 2011-02-28 27 * struct adis16060_state - device instance specific data
>> 14f98326 Jonathan Cameron 2011-02-28 28 * @us_w: actual spi_device to write config
>> 14f98326 Jonathan Cameron 2011-02-28 29 * @us_r: actual spi_device to read back data
>> 25985edc Lucas De Marchi 2011-03-30 30 * @buf: transmit or receive buffer
>> 14f98326 Jonathan Cameron 2011-02-28 31 * @buf_lock: mutex to protect tx and rx
>> 14f98326 Jonathan Cameron 2011-02-28 32 **/
>> 14f98326 Jonathan Cameron 2011-02-28 33 struct adis16060_state {
>> 14f98326 Jonathan Cameron 2011-02-28 34 struct spi_device *us_w;
>> 14f98326 Jonathan Cameron 2011-02-28 35 struct spi_device *us_r;
>> 14f98326 Jonathan Cameron 2011-02-28 36 struct mutex buf_lock;
>> 14f98326 Jonathan Cameron 2011-02-28 37
>> 14f98326 Jonathan Cameron 2011-02-28 38 u8 buf[3] ____cacheline_aligned;
>> 14f98326 Jonathan Cameron 2011-02-28 39 };
>> e071f6b8 Barry Song 2010-10-27 40
>> 3a5952f9 Jonathan Cameron 2011-06-27 41 static struct iio_dev *adis16060_iio_dev;
>> e071f6b8 Barry Song 2010-10-27 42
>> 71db16e5 simran singhal 2017-03-19 43 static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
>> 71db16e5 simran singhal 2017-03-19 44 u8 conf, u16 *val)
>> e071f6b8 Barry Song 2010-10-27 45 {
>> e071f6b8 Barry Song 2010-10-27 46 int ret;
>> 3a5952f9 Jonathan Cameron 2011-06-27 47 struct adis16060_state *st = iio_priv(indio_dev);
>> e071f6b8 Barry Song 2010-10-27 48
>> e071f6b8 Barry Song 2010-10-27 49 mutex_lock(&st->buf_lock);
>> 71db16e5 simran singhal 2017-03-19 50 st->buf[2] = conf; /* The last 8 bits clocked in are latched */
>> 14f98326 Jonathan Cameron 2011-02-28 51 ret = spi_write(st->us_w, st->buf, 3);
>> e071f6b8 Barry Song 2010-10-27 52
>> 71db16e5 simran singhal 2017-03-19 53 if (ret < 0) {
>> 71db16e5 simran singhal 2017-03-19 54 mutex_unlock(&st->buf_lock);
>> e071f6b8 Barry Song 2010-10-27 55 return ret;
>> e071f6b8 Barry Song 2010-10-27 56 }
>> e071f6b8 Barry Song 2010-10-27 57
>> 14f98326 Jonathan Cameron 2011-02-28 58 ret = spi_read(st->us_r, st->buf, 3);
>> e071f6b8 Barry Song 2010-10-27 59
>> d14ae859 Jonathan Cameron 2011-02-11 60 /* The internal successive approximation ADC begins the
>> d14ae859 Jonathan Cameron 2011-02-11 61 * conversion process on the falling edge of MSEL1 and
>> d14ae859 Jonathan Cameron 2011-02-11 62 * starts to place data MSB first on the DOUT line at
>> d14ae859 Jonathan Cameron 2011-02-11 63 * the 6th falling edge of SCLK
>> e071f6b8 Barry Song 2010-10-27 64 */
>> 05824120 Cristina Moraru 2015-10-20 65 if (!ret)
>> 14f98326 Jonathan Cameron 2011-02-28 66 *val = ((st->buf[0] & 0x3) << 12) |
>> 14f98326 Jonathan Cameron 2011-02-28 67 (st->buf[1] << 4) |
>> 14f98326 Jonathan Cameron 2011-02-28 68 ((st->buf[2] >> 4) & 0xF);
>> e071f6b8 Barry Song 2010-10-27 69 mutex_unlock(&st->buf_lock);
>> e071f6b8 Barry Song 2010-10-27 70
>> e071f6b8 Barry Song 2010-10-27 71 return ret;
>> e071f6b8 Barry Song 2010-10-27 72 }
>> e071f6b8 Barry Song 2010-10-27 73
>> 4f2ca080 Jonathan Cameron 2011-08-12 74 static int adis16060_read_raw(struct iio_dev *indio_dev,
>> 4f2ca080 Jonathan Cameron 2011-08-12 75 struct iio_chan_spec const *chan,
>> 4f2ca080 Jonathan Cameron 2011-08-12 76 int *val, int *val2,
>> 4f2ca080 Jonathan Cameron 2011-08-12 77 long mask)
>> e071f6b8 Barry Song 2010-10-27 78 {
>> 4f2ca080 Jonathan Cameron 2011-08-12 79 u16 tval = 0;
>> 4f2ca080 Jonathan Cameron 2011-08-12 80 int ret;
>> 71db16e5 simran singhal 2017-03-19 81 struct adis16060_state *st = iio_priv(indio_dev);
>> e071f6b8 Barry Song 2010-10-27 82
>> 4f2ca080 Jonathan Cameron 2011-08-12 83 switch (mask) {
>> fbaff213 Jonathan Cameron 2012-04-15 84 case IIO_CHAN_INFO_RAW:
>> e071f6b8 Barry Song 2010-10-27 85 /* Take the iio_dev status lock */
>> 71db16e5 simran singhal 2017-03-19 86 mutex_lock(&st->buf_lock);
>> 71db16e5 simran singhal 2017-03-19 87 ret = adis16060_spi_write_than_read(indio_dev,
>> 71db16e5 simran singhal 2017-03-19 88 chan->address, &tval);
>> 31a99443 Cristina Moraru 2015-10-25 @89 if (ret < 0)
>> 71db16e5 simran singhal 2017-03-19 90 mutex_unlock(&st->buf_lock);
>> 71db16e5 simran singhal 2017-03-19 91 return ret;
>> 31a99443 Cristina Moraru 2015-10-25 92
>>
>> :::::: The code at line 89 was first introduced by commit
>> :::::: 31a99443a53aa65cdaaaaa3bcd5ed685eec87419 staging: iio: adis16060_core: Fix error handling
>>
>> :::::: TO: Cristina Moraru <cristina.moraru09@gmail.com>
>> :::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> ---
>> 0-DAY kernel test infrastructure Open Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all Intel Corporation
> --
> 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
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web