iio: dht11 Updates

iio: dht11 Updates

[Richard Weinberger]

Please see my current patches for your driver.
As discussed in an earlier mail I'm testing with the DHT22 sensor only.
With the IRQ changes I see 84 edges.

I have also a question on your driver. Why you increment
DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?

        threshold = DHT11_DATA_BIT_HIGH / timeres;
        if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
                pr_err("dht11: WARNING: decoding ambiguous\n");

Thanks,
//richard

[PATCH 1/4] iio: dht11: Add locking
[PATCH 2/4] iio: dht11: IRQ fixes
[PATCH 3/4] iio: dht11: Logging updates
[PATCH 4/4] iio: dht11: Fix out-of-bounds read

[PATCH 1/4] iio: dht11: Add locking

[Richard Weinberger]

Make sure that the read function is not interrupted...

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/iio/humidity/dht11.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index d8771f5..168ebc4 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -29,6 +29,7 @@
 #include <linux/wait.h>
 #include <linux/bitops.h>
 #include <linux/completion.h>
+#include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
@@ -57,6 +58,7 @@ struct dht11 {
 	int				irq;
 
 	struct completion		completion;
+	struct mutex			lock;
 
 	s64				timestamp;
 	int				temperature;
@@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 	struct dht11 *dht11 = iio_priv(iio_dev);
 	int ret;
 
+	mutex_lock(&dht11->lock);
 	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
 		reinit_completion(&dht11->completion);
 
@@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		ret = -EINVAL;
 err:
 	dht11->num_edges = -1;
+	mutex_unlock(&dht11->lock);
 	return ret;
 }
 
@@ -268,6 +272,7 @@ static int dht11_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, iio);
 
 	init_completion(&dht11->completion);
+	mutex_init(&dht11->lock);
 	iio->name = pdev->name;
 	iio->dev.parent = &pdev->dev;
 	iio->info = &dht11_iio_info;
-- 
1.8.4.5

Re: [PATCH 1/4] iio: dht11: Add locking

[Hartmut Knaack]

Richard Weinberger schrieb am 03.12.2014 um 00:32:
> Make sure that the read function is not interrupted...
There is already a mutex iio_dev->info_exist_lock used to serialize
iio_channel_read(), which in turn accesses _read_raw(). See [1].

[1]http://lxr.free-electrons.com/ident?i=iio_channel_read
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/iio/humidity/dht11.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index d8771f5..168ebc4 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -29,6 +29,7 @@
>  #include <linux/wait.h>
>  #include <linux/bitops.h>
>  #include <linux/completion.h>
> +#include <linux/mutex.h>
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
> @@ -57,6 +58,7 @@ struct dht11 {
>  	int				irq;
>  
>  	struct completion		completion;
> +	struct mutex			lock;
>  
>  	s64				timestamp;
>  	int				temperature;
> @@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  	struct dht11 *dht11 = iio_priv(iio_dev);
>  	int ret;
>  
> +	mutex_lock(&dht11->lock);
>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>  		reinit_completion(&dht11->completion);
>  
> @@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		ret = -EINVAL;
>  err:
>  	dht11->num_edges = -1;
> +	mutex_unlock(&dht11->lock);
>  	return ret;
>  }
>  
> @@ -268,6 +272,7 @@ static int dht11_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, iio);
>  
>  	init_completion(&dht11->completion);
> +	mutex_init(&dht11->lock);
>  	iio->name = pdev->name;
>  	iio->dev.parent = &pdev->dev;
>  	iio->info = &dht11_iio_info;
> 

Re: [PATCH 1/4] iio: dht11: Add locking

[Richard Weinberger]

Am 14.12.2014 um 13:31 schrieb Hartmut Knaack:
> Richard Weinberger schrieb am 03.12.2014 um 00:32:
>> Make sure that the read function is not interrupted...
> There is already a mutex iio_dev->info_exist_lock used to serialize
> iio_channel_read(), which in turn accesses _read_raw(). See [1].
> 
> [1]http://lxr.free-electrons.com/ident?i=iio_channel_read

This is only for the in-kernel API and not for sysfs access.
I.e. if you access via sysfs it is not protected.

Thanks,
//richard

>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  drivers/iio/humidity/dht11.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>> index d8771f5..168ebc4 100644
>> --- a/drivers/iio/humidity/dht11.c
>> +++ b/drivers/iio/humidity/dht11.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/wait.h>
>>  #include <linux/bitops.h>
>>  #include <linux/completion.h>
>> +#include <linux/mutex.h>
>>  #include <linux/delay.h>
>>  #include <linux/gpio.h>
>>  #include <linux/of_gpio.h>
>> @@ -57,6 +58,7 @@ struct dht11 {
>>  	int				irq;
>>  
>>  	struct completion		completion;
>> +	struct mutex			lock;
>>  
>>  	s64				timestamp;
>>  	int				temperature;
>> @@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>  	struct dht11 *dht11 = iio_priv(iio_dev);
>>  	int ret;
>>  
>> +	mutex_lock(&dht11->lock);
>>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>>  		reinit_completion(&dht11->completion);
>>  
>> @@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>  		ret = -EINVAL;
>>  err:
>>  	dht11->num_edges = -1;
>> +	mutex_unlock(&dht11->lock);
>>  	return ret;
>>  }
>>  
>> @@ -268,6 +272,7 @@ static int dht11_probe(struct platform_device *pdev)
>>  	platform_set_drvdata(pdev, iio);
>>  
>>  	init_completion(&dht11->completion);
>> +	mutex_init(&dht11->lock);
>>  	iio->name = pdev->name;
>>  	iio->dev.parent = &pdev->dev;
>>  	iio->info = &dht11_iio_info;
>>
> 

[PATCH 2/4] iio: dht11: IRQ fixes

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/iio/humidity/dht11.c | 56 ++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 168ebc4..0023699 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -140,6 +140,27 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	return 0;
 }
 
+/*
+ * IRQ handler called on GPIO edges
+ */
+static irqreturn_t dht11_handle_irq(int irq, void *data)
+{
+	struct iio_dev *iio = data;
+	struct dht11 *dht11 = iio_priv(iio);
+
+	/* TODO: Consider making the handler safe for IRQ sharing */
+	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
+		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
+		dht11->edges[dht11->num_edges++].value =
+						gpio_get_value(dht11->gpio);
+
+		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
+			complete(&dht11->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int dht11_read_raw(struct iio_dev *iio_dev,
 			const struct iio_chan_spec *chan,
 			int *val, int *val2, long m)
@@ -160,8 +181,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		if (ret)
 			goto err;
 
+		ret = request_irq(dht11->irq, dht11_handle_irq,
+				  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				  iio_dev->name, iio_dev);
+		if (ret)
+			goto err;
+
 		ret = wait_for_completion_killable_timeout(&dht11->completion,
 								 HZ);
+
+		free_irq(dht11->irq, iio_dev);
+
 		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
 			dev_err(&iio_dev->dev,
 					"Only %d signal edges detected\n",
@@ -197,27 +227,6 @@ static const struct iio_info dht11_iio_info = {
 	.read_raw		= dht11_read_raw,
 };
 
-/*
- * IRQ handler called on GPIO edges
-*/
-static irqreturn_t dht11_handle_irq(int irq, void *data)
-{
-	struct iio_dev *iio = data;
-	struct dht11 *dht11 = iio_priv(iio);
-
-	/* TODO: Consider making the handler safe for IRQ sharing */
-	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
-		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
-		dht11->edges[dht11->num_edges++].value =
-						gpio_get_value(dht11->gpio);
-
-		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
-			complete(&dht11->completion);
-	}
-
-	return IRQ_HANDLED;
-}
-
 static const struct iio_chan_spec dht11_chan_spec[] = {
 	{ .type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
@@ -260,11 +269,6 @@ static int dht11_probe(struct platform_device *pdev)
 		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
 		return -EINVAL;
 	}
-	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
-				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-				pdev->name, iio);
-	if (ret)
-		return ret;
 
 	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
-- 
1.8.4.5

Re: [PATCH 2/4] iio: dht11: IRQ fixes

[Richard Weinberger]

Am 03.12.2014 um 00:32 schrieb Richard Weinberger:

<insert commit message here> :-\

> Signed-off-by: Richard Weinberger <richard@nod.at>

Re: [PATCH 2/4] iio: dht11: IRQ fixes

[harald]

Hi Richard,

finally got around to test this patch on all HW I have.

As expected the preamble needs to be shortend by two edges:
With your patch in its current form, the driver stops to work
reliably with DHT11. Also with DHT22 you get some delay when
reading the data, because you always wait for the timeout to
happen, before trying to decode the data.

Since your patch title includes "fix", the commit message
probably should mention that the patch as a side effect
changes behaviour - even if it's just diagnostic messages.

Thanks,
Harald

On Wed,  3 Dec 2014 00:32:54 +0100, Richard Weinberger <richard@nod.at>
wrote:
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/iio/humidity/dht11.c | 56
>  ++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 168ebc4..0023699 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -140,6 +140,27 @@ static int dht11_decode(struct dht11 *dht11, int
> offset)
>  	return 0;
>  }
>  
> +/*
> + * IRQ handler called on GPIO edges
> + */
> +static irqreturn_t dht11_handle_irq(int irq, void *data)
> +{
> +	struct iio_dev *iio = data;
> +	struct dht11 *dht11 = iio_priv(iio);
> +
> +	/* TODO: Consider making the handler safe for IRQ sharing */
> +	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0)
{
> +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> +		dht11->edges[dht11->num_edges++].value =
> +						gpio_get_value(dht11->gpio);
> +
> +		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
> +			complete(&dht11->completion);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int dht11_read_raw(struct iio_dev *iio_dev,
>  			const struct iio_chan_spec *chan,
>  			int *val, int *val2, long m)
> @@ -160,8 +181,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		if (ret)
>  			goto err;
>  
> +		ret = request_irq(dht11->irq, dht11_handle_irq,
> +				  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				  iio_dev->name, iio_dev);
> +		if (ret)
> +			goto err;
> +
>  		ret = wait_for_completion_killable_timeout(&dht11->completion,
>  								 HZ);
> +
> +		free_irq(dht11->irq, iio_dev);
> +
>  		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
>  			dev_err(&iio_dev->dev,
>  					"Only %d signal edges detected\n",
> @@ -197,27 +227,6 @@ static const struct iio_info dht11_iio_info = {
>  	.read_raw		= dht11_read_raw,
>  };
>  
> -/*
> - * IRQ handler called on GPIO edges
> -*/
> -static irqreturn_t dht11_handle_irq(int irq, void *data)
> -{
> -	struct iio_dev *iio = data;
> -	struct dht11 *dht11 = iio_priv(iio);
> -
> -	/* TODO: Consider making the handler safe for IRQ sharing */
> -	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0)
{
> -		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> -		dht11->edges[dht11->num_edges++].value =
> -						gpio_get_value(dht11->gpio);
> -
> -		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
> -			complete(&dht11->completion);
> -	}
> -
> -	return IRQ_HANDLED;
> -}
> -
>  static const struct iio_chan_spec dht11_chan_spec[] = {
>  	{ .type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
> @@ -260,11 +269,6 @@ static int dht11_probe(struct platform_device
*pdev)
>  		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
>  		return -EINVAL;
>  	}
> -	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
> -				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> -				pdev->name, iio);
> -	if (ret)
> -		return ret;
>  
>  	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>  	dht11->num_edges = -1;

[PATCH 3/4] iio: dht11: Logging updates

[Richard Weinberger]

Currently the driver uses pr_* and dev_* functions.
Change all logging functions to dev_* style to be consistent
and have the correct device prefix in all messages.
This change set also adds new messages to diagnose issues.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/iio/humidity/dht11.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 0023699..fbcd7cb 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 			timeres = t;
 	}
 	if (2*timeres > DHT11_DATA_BIT_HIGH) {
-		pr_err("dht11: timeresolution %d too bad for decoding\n",
+		dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
 			timeres);
 		return -EIO;
 	}
 	threshold = DHT11_DATA_BIT_HIGH / timeres;
 	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
-		pr_err("dht11: WARNING: decoding ambiguous\n");
+		dev_err(dht11->dev, "decoding ambiguous\n");
 
 	/* scale down with timeres and check validity */
 	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
 		t = dht11->edges[offset + 2*i + 2].ts -
 			dht11->edges[offset + 2*i + 1].ts;
-		if (!dht11->edges[offset + 2*i + 1].value)
-			return -EIO;  /* lost synchronisation */
+		if (!dht11->edges[offset + 2*i + 1].value) {
+			dev_err(dht11->dev, "lost synchronisation\n");
+			return -EIO;
+		}
 		timing[i] = t / timeres;
 	}
 
@@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	temp_dec = dht11_decode_byte(&timing[24], threshold);
 	checksum = dht11_decode_byte(&timing[32], threshold);
 
-	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
+	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
+		dev_err(dht11->dev, "invalid checksum\n");
 		return -EIO;
+	}
 
 	dht11->timestamp = iio_get_time_ns();
 	if (hum_int < 20) {  /* DHT22 */
@@ -193,7 +197,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		free_irq(dht11->irq, iio_dev);
 
 		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
-			dev_err(&iio_dev->dev,
+			dev_err(dht11->dev,
 					"Only %d signal edges detected\n",
 					dht11->num_edges);
 			ret = -ETIMEDOUT;
-- 
1.8.4.5

Re: [PATCH 3/4] iio: dht11: Logging updates

[Harald Geyer]

Richard Weinberger writes:
> Currently the driver uses pr_* and dev_* functions.
> Change all logging functions to dev_* style to be consistent
> and have the correct device prefix in all messages.

Yes, actually this was on purpose:
The dev_ messages are really about something wrong with the device.
Ie if something goes wrong with one device but could perfectly work
with some other device.
The pr_ messages OTOH are about something wrong with clock resolution,
etc that would affect any DHT11 sensor on the system. Ideally we would
notice these things during probe() and just return with an error there.
Right now we aren't as clever as that, so we just log an error message
about the driver, when actually we are reading the device.

That said, I don't have strong feelings about this. If you want to
change this, I won't object. However if you really want to fix this,
then the proper thing would be to check for this conditions in
probe().

> This change set also adds new messages to diagnose issues.

Comment below.

> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/iio/humidity/dht11.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 0023699..fbcd7cb 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  			timeres = t;
>  	}
>  	if (2*timeres > DHT11_DATA_BIT_HIGH) {
> -		pr_err("dht11: timeresolution %d too bad for decoding\n",
> +		dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
>  			timeres);
>  		return -EIO;
>  	}
>  	threshold = DHT11_DATA_BIT_HIGH / timeres;
>  	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> -		pr_err("dht11: WARNING: decoding ambiguous\n");
> +		dev_err(dht11->dev, "decoding ambiguous\n");
>  
>  	/* scale down with timeres and check validity */
>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>  		t = dht11->edges[offset + 2*i + 2].ts -
>  			dht11->edges[offset + 2*i + 1].ts;
> -		if (!dht11->edges[offset + 2*i + 1].value)
> -			return -EIO;  /* lost synchronisation */
> +		if (!dht11->edges[offset + 2*i + 1].value) {
> +			dev_err(dht11->dev, "lost synchronisation\n");
> +			return -EIO;
> +		}

Are you sure this warrants a log message? I don't think this provides
much information. The userspace application should just try reading
the sensor again.

We could do someting smart and try to recover from such errors, but
ultimately userspace will need to deal with failed sensor communication
anyway, so I don't see the point.

>  		timing[i] = t / timeres;
>  	}
>  
> @@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	temp_dec = dht11_decode_byte(&timing[24], threshold);
>  	checksum = dht11_decode_byte(&timing[32], threshold);
>  
> -	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
> +		dev_err(dht11->dev, "invalid checksum\n");
>  		return -EIO;
> +	}

Same thing here.

>  	dht11->timestamp = iio_get_time_ns();
>  	if (hum_int < 20) {  /* DHT22 */
> @@ -193,7 +197,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		free_irq(dht11->irq, iio_dev);
>  
>  		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
> -			dev_err(&iio_dev->dev,
> +			dev_err(dht11->dev,

Ack.

Thanks,
Harald

>  					"Only %d signal edges detected\n",
>  					dht11->num_edges);
>  			ret = -ETIMEDOUT;
> -- 
> 1.8.4.5
> 

Re: [PATCH 3/4] iio: dht11: Logging updates

[Richard Weinberger]

Am 03.12.2014 um 13:58 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Currently the driver uses pr_* and dev_* functions.
>> Change all logging functions to dev_* style to be consistent
>> and have the correct device prefix in all messages.
> 
> Yes, actually this was on purpose:
> The dev_ messages are really about something wrong with the device.
> Ie if something goes wrong with one device but could perfectly work
> with some other device.
> The pr_ messages OTOH are about something wrong with clock resolution,
> etc that would affect any DHT11 sensor on the system. Ideally we would
> notice these things during probe() and just return with an error there.
> Right now we aren't as clever as that, so we just log an error message
> about the driver, when actually we are reading the device.
> 
> That said, I don't have strong feelings about this. If you want to
> change this, I won't object. However if you really want to fix this,
> then the proper thing would be to check for this conditions in
> probe().

Currently we get log messages of style:
"iio iio:deviceX: foo bar"
and:
"dht11 <name in DT>: foo bar"

I really favorite "dht11 <name in DT".
In my device tree every senor has a sane name and log messages look like:
"dht11 toiletten sensor: invalid checksum"


>> This change set also adds new messages to diagnose issues.
> 
> Comment below.
> 
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  drivers/iio/humidity/dht11.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>> index 0023699..fbcd7cb 100644
>> --- a/drivers/iio/humidity/dht11.c
>> +++ b/drivers/iio/humidity/dht11.c
>> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>  			timeres = t;
>>  	}
>>  	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>> -		pr_err("dht11: timeresolution %d too bad for decoding\n",
>> +		dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
>>  			timeres);
>>  		return -EIO;
>>  	}
>>  	threshold = DHT11_DATA_BIT_HIGH / timeres;
>>  	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>> -		pr_err("dht11: WARNING: decoding ambiguous\n");
>> +		dev_err(dht11->dev, "decoding ambiguous\n");
>>  
>>  	/* scale down with timeres and check validity */
>>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>>  		t = dht11->edges[offset + 2*i + 2].ts -
>>  			dht11->edges[offset + 2*i + 1].ts;
>> -		if (!dht11->edges[offset + 2*i + 1].value)
>> -			return -EIO;  /* lost synchronisation */
>> +		if (!dht11->edges[offset + 2*i + 1].value) {
>> +			dev_err(dht11->dev, "lost synchronisation\n");
>> +			return -EIO;
>> +		}
> 
> Are you sure this warrants a log message? I don't think this provides
> much information. The userspace application should just try reading
> the sensor again.
> 
> We could do someting smart and try to recover from such errors, but
> ultimately userspace will need to deal with failed sensor communication
> anyway, so I don't see the point.

The sensors are rather flaky and if they go nuts the user/developer maybe wants
to know why.
>From a plain EIO she has no chance to find out. He'll have to add printk()s by hand
to find out.
With a log message one can start digging into the issue.

>>  		timing[i] = t / timeres;
>>  	}
>>  
>> @@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>  	temp_dec = dht11_decode_byte(&timing[24], threshold);
>>  	checksum = dht11_decode_byte(&timing[32], threshold);
>>  
>> -	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
>> +		dev_err(dht11->dev, "invalid checksum\n");
>>  		return -EIO;
>> +	}
> 
> Same thing here.

Same here.
I had some wiring issues with my sensors and wanted to know from where the EIO comes.

Thanks,
//richard

Re: [PATCH 3/4] iio: dht11: Logging updates

[Harald Geyer]

Richard Weinberger writes:
> Am 03.12.2014 um 13:58 schrieb Harald Geyer:
> > Richard Weinberger writes:
> >> Currently the driver uses pr_* and dev_* functions.
> >> Change all logging functions to dev_* style to be consistent
> >> and have the correct device prefix in all messages.
> > 
> > Yes, actually this was on purpose:
> > The dev_ messages are really about something wrong with the device.
> > Ie if something goes wrong with one device but could perfectly work
> > with some other device.
> > The pr_ messages OTOH are about something wrong with clock resolution,
> > etc that would affect any DHT11 sensor on the system. Ideally we would
> > notice these things during probe() and just return with an error there.
> > Right now we aren't as clever as that, so we just log an error message
> > about the driver, when actually we are reading the device.
> > 
> > That said, I don't have strong feelings about this. If you want to
> > change this, I won't object. However if you really want to fix this,
> > then the proper thing would be to check for this conditions in
> > probe().
> 
> Currently we get log messages of style:
> "iio iio:deviceX: foo bar"
> and:
> "dht11 <name in DT>: foo bar"
> 
> I really favorite "dht11 <name in DT".
> In my device tree every senor has a sane name and log messages look like:
> "dht11 toiletten sensor: invalid checksum"

I think I ACKed the one with "iio iio:deviceX: foo bar"? 
 
> >> This change set also adds new messages to diagnose issues.
> > 
> > Comment below.
> > 
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> ---
> >>  drivers/iio/humidity/dht11.c | 16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> >> index 0023699..fbcd7cb 100644
> >> --- a/drivers/iio/humidity/dht11.c
> >> +++ b/drivers/iio/humidity/dht11.c
> >> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> >>  			timeres = t;
> >>  	}
> >>  	if (2*timeres > DHT11_DATA_BIT_HIGH) {
> >> -		pr_err("dht11: timeresolution %d too bad for decoding\n",
> >> +		dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
> >>  			timeres);
> >>  		return -EIO;
> >>  	}
> >>  	threshold = DHT11_DATA_BIT_HIGH / timeres;
> >>  	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> >> -		pr_err("dht11: WARNING: decoding ambiguous\n");
> >> +		dev_err(dht11->dev, "decoding ambiguous\n");
> >>  
> >>  	/* scale down with timeres and check validity */
> >>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
> >>  		t = dht11->edges[offset + 2*i + 2].ts -
> >>  			dht11->edges[offset + 2*i + 1].ts;
> >> -		if (!dht11->edges[offset + 2*i + 1].value)
> >> -			return -EIO;  /* lost synchronisation */
> >> +		if (!dht11->edges[offset + 2*i + 1].value) {
> >> +			dev_err(dht11->dev, "lost synchronisation\n");
> >> +			return -EIO;
> >> +		}
> > 
> > Are you sure this warrants a log message? I don't think this provides
> > much information. The userspace application should just try reading
> > the sensor again.
> > 
> > We could do someting smart and try to recover from such errors, but
> > ultimately userspace will need to deal with failed sensor communication
> > anyway, so I don't see the point.
> 
> The sensors are rather flaky and if they go nuts the user/developer
> maybe wants to know why.
> From a plain EIO she has no chance to find out. He'll have to add
> printk()s by hand to find out.
> With a log message one can start digging into the issue.

I think any log messages that are caused by wiring problems are
in place already. Also you should get an ETIMEDOUT not an EIO in
these cases. In these cases the number of edges received is reported,
which actually tells you a lot about the type of wiring problem.

BTW when I originally submitted the driver, it contained the following
code that got rejected:
/*
 * dht11_edges_print: show the data as actually received by the
 *                    driver.
 * This is dead code, keeping it for now just in case somebody needs
 * it for porting the driver to new sensor HW, etc.
 *
static void dht11_edges_print(struct dht11_gpio *dht11)
{
        int i;

        pr_err("dht11: transfer timed out:\n");
        for (i = 1; i < dht11->num_edges; ++i) {
                pr_err("dht11: %d: %lld ns %s\n", i,
                        dht11->edges[i].ts - dht11->edges[i-1].ts,
                        dht11->edges[i-1].value ? "high" : "low");
        }
}
*/

which I used to call on errors to debug the decoder and the quirks
necessary to support different sensors. Maybe someting like this would
get accepted if it integrated properly with the debugging frameworks
of the kernel instead of flooding the log?

I think the EIO errors are all really of the type:
"Some noise injection into the data line caused an hiccup, just try again."

I wouldn't object to changing some of the EIOs to EAGAINs or something,
but I doubt we get this through review.

JFTR: I don't object strongly to additional logging. Just telling you
1) This has been considered when writing the driver.
2) I probably won't ACK it, so Jonathan will have to decide wether this
is a valuable addition.

Thanks,
Harald

> >>  		timing[i] = t / timeres;
> >>  	}
> >>  
> >> @@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> >>  	temp_dec = dht11_decode_byte(&timing[24], threshold);
> >>  	checksum = dht11_decode_byte(&timing[32], threshold);
> >>  
> >> -	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> >> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
> >> +		dev_err(dht11->dev, "invalid checksum\n");
> >>  		return -EIO;
> >> +	}
> > 
> > Same thing here.
> 
> Same here.
> I had some wiring issues with my sensors and wanted to know from where the EIO comes.
> 
> Thanks,
> //richard

Re: [PATCH 3/4] iio: dht11: Logging updates

[Richard Weinberger]

Am 03.12.2014 um 14:56 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Am 03.12.2014 um 13:58 schrieb Harald Geyer:
>>> Richard Weinberger writes:
>>>> Currently the driver uses pr_* and dev_* functions.
>>>> Change all logging functions to dev_* style to be consistent
>>>> and have the correct device prefix in all messages.
>>>
>>> Yes, actually this was on purpose:
>>> The dev_ messages are really about something wrong with the device.
>>> Ie if something goes wrong with one device but could perfectly work
>>> with some other device.
>>> The pr_ messages OTOH are about something wrong with clock resolution,
>>> etc that would affect any DHT11 sensor on the system. Ideally we would
>>> notice these things during probe() and just return with an error there.
>>> Right now we aren't as clever as that, so we just log an error message
>>> about the driver, when actually we are reading the device.
>>>
>>> That said, I don't have strong feelings about this. If you want to
>>> change this, I won't object. However if you really want to fix this,
>>> then the proper thing would be to check for this conditions in
>>> probe().
>>
>> Currently we get log messages of style:
>> "iio iio:deviceX: foo bar"
>> and:
>> "dht11 <name in DT>: foo bar"
>>
>> I really favorite "dht11 <name in DT".
>> In my device tree every senor has a sane name and log messages look like:
>> "dht11 toiletten sensor: invalid checksum"
> 
> I think I ACKed the one with "iio iio:deviceX: foo bar"? 

No, my patch turns all logs to "dht11 <name in DT>: foo bar".

>>>> This change set also adds new messages to diagnose issues.
>>>
>>> Comment below.
>>>
>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>> ---
>>>>  drivers/iio/humidity/dht11.c | 16 ++++++++++------
>>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>>>> index 0023699..fbcd7cb 100644
>>>> --- a/drivers/iio/humidity/dht11.c
>>>> +++ b/drivers/iio/humidity/dht11.c
>>>> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>>>  			timeres = t;
>>>>  	}
>>>>  	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>>>> -		pr_err("dht11: timeresolution %d too bad for decoding\n",
>>>> +		dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
>>>>  			timeres);
>>>>  		return -EIO;
>>>>  	}
>>>>  	threshold = DHT11_DATA_BIT_HIGH / timeres;
>>>>  	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>>>> -		pr_err("dht11: WARNING: decoding ambiguous\n");
>>>> +		dev_err(dht11->dev, "decoding ambiguous\n");
>>>>  
>>>>  	/* scale down with timeres and check validity */
>>>>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>>>>  		t = dht11->edges[offset + 2*i + 2].ts -
>>>>  			dht11->edges[offset + 2*i + 1].ts;
>>>> -		if (!dht11->edges[offset + 2*i + 1].value)
>>>> -			return -EIO;  /* lost synchronisation */
>>>> +		if (!dht11->edges[offset + 2*i + 1].value) {
>>>> +			dev_err(dht11->dev, "lost synchronisation\n");
>>>> +			return -EIO;
>>>> +		}
>>>
>>> Are you sure this warrants a log message? I don't think this provides
>>> much information. The userspace application should just try reading
>>> the sensor again.
>>>
>>> We could do someting smart and try to recover from such errors, but
>>> ultimately userspace will need to deal with failed sensor communication
>>> anyway, so I don't see the point.
>>
>> The sensors are rather flaky and if they go nuts the user/developer
>> maybe wants to know why.
>> From a plain EIO she has no chance to find out. He'll have to add
>> printk()s by hand to find out.
>> With a log message one can start digging into the issue.
> 
> I think any log messages that are caused by wiring problems are
> in place already. Also you should get an ETIMEDOUT not an EIO in
> these cases. In these cases the number of edges received is reported,
> which actually tells you a lot about the type of wiring problem.
> 
> BTW when I originally submitted the driver, it contained the following
> code that got rejected:
> /*
>  * dht11_edges_print: show the data as actually received by the
>  *                    driver.
>  * This is dead code, keeping it for now just in case somebody needs
>  * it for porting the driver to new sensor HW, etc.
>  *
> static void dht11_edges_print(struct dht11_gpio *dht11)
> {
>         int i;
> 
>         pr_err("dht11: transfer timed out:\n");
>         for (i = 1; i < dht11->num_edges; ++i) {
>                 pr_err("dht11: %d: %lld ns %s\n", i,
>                         dht11->edges[i].ts - dht11->edges[i-1].ts,
>                         dht11->edges[i-1].value ? "high" : "low");
>         }
> }
> */
> 
> which I used to call on errors to debug the decoder and the quirks
> necessary to support different sensors. Maybe someting like this would
> get accepted if it integrated properly with the debugging frameworks
> of the kernel instead of flooding the log?
> 
> I think the EIO errors are all really of the type:
> "Some noise injection into the data line caused an hiccup, just try again."
> 
> I wouldn't object to changing some of the EIOs to EAGAINs or something,
> but I doubt we get this through review.
> 
> JFTR: I don't object strongly to additional logging. Just telling you
> 1) This has been considered when writing the driver.
> 2) I probably won't ACK it, so Jonathan will have to decide wether this
> is a valuable addition.

Okay. Let's see what Jonathan's opinion is.
IMHO a driver should use only one logging style not two.

Enough logging style bikeshedded, back to real work. :)

Thanks,
//richard

Re: [PATCH 3/4] iio: dht11: Logging updates

[Harald Geyer]

Richard Weinberger writes:
> Am 03.12.2014 um 14:56 schrieb Harald Geyer:
> > Richard Weinberger writes:
> >> Am 03.12.2014 um 13:58 schrieb Harald Geyer:
> >> Currently we get log messages of style:
> >> "iio iio:deviceX: foo bar"
> >> and:
> >> "dht11 <name in DT>: foo bar"
> >>
> >> I really favorite "dht11 <name in DT".
> >> In my device tree every senor has a sane name and log messages look like:
> >> "dht11 toiletten sensor: invalid checksum"
> > 
> > I think I ACKed the one with "iio iio:deviceX: foo bar"? 
> 
> No, my patch turns all logs to "dht11 <name in DT>: foo bar".

Maybe I was unclear. AFAICS there is one case where we actually have
"iio iio:deviceX: foo bar" now and I said:

> @@ -193,7 +197,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>               free_irq(dht11->irq, iio_dev);
>
>               if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
> -                     dev_err(&iio_dev->dev,
> +                     dev_err(dht11->dev,

Ack.

Hope that clarifies,
Harald

[PATCH 4/4] iio: dht11: Fix out-of-bounds read

[Richard Weinberger]

As we access i-1 we must not start with i=0.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/iio/humidity/dht11.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index fbcd7cb..5dfe71b 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -90,7 +90,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
 	/* Calculate timestamp resolution */
-	for (i = 0; i < dht11->num_edges; ++i) {
+	for (i = 1; i < dht11->num_edges; ++i) {
 		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
 		if (t > 0 && t < timeres)
 			timeres = t;
-- 
1.8.4.5

Re: [PATCH 4/4] iio: dht11: Fix out-of-bounds read

[Hartmut Knaack]

Richard Weinberger schrieb am 03.12.2014 um 00:32:
> As we access i-1 we must not start with i=0.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
Acked-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
>  drivers/iio/humidity/dht11.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index fbcd7cb..5dfe71b 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -90,7 +90,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>  
>  	/* Calculate timestamp resolution */
> -	for (i = 0; i < dht11->num_edges; ++i) {
> +	for (i = 1; i < dht11->num_edges; ++i) {
>  		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
>  		if (t > 0 && t < timeres)
>  			timeres = t;
> 

Re: iio: dht11 Updates

[Harald Geyer]

Hi Richard,

thanks for all the work you put into this!

Richard Weinberger writes:
> Please see my current patches for your driver.
> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
> With the IRQ changes I see 84 edges.

This still surprises me. With the IRQ changes I would expect the
preamble to be 2 edges only. I must be missing something. Can you
explain this to me?

I'll look into this as soon as I've time.

> I have also a question on your driver. Why you increment
> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
> 
>         threshold = DHT11_DATA_BIT_HIGH / timeres;
>         if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>                 pr_err("dht11: WARNING: decoding ambiguous\n");

This is to take ambiguity of when the bit started relativ to the
clock ticks into account. For example with common 32kHz clocks:
DHT11_DATA_BIT_LOW / timeres = 0
DHT11_DATA_BIT_HIGH / timeres = 2
but since the bit might not start at a clock tick the actual t of
a low bit can be either 0 or 1 while the actual t of a high bit
can be either 2 or 3.

This case is fine.

But if we had a 38kHz clock:
DHT11_DATA_BIT_LOW / timeres = 1    t can be 1 or 2
DHT11_DATA_BIT_HIGH / timeres = 2   t can be 2 or 3
so we have an ambiguity. The ambiguity could be removed by a smarter
decoder, that looks at the t of other bits, but I'm not going to do
that unless somebody is promising to test it on affected hardware.

Feel free to add some comment about this to the code.

> [PATCH 1/4] iio: dht11: Add locking
> [PATCH 2/4] iio: dht11: IRQ fixes
> [PATCH 3/4] iio: dht11: Logging updates
> [PATCH 4/4] iio: dht11: Fix out-of-bounds read

You can add my Acked-by or Reviewed-by to 1/4 and 4/4 if you want to.
Maybe Jonathan wants the patches reordered so that fixes come first, but
then 4/4 should not cause any problems in practice, so perhaps it doesn't
matter.

I really have to understand this preamble length thing on 2/4 and I will
reply to 3/4 separately.

Thanks,
Harald

Re: iio: dht11 Updates

[Richard Weinberger]

Harald,

Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> Hi Richard,
> 
> thanks for all the work you put into this!
> 
> Richard Weinberger writes:
>> Please see my current patches for your driver.
>> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
>> With the IRQ changes I see 84 edges.
> 
> This still surprises me. With the IRQ changes I would expect the
> preamble to be 2 edges only. I must be missing something. Can you
> explain this to me?

I'll try to find out!

> I'll look into this as soon as I've time.
> 
>> I have also a question on your driver. Why you increment
>> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
>>
>>         threshold = DHT11_DATA_BIT_HIGH / timeres;
>>         if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>>                 pr_err("dht11: WARNING: decoding ambiguous\n");
> 
> This is to take ambiguity of when the bit started relativ to the
> clock ticks into account. For example with common 32kHz clocks:
> DHT11_DATA_BIT_LOW / timeres = 0
> DHT11_DATA_BIT_HIGH / timeres = 2
> but since the bit might not start at a clock tick the actual t of
> a low bit can be either 0 or 1 while the actual t of a high bit
> can be either 2 or 3.
> 
> This case is fine.
> 
> But if we had a 38kHz clock:
> DHT11_DATA_BIT_LOW / timeres = 1    t can be 1 or 2
> DHT11_DATA_BIT_HIGH / timeres = 2   t can be 2 or 3
> so we have an ambiguity. The ambiguity could be removed by a smarter
> decoder, that looks at the t of other bits, but I'm not going to do
> that unless somebody is promising to test it on affected hardware.
> 
> Feel free to add some comment about this to the code.

Will do, thanks a lot for the explanation.

I was asking because I see the "dht11: WARNING: decoding ambiguous"
very often. (with and without my patches)

>> [PATCH 1/4] iio: dht11: Add locking
>> [PATCH 2/4] iio: dht11: IRQ fixes
>> [PATCH 3/4] iio: dht11: Logging updates
>> [PATCH 4/4] iio: dht11: Fix out-of-bounds read
> 
> You can add my Acked-by or Reviewed-by to 1/4 and 4/4 if you want to.
> Maybe Jonathan wants the patches reordered so that fixes come first, but
> then 4/4 should not cause any problems in practice, so perhaps it doesn't
> matter.
> 
> I really have to understand this preamble length thing on 2/4 and I will
> reply to 3/4 separately.

Thanks a lot!
//richard

Re: iio: dht11 Updates

[Harald Geyer]

Richard Weinberger writes:
> Harald,
> 
> Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> > Hi Richard,
> > 
> > thanks for all the work you put into this!
> > 
> > Richard Weinberger writes:
> >> I have also a question on your driver. Why you increment
> >> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
> >>
> >>         threshold = DHT11_DATA_BIT_HIGH / timeres;
> >>         if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> >>                 pr_err("dht11: WARNING: decoding ambiguous\n");
> > 
> > This is to take ambiguity of when the bit started relativ to the
> > clock ticks into account. For example with common 32kHz clocks:
> > DHT11_DATA_BIT_LOW / timeres = 0
> > DHT11_DATA_BIT_HIGH / timeres = 2
> > but since the bit might not start at a clock tick the actual t of
> > a low bit can be either 0 or 1 while the actual t of a high bit
> > can be either 2 or 3.
> > 
> > This case is fine.
> > 
> > But if we had a 38kHz clock:
> > DHT11_DATA_BIT_LOW / timeres = 1    t can be 1 or 2
> > DHT11_DATA_BIT_HIGH / timeres = 2   t can be 2 or 3
> > so we have an ambiguity. The ambiguity could be removed by a smarter
> > decoder, that looks at the t of other bits, but I'm not going to do
> > that unless somebody is promising to test it on affected hardware.
> > 
> > Feel free to add some comment about this to the code.
> 
> Will do, thanks a lot for the explanation.
> 
> I was asking because I see the "dht11: WARNING: decoding ambiguous"
> very often. (with and without my patches)

Yes, your patches shouldn't have any effect on this.
"very often" in the sense of "not always"? This would be very surprising,
because this would involve variable length clock ticks, i think.

I guess we should include timeres into the warning message.

Also I guess now is the time to think about a smarter decoder.

Thanks a lot for your effort.
Harald

Re: iio: dht11 Updates

[Richard Weinberger]

Am 03.12.2014 um 15:08 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Harald,
>>
>> Am 03.12.2014 um 13:18 schrieb Harald Geyer:
>>> Hi Richard,
>>>
>>> thanks for all the work you put into this!
>>>
>>> Richard Weinberger writes:
>>>> I have also a question on your driver. Why you increment
>>>> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
>>>>
>>>>         threshold = DHT11_DATA_BIT_HIGH / timeres;
>>>>         if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>>>>                 pr_err("dht11: WARNING: decoding ambiguous\n");
>>>
>>> This is to take ambiguity of when the bit started relativ to the
>>> clock ticks into account. For example with common 32kHz clocks:
>>> DHT11_DATA_BIT_LOW / timeres = 0
>>> DHT11_DATA_BIT_HIGH / timeres = 2
>>> but since the bit might not start at a clock tick the actual t of
>>> a low bit can be either 0 or 1 while the actual t of a high bit
>>> can be either 2 or 3.
>>>
>>> This case is fine.
>>>
>>> But if we had a 38kHz clock:
>>> DHT11_DATA_BIT_LOW / timeres = 1    t can be 1 or 2
>>> DHT11_DATA_BIT_HIGH / timeres = 2   t can be 2 or 3
>>> so we have an ambiguity. The ambiguity could be removed by a smarter
>>> decoder, that looks at the t of other bits, but I'm not going to do
>>> that unless somebody is promising to test it on affected hardware.
>>>
>>> Feel free to add some comment about this to the code.
>>
>> Will do, thanks a lot for the explanation.
>>
>> I was asking because I see the "dht11: WARNING: decoding ambiguous"
>> very often. (with and without my patches)
> 
> Yes, your patches shouldn't have any effect on this.
> "very often" in the sense of "not always"? This would be very surprising,
> because this would involve variable length clock ticks, i think.

It happens around 33% of all reads.
BTW: I'm using the DHT22's on my Beaglebone Black.
So the board should be "sane".
But I'll test them on another board too, just to be sure...

> I guess we should include timeres into the warning message.

I'll add some diagnose printk()s to find out.

Thanks,
//richard

Re: iio: dht11 Updates

[Richard Weinberger]

Am 03.12.2014 um 15:08 schrieb Harald Geyer:
>> I was asking because I see the "dht11: WARNING: decoding ambiguous"
>> very often. (with and without my patches)
> 
> Yes, your patches shouldn't have any effect on this.
> "very often" in the sense of "not always"? This would be very surprising,
> because this would involve variable length clock ticks, i think.
> 
> I guess we should include timeres into the warning message.
> 
> Also I guess now is the time to think about a smarter decoder.

Another question. Your driver defines:
#define DHT11_DATA_BIT_LOW      27000
#define DHT11_DATA_BIT_HIGH     70000

If I read the manual [0] correctly these constants are T_h0/1.
Why did you use 27000 for T_h0?
Setting it to 26000 (the typical value as stated by the manual),
I get the "decoding ambiguous" warning *always*. Setting it higher
makes the message go away.

Thanks,
//richard

[0] http://meteobox.tk/files/AM2302.pdf

Re: iio: dht11 Updates

[Harald Geyer]

Richard Weinberger writes:
> Am 03.12.2014 um 15:08 schrieb Harald Geyer:
> >> I was asking because I see the "dht11: WARNING: decoding ambiguous"
> >> very often. (with and without my patches)
> > 
> > Yes, your patches shouldn't have any effect on this.
> > "very often" in the sense of "not always"? This would be very surprising,
> > because this would involve variable length clock ticks, i think.
> > 
> > I guess we should include timeres into the warning message.
> > 
> > Also I guess now is the time to think about a smarter decoder.

I was wrong.
 
> Another question. Your driver defines:
> #define DHT11_DATA_BIT_LOW      27000
> #define DHT11_DATA_BIT_HIGH     70000
> 
> If I read the manual [0] correctly these constants are T_h0/1.
> Why did you use 27000 for T_h0?
>
> [0] http://meteobox.tk/files/AM2302.pdf

It's a compromise between data sheets for various slightly different
sensors. In particular the data sheet for AM2303 specifies
26-28us, so it's just in the middle. But note that DHT11_DATA_BIT_LOW
isn't used in the actual decoding. Just for printing the warning.

But looking at your data sheet I see the we currently use a start
command that's outside the specified range... I will have to look
up where the current 80ms value was suggested.

> Setting it to 26000 (the typical value as stated by the manual),
> I get the "decoding ambiguous" warning *always*. Setting it higher
> makes the message go away.

If you misstyped "always" and "go away" then this is to be expected.

Also I think I now understand what is going on:
Your board most probably has a clock much faster then 32kH and when
we calculate timeres, we don't actually calculate the timestamp
resolution but the length of the shortest pulse. The decoding
algorithm is robust in such cases, but it generates wrong warnings.

The proper fix is to drop messages about clock speed from the
decoding functin. If we want to keep such diagnostics, we should
have them at probe() time. - This will also resolve our disagreement
about proper formatting of log messages about clock issues. :)

Optionally we can drop the calculation of timeres and just use a
constant threshold.

Thanks,
Harald

Re: iio: dht11 Updates

[Richard Weinberger]

Harald,

Am 04.12.2014 um 14:45 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Am 03.12.2014 um 15:08 schrieb Harald Geyer:
>>>> I was asking because I see the "dht11: WARNING: decoding ambiguous"
>>>> very often. (with and without my patches)
>>>
>>> Yes, your patches shouldn't have any effect on this.
>>> "very often" in the sense of "not always"? This would be very surprising,
>>> because this would involve variable length clock ticks, i think.
>>>
>>> I guess we should include timeres into the warning message.
>>>
>>> Also I guess now is the time to think about a smarter decoder.
> 
> I was wrong.
>  
>> Another question. Your driver defines:
>> #define DHT11_DATA_BIT_LOW      27000
>> #define DHT11_DATA_BIT_HIGH     70000
>>
>> If I read the manual [0] correctly these constants are T_h0/1.
>> Why did you use 27000 for T_h0?
>>
>> [0] http://meteobox.tk/files/AM2302.pdf
> 
> It's a compromise between data sheets for various slightly different
> sensors. In particular the data sheet for AM2303 specifies
> 26-28us, so it's just in the middle. But note that DHT11_DATA_BIT_LOW
> isn't used in the actual decoding. Just for printing the warning.
> 
> But looking at your data sheet I see the we currently use a start
> command that's outside the specified range... I will have to look
> up where the current 80ms value was suggested.
> 
>> Setting it to 26000 (the typical value as stated by the manual),
>> I get the "decoding ambiguous" warning *always*. Setting it higher
>> makes the message go away.
> 
> If you misstyped "always" and "go away" then this is to be expected.

Nope, a lower value is bad and a higher makes the warning go away.

> Also I think I now understand what is going on:
> Your board most probably has a clock much faster then 32kH and when
> we calculate timeres, we don't actually calculate the timestamp
> resolution but the length of the shortest pulse. The decoding
> algorithm is robust in such cases, but it generates wrong warnings.
> 
> The proper fix is to drop messages about clock speed from the
> decoding functin. If we want to keep such diagnostics, we should
> have them at probe() time. - This will also resolve our disagreement
> about proper formatting of log messages about clock issues. :)

Sounds sane.

> Optionally we can drop the calculation of timeres and just use a
> constant threshold.

Same here.

Thanks,
//richard

Re: iio: dht11 Updates

[Richard Weinberger]

Harald,

Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> Hi Richard,
> 
> thanks for all the work you put into this!
> 
> Richard Weinberger writes:
>> Please see my current patches for your driver.
>> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
>> With the IRQ changes I see 84 edges.
> 
> This still surprises me. With the IRQ changes I would expect the
> preamble to be 2 edges only. I must be missing something. Can you
> explain this to me?

Did some more tests.
With my IRQ changes applied I get as stated 84 edges,
without I get 85. So we're only losing one edge.

Of course if we're too slow with registering the IRQ we'll lose
more edges and the results renders unusable.

Thanks,
//richard

Re: iio: dht11 Updates

[Harald Geyer]

Richard Weinberger writes:
> Harald,
> 
> Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> > Hi Richard,
> > 
> > thanks for all the work you put into this!
> > 
> > Richard Weinberger writes:
> >> Please see my current patches for your driver.
> >> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
> >> With the IRQ changes I see 84 edges.
> > 
> > This still surprises me. With the IRQ changes I would expect the
> > preamble to be 2 edges only. I must be missing something. Can you
> > explain this to me?
> 
> Did some more tests.
> With my IRQ changes applied I get as stated 84 edges,
> without I get 85. So we're only losing one edge.

Ok, finally had time to look into this and it's a simple off-by-one
issue (no bug) that confused me:
Without your patch the full number of edges per data transmission
is 86. But since the last edge is not significant, we only store
85 edges. - There should be a comment about this in the source...

With your patch the preamble is shortened by two edges as expected,
but since there is a workaround for sensors where the preamble wasn't
properly recorded, nobody notices unless they are looking closely at
the number of edges recorded. The next thing to do is check on DHT11
if the workaround is still needed.

Note that the old behaviour was nice for debugging wiring problems:
With my standard setup (3.3V, pin drive strenght 4 mA) I got the
following:
supply not connected: 0 edges (sensor always pulls the data line low)
data not connected: 2 edges (we only see the signal we generate ourselves)
gnd not connected: random amount of edges

With the new behaviour we will get 0 edges in both of the first two
cases. So we are really losing a tiny bit of functionality... :-(

HTH,
Harald

Re: iio: dht11 Updates

[Jonathan Cameron]

On 02/12/14 23:32, Richard Weinberger wrote:
> Please see my current patches for your driver.
> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
> With the IRQ changes I see 84 edges.
> 
> I have also a question on your driver. Why you increment
> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
> 
>         threshold = DHT11_DATA_BIT_HIGH / timeres;
>         if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>                 pr_err("dht11: WARNING: decoding ambiguous\n");
> 
> Thanks,
> //richard
> 
> [PATCH 1/4] iio: dht11: Add locking
> [PATCH 2/4] iio: dht11: IRQ fixes
> [PATCH 3/4] iio: dht11: Logging updates
> [PATCH 4/4] iio: dht11: Fix out-of-bounds read
> 
Hi all,

I've lost track of where we are with this patch series.
Does this cause trouble on any of the parts supported?

Re: iio: dht11 Updates

[harald]

Hi!

On Thu, 01 Jan 2015 12:38:23 +0000, Jonathan Cameron <jic23@kernel.org>
wrote:
> On 02/12/14 23:32, Richard Weinberger wrote:
>> Please see my current patches for your driver.
>> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
>> With the IRQ changes I see 84 edges.
>> 
>> 
>> [PATCH 1/4] iio: dht11: Add locking
>> [PATCH 2/4] iio: dht11: IRQ fixes
>> [PATCH 3/4] iio: dht11: Logging updates
>> [PATCH 4/4] iio: dht11: Fix out-of-bounds read
>> 
 
> I've lost track of where we are with this patch series.
> Does this cause trouble on any of the parts supported?

Yes, 2/4 needs an update. 

1/4 is fine by me, however it was noted that there is already
locking in the iio core for the in-kernel interface but not
for the sysfs interface. I think we need locking in the case of
the sysfs interace as well, but the final decision is yours...

4/4 is obviously right.

3/4 is cleanup. I'd rather defer this to later and do it right,
because right now this patch is cosmetic and doesn't get rid of
the deeper issues. I'll start to work on a cleanup series once
the fixes are merged.

Richard, what's your timeframe to send an updated series?
I can send an update if you don't have time.

HTH,
Harald

Re: iio: dht11 Updates

[Richard Weinberger]

Am 01.01.2015 um 22:18 schrieb harald@ccbib.org:
> Hi!
> 
> On Thu, 01 Jan 2015 12:38:23 +0000, Jonathan Cameron <jic23@kernel.org>
> wrote:
>> On 02/12/14 23:32, Richard Weinberger wrote:
>>> Please see my current patches for your driver.
>>> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
>>> With the IRQ changes I see 84 edges.
>>>
>>>
>>> [PATCH 1/4] iio: dht11: Add locking
>>> [PATCH 2/4] iio: dht11: IRQ fixes
>>> [PATCH 3/4] iio: dht11: Logging updates
>>> [PATCH 4/4] iio: dht11: Fix out-of-bounds read
>>>
>  
>> I've lost track of where we are with this patch series.
>> Does this cause trouble on any of the parts supported?
> 
> Yes, 2/4 needs an update. 

Correct.

> 1/4 is fine by me, however it was noted that there is already
> locking in the iio core for the in-kernel interface but not
> for the sysfs interface. I think we need locking in the case of
> the sysfs interace as well, but the final decision is yours...

We definitely need it.
I have more than one sensor attached and collectd queries them.
Without locking collectd manages to hit this race window.
So the issue is real.

> 4/4 is obviously right.
> 
> 3/4 is cleanup. I'd rather defer this to later and do it right,
> because right now this patch is cosmetic and doesn't get rid of
> the deeper issues. I'll start to work on a cleanup series once
> the fixes are merged.

Agreed.

> Richard, what's your timeframe to send an updated series?
> I can send an update if you don't have time.

It would be great if you can send an updated version of 2/4.
I don't have a DHT11 sensor (only DHT22) and I'm still recovering
from a cold.

Thanks
//richard

Re: iio: dht11 Updates

[Jonathan Cameron]

On 02/01/15 11:28, Richard Weinberger wrote:
> Am 01.01.2015 um 22:18 schrieb harald@ccbib.org:
>> Hi!
>>
>> On Thu, 01 Jan 2015 12:38:23 +0000, Jonathan Cameron <jic23@kernel.org>
>> wrote:
>>> On 02/12/14 23:32, Richard Weinberger wrote:
>>>> Please see my current patches for your driver.
>>>> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
>>>> With the IRQ changes I see 84 edges.
>>>>
>>>>
>>>> [PATCH 1/4] iio: dht11: Add locking
>>>> [PATCH 2/4] iio: dht11: IRQ fixes
>>>> [PATCH 3/4] iio: dht11: Logging updates
>>>> [PATCH 4/4] iio: dht11: Fix out-of-bounds read
>>>>
>>  
>>> I've lost track of where we are with this patch series.
>>> Does this cause trouble on any of the parts supported?
>>
>> Yes, 2/4 needs an update. 
> 
> Correct.
> 
>> 1/4 is fine by me, however it was noted that there is already
>> locking in the iio core for the in-kernel interface but not
>> for the sysfs interface. I think we need locking in the case of
>> the sysfs interace as well, but the final decision is yours...
> 
> We definitely need it.
> I have more than one sensor attached and collectd queries them.
> Without locking collectd manages to hit this race window.
> So the issue is real.
Locking is needed.  Convention has normally been to do this in the
driver rather than a subsystem core.
That gives more fine grained control and avoids locking
when not necessary (some devices will need a lock on data read,
others will not).
Clearly in this case, the need should have been picked up in review
(oops).  Sorry about that.
> 
>> 4/4 is obviously right.
>>
>> 3/4 is cleanup. I'd rather defer this to later and do it right,
>> because right now this patch is cosmetic and doesn't get rid of
>> the deeper issues. I'll start to work on a cleanup series once
>> the fixes are merged.
> 
> Agreed.
> 
>> Richard, what's your timeframe to send an updated series?
>> I can send an update if you don't have time.
> 
> It would be great if you can send an updated version of 2/4.
> I don't have a DHT11 sensor (only DHT22) and I'm still recovering
> from a cold.
> 
> Thanks
> //richard
> --
> 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
> 

[PATCHv2 1/3] iio: dht11: Fix out-of-bounds read

[Harald Geyer]

As we access i-1 we must not start with i=0.

Signed-off-by: Richard Weinberger <richard@nod.at>
Acked-by: Hartmut Knaack <knaack.h@gmx.de>
Acked-by: Harald Geyer <harald@ccbib.org>
---
Resending for Richard Weinberger.
No changes since v1 except reordering.

 drivers/iio/humidity/dht11.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 623c145..f546eca 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -88,7 +88,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
 	/* Calculate timestamp resolution */
-	for (i = 0; i < dht11->num_edges; ++i) {
+	for (i = 1; i < dht11->num_edges; ++i) {
 		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
 		if (t > 0 && t < timeres)
 			timeres = t;
-- 
1.7.2.5

Re: [PATCHv2 1/3] iio: dht11: Fix out-of-bounds read

[Richard Weinberger]

Am 05.01.2015 um 14:49 schrieb Harald Geyer:
> As we access i-1 we must not start with i=0.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Acked-by: Hartmut Knaack <knaack.h@gmx.de>
> Acked-by: Harald Geyer <harald@ccbib.org>
> ---
> Resending for Richard Weinberger.
> No changes since v1 except reordering.

I fear you got the resend wrong.
The mail should be "From: Richard Weinberger <richard@nod.at>" to keep me
as author and the SoB-Chain should have a "Signed-off-by: Harald Geyer <harald@ccbib.org>"
at the end to indicate that this patch went though your hands.

Thanks,
//richard

Re: [PATCHv2 1/3] iio: dht11: Fix out-of-bounds read

[sanjeev sharma]

On Mon, Jan 5, 2015 at 7:25 PM, Richard Weinberger <richard@nod.at> wrote:
> Am 05.01.2015 um 14:49 schrieb Harald Geyer:
>> As we access i-1 we must not start with i=0.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> Acked-by: Hartmut Knaack <knaack.h@gmx.de>
>> Acked-by: Harald Geyer <harald@ccbib.org>
     Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
>> ---
>> Resending for Richard Weinberger.
>> No changes since v1 except reordering.
>
> I fear you got the resend wrong.
> The mail should be "From: Richard Weinberger <richard@nod.at>" to keep me
> as author and the SoB-Chain should have a "Signed-off-by: Harald Geyer <harald@ccbib.org>"
> at the end to indicate that this patch went though your hands.
>
> Thanks,
> //richard
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv2 1/3,RESEND] iio: dht11: Fix out-of-bounds read

[Harald Geyer]

As we access i-1 we must not start with i=0.

From: Richard Weinberger <richard@nod.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
Acked-by: Hartmut Knaack <knaack.h@gmx.de>
Acked-by: Harald Geyer <harald@ccbib.org>
Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
---
Resending again to get the metadata right.
No Signed-off-by from me, because I didn't contribute anything.
No changes since v1 except reordering.

 drivers/iio/humidity/dht11.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 623c145..f546eca 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -88,7 +88,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
 	/* Calculate timestamp resolution */
-	for (i = 0; i < dht11->num_edges; ++i) {
+	for (i = 1; i < dht11->num_edges; ++i) {
 		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
 		if (t > 0 && t < timeres)
 			timeres = t;
-- 
1.7.2.5

Re: [PATCHv2 1/3,RESEND] iio: dht11: Fix out-of-bounds read

[Jonathan Cameron]

On 07/01/15 12:15, Harald Geyer wrote:
> As we access i-1 we must not start with i=0.
> 
> From: Richard Weinberger <richard@nod.at>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Acked-by: Hartmut Knaack <knaack.h@gmx.de>
> Acked-by: Harald Geyer <harald@ccbib.org>
> Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
> ---
> Resending again to get the metadata right.
Putting a From: in doesn't work, you need to use git commit --amend --author to
fix it up.

Anyhow, applied and author fixed up.
> No Signed-off-by from me, because I didn't contribute anything.
> No changes since v1 except reordering.
> 
>  drivers/iio/humidity/dht11.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 623c145..f546eca 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -88,7 +88,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>  
>  	/* Calculate timestamp resolution */
> -	for (i = 0; i < dht11->num_edges; ++i) {
> +	for (i = 1; i < dht11->num_edges; ++i) {
>  		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
>  		if (t > 0 && t < timeres)
>  			timeres = t;
> 

Re: [PATCHv2 1/3,RESEND] iio: dht11: Fix out-of-bounds read

[Richard Weinberger]

Am 10.01.2015 um 12:14 schrieb Jonathan Cameron:
> On 07/01/15 12:15, Harald Geyer wrote:
>> As we access i-1 we must not start with i=0.
>>
>> From: Richard Weinberger <richard@nod.at>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> Acked-by: Hartmut Knaack <knaack.h@gmx.de>
>> Acked-by: Harald Geyer <harald@ccbib.org>
>> Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
>> ---
>> Resending again to get the metadata right.
> Putting a From: in doesn't work, you need to use git commit --amend --author to
> fix it up.

Or just use "git am" to apply a patch to a tree and keep the original author.
If you use "git format-patch" later it will produce the correct "From:".


Thanks,
//richard

Re: [PATCHv2 1/3,RESEND] iio: dht11: Fix out-of-bounds read

[Harald Geyer]

Richard Weinberger writes:
> Am 10.01.2015 um 12:14 schrieb Jonathan Cameron:
> > On 07/01/15 12:15, Harald Geyer wrote:
> >> As we access i-1 we must not start with i=0.
> >>
> >> From: Richard Weinberger <richard@nod.at>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> Acked-by: Hartmut Knaack <knaack.h@gmx.de>
> >> Acked-by: Harald Geyer <harald@ccbib.org>
> >> Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
> >> ---
> >> Resending again to get the metadata right.
> > Putting a From: in doesn't work, you need to use
> > git commit --amend --author to fix it up.
> 
> Or just use "git am" to apply a patch to a tree and keep the original author.
> If you use "git format-patch" later it will produce the correct "From:".

Thats exactly what I did initially, but of course when pasting the
patch into my MUA, the headers got lost. I guess you don't suggest that
I forge your address, so what's the right way to handle this? Include
the entire mbox file into the body of the message?

TIA,
Harald

Re: [PATCHv2 1/3,RESEND] iio: dht11: Fix out-of-bounds read

[Richard Weinberger]

Am 12.01.2015 um 12:26 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Am 10.01.2015 um 12:14 schrieb Jonathan Cameron:
>>> On 07/01/15 12:15, Harald Geyer wrote:
>>>> As we access i-1 we must not start with i=0.
>>>>
>>>> From: Richard Weinberger <richard@nod.at>
>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>> Acked-by: Hartmut Knaack <knaack.h@gmx.de>
>>>> Acked-by: Harald Geyer <harald@ccbib.org>
>>>> Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
>>>> ---
>>>> Resending again to get the metadata right.
>>> Putting a From: in doesn't work, you need to use
>>> git commit --amend --author to fix it up.
>>
>> Or just use "git am" to apply a patch to a tree and keep the original author.
>> If you use "git format-patch" later it will produce the correct "From:".
> 
> Thats exactly what I did initially, but of course when pasting the
> patch into my MUA, the headers got lost. I guess you don't suggest that
> I forge your address, so what's the right way to handle this? Include
> the entire mbox file into the body of the message?

Use git send-email. :-)

Thanks,
//richard

[PATCHv2 2/3,RESEND] iio: dht11: Add locking

[Harald Geyer]

Make sure that the read function is not interrupted...

From: Richard Weinberger <richard@nod.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
Acked-by: Harald Geyer <harald@ccbib.org>
Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
---
Resending again to get the metadata right.
No Signed-off-by from me, because I didn't contribute anything.
No changes since v1 except reordering.

 drivers/iio/humidity/dht11.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index f546eca..7717f5c 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -29,6 +29,7 @@
 #include <linux/wait.h>
 #include <linux/bitops.h>
 #include <linux/completion.h>
+#include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
@@ -57,6 +58,7 @@ struct dht11 {
 	int				irq;
 
 	struct completion		completion;
+	struct mutex			lock;
 
 	s64				timestamp;
 	int				temperature;
@@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 	struct dht11 *dht11 = iio_priv(iio_dev);
 	int ret;
 
+	mutex_lock(&dht11->lock);
 	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
 		reinit_completion(&dht11->completion);
 
@@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		ret = -EINVAL;
 err:
 	dht11->num_edges = -1;
+	mutex_unlock(&dht11->lock);
 	return ret;
 }
 
@@ -268,6 +272,7 @@ static int dht11_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, iio);
 
 	init_completion(&dht11->completion);
+	mutex_init(&dht11->lock);
 	iio->name = pdev->name;
 	iio->dev.parent = &pdev->dev;
 	iio->info = &dht11_iio_info;
-- 
1.7.2.5

Re: [PATCHv2 2/3,RESEND] iio: dht11: Add locking

[Jonathan Cameron]

On 07/01/15 12:18, Harald Geyer wrote:
> Make sure that the read function is not interrupted...
> 
> From: Richard Weinberger <richard@nod.at>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Acked-by: Harald Geyer <harald@ccbib.org>
> Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
> ---
> Resending again to get the metadata right.
> No Signed-off-by from me, because I didn't contribute anything.
Technically you 'handled' the patch which is all that actually
matters for a sign-off.  Anyhow I don't really care either way ;)

Applied to the fixes-togreg branch of iio.git

Thanks,

Jonathan
> No changes since v1 except reordering.
> 
>  drivers/iio/humidity/dht11.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index f546eca..7717f5c 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -29,6 +29,7 @@
>  #include <linux/wait.h>
>  #include <linux/bitops.h>
>  #include <linux/completion.h>
> +#include <linux/mutex.h>
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
> @@ -57,6 +58,7 @@ struct dht11 {
>  	int				irq;
>  
>  	struct completion		completion;
> +	struct mutex			lock;
>  
>  	s64				timestamp;
>  	int				temperature;
> @@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  	struct dht11 *dht11 = iio_priv(iio_dev);
>  	int ret;
>  
> +	mutex_lock(&dht11->lock);
>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>  		reinit_completion(&dht11->completion);
>  
> @@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		ret = -EINVAL;
>  err:
>  	dht11->num_edges = -1;
> +	mutex_unlock(&dht11->lock);
>  	return ret;
>  }
>  
> @@ -268,6 +272,7 @@ static int dht11_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, iio);
>  
>  	init_completion(&dht11->completion);
> +	mutex_init(&dht11->lock);
>  	iio->name = pdev->name;
>  	iio->dev.parent = &pdev->dev;
>  	iio->info = &dht11_iio_info;
> 

Re: [PATCHv2 2/3,RESEND] iio: dht11: Add locking

[Richard Weinberger]

On Sat, Jan 10, 2015 at 12:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 07/01/15 12:18, Harald Geyer wrote:
>> Make sure that the read function is not interrupted...
>>
>> From: Richard Weinberger <richard@nod.at>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> Acked-by: Harald Geyer <harald@ccbib.org>
>> Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
>> ---
>> Resending again to get the metadata right.
>> No Signed-off-by from me, because I didn't contribute anything.
> Technically you 'handled' the patch which is all that actually
> matters for a sign-off.  Anyhow I don't really care either way ;)
>
> Applied to the fixes-togreg branch of iio.git

I don't see any of my fixes in Linus' tree.
Did I miss something?

-- 
Thanks,
//richard

Re: [PATCHv2 2/3,RESEND] iio: dht11: Add locking

[Jonathan Cameron]

On 22/02/15 10:44, Richard Weinberger wrote:
> On Sat, Jan 10, 2015 at 12:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 07/01/15 12:18, Harald Geyer wrote:
>>> Make sure that the read function is not interrupted...
>>>
>>> From: Richard Weinberger <richard@nod.at>
>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>> Acked-by: Harald Geyer <harald@ccbib.org>
>>> Reviewed-by: Sanjeev Sharma <sanjeev_sharma@mentor.com>
>>> ---
>>> Resending again to get the metadata right.
>>> No Signed-off-by from me, because I didn't contribute anything.
>> Technically you 'handled' the patch which is all that actually
>> matters for a sign-off.  Anyhow I don't really care either way ;)
>>
>> Applied to the fixes-togreg branch of iio.git
> 
> I don't see any of my fixes in Linus' tree.
> Did I miss something?
> 
Timing was unfortunate for these.  It's messy to send
fixes very close to the end of a cycle for anything less
than high impact bugs (Greg KH won't send them on to Linus).
Then the merge window and now I'm in a clean room without my ssh
keys.  Hence these will go to Greg soon, but haven't just yet.
Quite a few other fixes queued up along side yours.

Sorry for the delay.

Jonathan

[PATCHv2 3/3,RESEND] iio: dht11: IRQ fixes

[Harald Geyer]

Since setting irq-enabled GPIOs into output state is not supported
by all GPIO controllers, we need to disable the irq while requesting
sensor data. As side effect we lose a tiny bit of functionality:
Some wiring problems can't be concluded from log messages anymore.

From: Richard Weinberger <richard@nod.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Resending to get the metadata right.

changes since v1:
Reduce the expected number of edges per data transmission, since we
won't see the edges we generate ourselves.

 drivers/iio/humidity/dht11.c |   62 +++++++++++++++++++++++------------------
 1 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 7717f5c..7d79a1a 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -40,8 +40,12 @@
 
 #define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
 
-#define DHT11_EDGES_PREAMBLE 4
+#define DHT11_EDGES_PREAMBLE 2
 #define DHT11_BITS_PER_READ 40
+/*
+ * Note that when reading the sensor actually 84 edges are detected, but
+ * since the last edge is not significant, we only store 83:
+ */
 #define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)
 
 /* Data transmission timing (nano seconds) */
@@ -140,6 +144,27 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	return 0;
 }
 
+/*
+ * IRQ handler called on GPIO edges
+ */
+static irqreturn_t dht11_handle_irq(int irq, void *data)
+{
+	struct iio_dev *iio = data;
+	struct dht11 *dht11 = iio_priv(iio);
+
+	/* TODO: Consider making the handler safe for IRQ sharing */
+	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
+		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
+		dht11->edges[dht11->num_edges++].value =
+						gpio_get_value(dht11->gpio);
+
+		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
+			complete(&dht11->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int dht11_read_raw(struct iio_dev *iio_dev,
 			const struct iio_chan_spec *chan,
 			int *val, int *val2, long m)
@@ -160,8 +185,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		if (ret)
 			goto err;
 
+		ret = request_irq(dht11->irq, dht11_handle_irq,
+				  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				  iio_dev->name, iio_dev);
+		if (ret)
+			goto err;
+
 		ret = wait_for_completion_killable_timeout(&dht11->completion,
 								 HZ);
+
+		free_irq(dht11->irq, iio_dev);
+
 		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
 			dev_err(&iio_dev->dev,
 					"Only %d signal edges detected\n",
@@ -197,27 +231,6 @@ static const struct iio_info dht11_iio_info = {
 	.read_raw		= dht11_read_raw,
 };
 
-/*
- * IRQ handler called on GPIO edges
-*/
-static irqreturn_t dht11_handle_irq(int irq, void *data)
-{
-	struct iio_dev *iio = data;
-	struct dht11 *dht11 = iio_priv(iio);
-
-	/* TODO: Consider making the handler safe for IRQ sharing */
-	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
-		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
-		dht11->edges[dht11->num_edges++].value =
-						gpio_get_value(dht11->gpio);
-
-		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
-			complete(&dht11->completion);
-	}
-
-	return IRQ_HANDLED;
-}
-
 static const struct iio_chan_spec dht11_chan_spec[] = {
 	{ .type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
@@ -260,11 +273,6 @@ static int dht11_probe(struct platform_device *pdev)
 		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
 		return -EINVAL;
 	}
-	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
-				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-				pdev->name, iio);
-	if (ret)
-		return ret;
 
 	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
-- 
1.7.2.5