Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked registerread

[Éric Piel]

Op 16-05-11 23:36, Christian Lamparter schreef:
> On Monday 16 May 2011 13:16:46 Éric Piel wrote:
> > Op 16-05-11 00:46, Christian Lamparter schreef:
> > >  From my POV, it looks like the hardware is not working as expected
> > > and returns a bogus data rate. The driver doesn't check the result
> > > and directly uses it as some sort of divisor in some places:
> > >
> > > msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> > >
> > > Under this circumstances, this could very well cause the
> > > "divide by zero" exception from above.
> > However, I'd fix it a bit differently: let lis3lv02d_get_odr() return
> > the raw data, and create a special function
> > lis3lv02d_get_pwron_delay_ms() which does the "lis3->pwron_delay /
> > lis3lv02d_get_odr()" with special handling for 0 (returning a large
> > value and also sending a printk_once() ).
> Do you know how "volatile" this data rate is? If it never changes
> [at least it doesn't here?] then why not read it once in init_device
> and store it in the device context?
It is not normally changing, normally it is set just at init/unsuspend 
(where the bios can also interfere sometimes) and when the user changes 
it. So definitely within the same function it's not going to suddenly 
change. We could avoid calculating/checking it twice in 
lis3lv02d_selftest(). Care to do a third version with this little clean up?

> Anyway, I updated the code... But instead of returning a "large value"
> I went for the -ENXIO to bail-out early, so now we won't continue if
> something went bad [after resume for instance?].
Sounds even better than using a conservative value!

> > As you have noted, we might want to check other parts of the driver to
> > validate the data from the device. So far, all the code considers that
> > the device is flawless :-S
> well:
>    CHECK   drivers/misc/lis3lv02d/lis3lv02d.c
> drivers/misc/lis3lv02d/lis3lv02d.c:170:52: warning: cast to restricted __le16
This is not introduced by your patch, right? So it's fine for now :-)

Thanks,
Eric
--
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/