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

["Éric Piel" <Eric.Piel@tremplin-utc.net>]

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