Re: [PATCH 01/10] lis3lv02d: avoid divide by zero due to unchecked

[Éric Piel]

Op 01-08-11 23:29, Andrew Morton schreef:
> On Mon, 1 Aug 2011 23:11:17 +0200
> Christian Lamparter<chunkeey@xxxxxxxxxxxxxx>  wrote:
>
> > On Monday, August 01, 2011 10:29:06 PM Andrew Morton wrote:
> > > On Mon, 25 Jul 2011 17:16:23 +0200
> > > __ric Piel<eric.piel@xxxxxxxxxxxxxxxx>  wrote:
> > >
> > > > +static int lis3lv02d_get_pwron_wait(struct lis3lv02d *lis3)
> > > > +{
> > > > +	int div = lis3lv02d_get_odr();
> > > > +
> > > > +	if (WARN_ONCE(div == 0, "device returned spurious data"))
> > > > +		return -ENXIO;
> > > > +
> > > > +	/* LIS3 power on delay is quite long */
> > > > +	msleep(lis3->pwron_delay / div);
> > > > +	return 0;
> > > > +}
> > >
> > > The WARN_ONCE may not be very useful.  The user gets worried, might
> > > report it (often to a distro, not to you!).  But we won't actually *do*
> > > anything with the information?
> > The sensor is used to park the hdd in case of an "accident". However,
> > if the sensors is not working, the user should at least get a WARN
> > that something is very wrong, right?
>
> Well if we're doing this for the user's benefit (most WARNs are for developers)
> then the message should be user-useful.  That one isn't, really.
>
> Can we come up with some text which is more useful to the user/operator and
> won't require him/her/it to send emails and raise bug reports?
>
> Also, the stack trace which WARN emits is not useful in this application?
Thanks Andrew for pointing out this.
Indeed, a WARN with such a message seems not the best way to explain 
what's is going on. IIRC, Christian suspects the bug happens due to some 
weird things that the bios does. So do you think this code looks better?

if (div == 0) {
  pr_warn_once("device returned spurious data, it will not be used. "
	       "It might be a hardware or firmware bug. "
	       "Contact the driver's authors if you think it is not.");
  return -ENXIO;
}

If every one likes it, I'll update the patch and send you the new version.

See you,
Éric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/