[PATCH] iio:accel:kxsd9 fix missing mutex unlock

[PATCH] iio:accel:kxsd9 fix missing mutex unlock

[Jonathan Cameron]

From: Frank Zago <frank@zago.net>

This will leave a lock held after reading from the device, preventing
any further reads.

Signed-off-by: Frank Zago <frank@zago.net>
Signed-off-by: Jonathan Cameron <jic23@kernel.org>
---
I have reworked this from a patch sent to me by Frank and put it
into the correct format.

Frank, do you mind me adding your sign off on this. It's shorter
than your patch, but the effect is the same.  This approach of unlocking
before checking a return value is a common kernel idiom.

Jonathan

 drivers/iio/accel/kxsd9.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
index d72118d1189c..12df91986dd2 100644
--- a/drivers/iio/accel/kxsd9.c
+++ b/drivers/iio/accel/kxsd9.c
@@ -112,6 +112,7 @@ static int kxsd9_read(struct iio_dev *indio_dev, u8 address)
 	mutex_lock(&st->buf_lock);
 	st->tx[0] = KXSD9_READ(address);
 	ret = spi_sync_transfer(st->us, xfers, ARRAY_SIZE(xfers));
+	mutex_unlock(&st->buf_lock);
 	if (ret)
 		return ret;
 	return (((u16)(st->rx[0])) << 8) | (st->rx[1] & 0xF0);
-- 
1.8.4.2

Re: [PATCH] iio:accel:kxsd9 fix missing mutex unlock

[Lars-Peter Clausen]

On 11/13/2013 09:27 PM, Jonathan Cameron wrote:
> From: Frank Zago <frank@zago.net>
>
> This will leave a lock held after reading from the device, preventing
> any further reads.
>
> Signed-off-by: Frank Zago <frank@zago.net>
> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
> ---
> I have reworked this from a patch sent to me by Frank and put it
> into the correct format.
>
> Frank, do you mind me adding your sign off on this. It's shorter
> than your patch, but the effect is the same.  This approach of unlocking
> before checking a return value is a common kernel idiom.
>
> Jonathan
>
>   drivers/iio/accel/kxsd9.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
> index d72118d1189c..12df91986dd2 100644
> --- a/drivers/iio/accel/kxsd9.c
> +++ b/drivers/iio/accel/kxsd9.c
> @@ -112,6 +112,7 @@ static int kxsd9_read(struct iio_dev *indio_dev, u8 address)
>   	mutex_lock(&st->buf_lock);
>   	st->tx[0] = KXSD9_READ(address);
>   	ret = spi_sync_transfer(st->us, xfers, ARRAY_SIZE(xfers));
> +	mutex_unlock(&st->buf_lock);
>   	if (ret)
>   		return ret;
>   	return (((u16)(st->rx[0])) << 8) | (st->rx[1] & 0xF0);

I think you'll need to hold the lock until you've actually read the data from 
the buffer.

- Lars

Re: [PATCH] iio:accel:kxsd9 fix missing mutex unlock

[Jonathan Cameron]

On 11/13/13 20:39, Lars-Peter Clausen wrote:
> On 11/13/2013 09:27 PM, Jonathan Cameron wrote:
>> From: Frank Zago <frank@zago.net>
>>
>> This will leave a lock held after reading from the device, preventing
>> any further reads.
>>
>> Signed-off-by: Frank Zago <frank@zago.net>
>> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>> I have reworked this from a patch sent to me by Frank and put it
>> into the correct format.
>>
>> Frank, do you mind me adding your sign off on this. It's shorter
>> than your patch, but the effect is the same.  This approach of unlocking
>> before checking a return value is a common kernel idiom.
>>
>> Jonathan
>>
>>   drivers/iio/accel/kxsd9.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/iio/accel/kxsd9.c b/drivers/iio/accel/kxsd9.c
>> index d72118d1189c..12df91986dd2 100644
>> --- a/drivers/iio/accel/kxsd9.c
>> +++ b/drivers/iio/accel/kxsd9.c
>> @@ -112,6 +112,7 @@ static int kxsd9_read(struct iio_dev *indio_dev, u8 address)
>>       mutex_lock(&st->buf_lock);
>>       st->tx[0] = KXSD9_READ(address);
>>       ret = spi_sync_transfer(st->us, xfers, ARRAY_SIZE(xfers));
>> +    mutex_unlock(&st->buf_lock);
>>       if (ret)
>>           return ret;
>>       return (((u16)(st->rx[0])) << 8) | (st->rx[1] & 0xF0);
> 
> I think you'll need to hold the lock until you've actually read the data from the buffer.
Good point, I wasn't thinking through what it was actually for.  Frank was in fact much more correct. I'll post his
version in a sec.
> 
> - Lars
> 
> -- 
> 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