linux-kernel - Re: [PATCH] iio: dht11: set debug log level for parsing error messages

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]

Date: Mon, 25 Mar 2024 19:10:43 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Harald Geyer <harald@...ib.org>
Cc: George Stark <gnstark@...utedevices.com>, lars@...afoo.de,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel@...utedevices.com
Subject: Re: [PATCH] iio: dht11: set debug log level for parsing error
 messages

On Mon, 25 Mar 2024 19:48:00 +0100
Harald Geyer <harald@...ib.org> wrote:

> Hi George!
> 
> I'm torn on this:
> 
> Am Montag, dem 25.03.2024 um 19:54 +0300 schrieb George Stark:
> > Protocol parsing errors could happen due to several reasons like
> > noise
> > environment, heavy load on system etc. If to poll the sensor
> > frequently
> > and/or for a long period kernel log will become polluted with error
> > messages if their log level is err (i.e. on by default).  
> 
> Yes, these error are often recoverable. (As are many other HW errors,
> that typically are logged. Eg USB bus resets due to EMI)
> 
> However they are still genuine errors of the HW.
> 
> >  Also some types
> > of those messages already have dbg level so use unified log level for
> > all such cases.  
> 
> My take so far has been: Debug level messages are for debugging the
> code (ie adding/testing support of new device variants etc). Users
> aren't expected to know about or enable debug output. OTOH anything
> actually going wrong is an error and should be logged as such.
> 
> The idea is, that these messages help users understand issues with
> their HW (like too long cables, broken cables etc). But it is true,
> that they will slowly accumulate in many real world scenarios without
> anything being truly wrong.
> 
> I don't consider the dmesg buffer being rotated after a month or two a
> bug. But I suppose this is a corner case. I'll happily accept whatever
> Jonathan thinks is reasonable.
My take:

If the errors are eaten with no user visibility then they should
be logged (errors in interrupt handlers etc) but for errors in
code that returns an error code to the userspace read or similar there
is info that 'something went wrong' available via that then it's fine
to use dev_dbg() with expectation anyone who is looking into issues
can turn them on.  However, I defer to driver maintainers on whether
they prefer dev_err() or dev_dbg() for these sorts of cases. Far
as I'm concerned either choice is fine and it's a judgement on
the expected rates of error and impact.

Jonathan

> 
> Best regards,
> Harald
> 
> 
> > Signed-off-by: George Stark <gnstark@...utedevices.com>
> > ---
> > I use DHT22 sensor with Raspberry Pi Zero W as a simple home meteo
> > station.
> > Even if to poll the sensor once per tens of seconds after month or
> > two dmesg
> > may become full of useless parsing error messages. Anyway those
> > errors are caught
> > in the user software thru return values.
> > 
> >  drivers/iio/humidity/dht11.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/humidity/dht11.c
> > b/drivers/iio/humidity/dht11.c
> > index c97e25448772..e2cbc442177b 100644
> > --- a/drivers/iio/humidity/dht11.c
> > +++ b/drivers/iio/humidity/dht11.c
> > @@ -156,7 +156,7 @@ static int dht11_decode(struct dht11 *dht11, int
> > offset)
> >                 dht11->temperature = temp_int * 1000;
> >                 dht11->humidity = hum_int * 1000;
> >         } else {
> > -               dev_err(dht11->dev,
> > +               dev_dbg(dht11->dev,
> >                         "Don't know how to decode data: %d %d %d
> > %d\n",
> >                         hum_int, hum_dec, temp_int, temp_dec);
> >                 return -EIO;
> > @@ -239,7 +239,7 @@ static int dht11_read_raw(struct iio_dev
> > *iio_dev,
> >  #endif
> > 
> >                 if (ret == 0 && dht11->num_edges <
> > DHT11_EDGES_PER_READ - 1) {
> > -                       dev_err(dht11->dev, "Only %d signal edges
> > detected\n",
> > +                       dev_dbg(dht11->dev, "Only %d signal edges
> > detected\n",
> >                                 dht11->num_edges);
> >                         ret = -ETIMEDOUT;
> >                 }
> > --
> > 2.25.1
> >   
>