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

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: <E1Xw9W8-0001Db-9a@stardust.g4.wien.funkfeuer.at>
Date:	Wed, 03 Dec 2014 13:58:52 +0100
From:	Harald Geyer <harald@...ib.org>
To:	Richard Weinberger <richard@....at>
cc:	jic23@...nel.org, knaack.h@....de, lars@...afoo.de,
	pmeerw@...erw.net, sanjeev_sharma@...tor.com,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] iio: dht11: Logging updates

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@....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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/