linux-kernel - Re: [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <69bc7ae4-218c-3952-0105-e16c665e3d6f@kernel.org>
Date:	Sat, 11 Jun 2016 17:03:41 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Peter Meerwald-Stadler <pmeerw@...erw.net>,
	Alison Schofield <amsfield22@...il.com>
Cc:	knaack.h@....de, lars@...afoo.de, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: light: tcs3472: use iio helper function to guarantee
 direct mode

On 07/06/16 06:23, Peter Meerwald-Stadler wrote:
> On Mon, 6 Jun 2016, Alison Schofield wrote:
> 
>> Replace the code that guarantees the device stays in direct mode
>> with iio_device_claim_direct_mode() which does same.  This allows
>> removal of an unused lock in the device private global data.
> 
> Acked-by: Peter Meerwald-Stadler <pmeerw@...erw.net>
>  
>> Signed-off-by: Alison Schofield <amsfield22@...il.com>
>> Cc: Daniel Baluta <daniel.baluta@...il.com>
There is an ever so slightly difference in how these all work after
the change.  They will return -EBUSY rather than holding then returning
a valid value under the circumstances of two reads coming through
sysfs at the same time.  This is a pretty obscure case so
I think we are OK with this.

Actually pending responses to this, I'm going to back out the other
two similar patches.  Just goes to show, that if you show someone
something 3 times they might finally notice the issue!


>> ---
>>  drivers/iio/light/tcs3472.c | 13 +++++--------
>>  1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
>> index 1b530bf..b29312f 100644
>> --- a/drivers/iio/light/tcs3472.c
>> +++ b/drivers/iio/light/tcs3472.c
>> @@ -52,7 +52,6 @@
>>  
>>  struct tcs3472_data {
>>  	struct i2c_client *client;
>> -	struct mutex lock;
>>  	u8 enable;
>>  	u8 control;
>>  	u8 atime;
>> @@ -117,17 +116,16 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
>>  
>>  	switch (mask) {
>>  	case IIO_CHAN_INFO_RAW:
>> -		if (iio_buffer_enabled(indio_dev))
>> -			return -EBUSY;
>> -
>> -		mutex_lock(&data->lock);
>> +		ret = iio_device_claim_direct_mode(indio_dev);
>> +		if (ret)
>> +			return ret;
>>  		ret = tcs3472_req_data(data);
>>  		if (ret < 0) {
>> -			mutex_unlock(&data->lock);
>> +			iio_device_release_direct_mode(indio_dev);
>>  			return ret;
>>  		}
>>  		ret = i2c_smbus_read_word_data(data->client, chan->address);
>> -		mutex_unlock(&data->lock);
>> +		iio_device_release_direct_mode(indio_dev);
>>  		if (ret < 0)
>>  			return ret;
>>  		*val = ret;
>> @@ -263,7 +261,6 @@ static int tcs3472_probe(struct i2c_client *client,
>>  	data = iio_priv(indio_dev);
>>  	i2c_set_clientdata(client, indio_dev);
>>  	data->client = client;
>> -	mutex_init(&data->lock);
>>  
>>  	indio_dev->dev.parent = &client->dev;
>>  	indio_dev->info = &tcs3472_info;
>>
>