[PATCH] lis3lv02d: Avoid zero-division

[PATCH] lis3lv02d: Avoid zero-division

[Takashi Iwai]

In some weird situation, HP DriveGuard chip can't read ODR value
correctly, and it results in a zero-division Oops in lis3lv02d driver.
This patch fixes the Oops by checking the value appopriately, and skips
if any weird value is read.

Cc: <stable@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/misc/lis3lv02d/lis3lv02d.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
index 8b51cd6..c4eb2f3 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d.c
@@ -228,6 +228,14 @@ static int lis3lv02d_set_odr(int rate)
 	return -EINVAL;
 }
 
+static void lis3lv02d_power_delay(struct lis3lv02d *lis3)
+{
+	int odr = lis3lv02d_get_odr();
+	if (odr <= 0)
+		return;
+	msleep(lis3->pwron_delay / odr);
+}
+
 static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 {
 	u8 ctlreg, reg;
@@ -266,7 +274,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 
 	lis3->read(lis3, ctlreg, &reg);
 	lis3->write(lis3, ctlreg, (reg | selftest));
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	lis3lv02d_power_delay(lis3);
 
 	/* Read directly to avoid axis remap */
 	x = lis3->read_data(lis3, OUTX);
@@ -275,7 +283,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 
 	/* back to normal settings */
 	lis3->write(lis3, ctlreg, reg);
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	lis3lv02d_power_delay(lis3);
 
 	results[0] = x - lis3->read_data(lis3, OUTX);
 	results[1] = y - lis3->read_data(lis3, OUTY);
@@ -385,7 +393,7 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
 	}
 
 	/* LIS3 power on delay is quite long */
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	lis3lv02d_power_delay(lis3);
 
 	if (lis3->reg_ctrl)
 		lis3_context_restore(lis3);
-- 
1.7.7

Re: [PATCH] lis3lv02d: Avoid zero-division

[Éric Piel]

Op 03-11-11 13:09, Takashi Iwai schreef:
> In some weird situation, HP DriveGuard chip can't read ODR value
> correctly, and it results in a zero-division Oops in lis3lv02d driver.
> This patch fixes the Oops by checking the value appopriately, and skips
> if any weird value is read.
Hi Takashi,
Actually, a similar patch already just landed in linus' tree:
1510dd5954 (lis3lv02d: avoid divide by zero due to unchecked)

However, in the patch applied, the device is disabled (until next 
reboot) while in yours, the sleep is just skipped. Does it work again 
after the read of odr fails? If so, maybe I could improve the current 
version by, after the odr read fails, sleeping a long and safe time and 
then trying to read the odr again. Then if it fails again, we give up, 
otherwise the device can be used again.

Do you have such a device yourself? Could you let me know if after a 
failing read of the odr, the device keeps working?

Cheers,
Éric

>
> Cc:<stable@kernel.org>
> Signed-off-by: Takashi Iwai<tiwai@suse.de>
> ---
>   drivers/misc/lis3lv02d/lis3lv02d.c |   14 +++++++++++---
>   1 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
> index 8b51cd6..c4eb2f3 100644
> --- a/drivers/misc/lis3lv02d/lis3lv02d.c
> +++ b/drivers/misc/lis3lv02d/lis3lv02d.c
> @@ -228,6 +228,14 @@ static int lis3lv02d_set_odr(int rate)
>   	return -EINVAL;
>   }
>
> +static void lis3lv02d_power_delay(struct lis3lv02d *lis3)
> +{
> +	int odr = lis3lv02d_get_odr();
> +	if (odr<= 0)
> +		return;
> +	msleep(lis3->pwron_delay / odr);
> +}
> +
>   static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
>   {
>   	u8 ctlreg, reg;
> @@ -266,7 +274,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
>
>   	lis3->read(lis3, ctlreg,&reg);
>   	lis3->write(lis3, ctlreg, (reg | selftest));
> -	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> +	lis3lv02d_power_delay(lis3);
>
>   	/* Read directly to avoid axis remap */
>   	x = lis3->read_data(lis3, OUTX);
> @@ -275,7 +283,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
>
>   	/* back to normal settings */
>   	lis3->write(lis3, ctlreg, reg);
> -	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> +	lis3lv02d_power_delay(lis3);
>
>   	results[0] = x - lis3->read_data(lis3, OUTX);
>   	results[1] = y - lis3->read_data(lis3, OUTY);
> @@ -385,7 +393,7 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
>   	}
>
>   	/* LIS3 power on delay is quite long */
> -	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> +	lis3lv02d_power_delay(lis3);
>
>   	if (lis3->reg_ctrl)
>   		lis3_context_restore(lis3);

Re: [PATCH] lis3lv02d: Avoid zero-division

[Takashi Iwai]

At Tue, 08 Nov 2011 04:57:42 +0100,
Éric Piel wrote:
> 
> Op 03-11-11 13:09, Takashi Iwai schreef:
> > In some weird situation, HP DriveGuard chip can't read ODR value
> > correctly, and it results in a zero-division Oops in lis3lv02d driver.
> > This patch fixes the Oops by checking the value appopriately, and skips
> > if any weird value is read.
> Hi Takashi,
> Actually, a similar patch already just landed in linus' tree:
> 1510dd5954 (lis3lv02d: avoid divide by zero due to unchecked)
> 
> However, in the patch applied, the device is disabled (until next 
> reboot) while in yours, the sleep is just skipped. Does it work again 
> after the read of odr fails? If so, maybe I could improve the current 
> version by, after the odr read fails, sleeping a long and safe time and 
> then trying to read the odr again. Then if it fails again, we give up, 
> otherwise the device can be used again.

I guess it's possible to use the device afterward.  The possible
reason is either the chip is set to an invalid mode or ACPI isn't set
up properly.  But this path usually means that ACPI does work more or
less since you could read WHOAMI.

> Do you have such a device yourself? Could you let me know if after a 
> failing read of the odr, the device keeps working?

I have a machine but I'm not quite sure how to reproduce this error.
It happened casually during the installation of a new system, so it's
not so trivial to switch the module during it...


thanks,

Takashi

> 
> Cheers,
> Éric
> 
> >
> > Cc:<stable@kernel.org>
> > Signed-off-by: Takashi Iwai<tiwai@suse.de>
> > ---
> >   drivers/misc/lis3lv02d/lis3lv02d.c |   14 +++++++++++---
> >   1 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
> > index 8b51cd6..c4eb2f3 100644
> > --- a/drivers/misc/lis3lv02d/lis3lv02d.c
> > +++ b/drivers/misc/lis3lv02d/lis3lv02d.c
> > @@ -228,6 +228,14 @@ static int lis3lv02d_set_odr(int rate)
> >   	return -EINVAL;
> >   }
> >
> > +static void lis3lv02d_power_delay(struct lis3lv02d *lis3)
> > +{
> > +	int odr = lis3lv02d_get_odr();
> > +	if (odr<= 0)
> > +		return;
> > +	msleep(lis3->pwron_delay / odr);
> > +}
> > +
> >   static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
> >   {
> >   	u8 ctlreg, reg;
> > @@ -266,7 +274,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
> >
> >   	lis3->read(lis3, ctlreg,&reg);
> >   	lis3->write(lis3, ctlreg, (reg | selftest));
> > -	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> > +	lis3lv02d_power_delay(lis3);
> >
> >   	/* Read directly to avoid axis remap */
> >   	x = lis3->read_data(lis3, OUTX);
> > @@ -275,7 +283,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
> >
> >   	/* back to normal settings */
> >   	lis3->write(lis3, ctlreg, reg);
> > -	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> > +	lis3lv02d_power_delay(lis3);
> >
> >   	results[0] = x - lis3->read_data(lis3, OUTX);
> >   	results[1] = y - lis3->read_data(lis3, OUTY);
> > @@ -385,7 +393,7 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
> >   	}
> >
> >   	/* LIS3 power on delay is quite long */
> > -	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
> > +	lis3lv02d_power_delay(lis3);
> >
> >   	if (lis3->reg_ctrl)
> >   		lis3_context_restore(lis3);
> 

Re: [PATCH] lis3lv02d: Avoid zero-division

[Éric Piel]

Op 08-11-11 07:19, Takashi Iwai schreef:
> At Tue, 08 Nov 2011 04:57:42 +0100,
> Éric Piel wrote:
>>
>> Op 03-11-11 13:09, Takashi Iwai schreef:
>>> In some weird situation, HP DriveGuard chip can't read ODR value
>>> correctly, and it results in a zero-division Oops in lis3lv02d driver.
>>> This patch fixes the Oops by checking the value appopriately, and skips
>>> if any weird value is read.
>> Hi Takashi,
>> Actually, a similar patch already just landed in linus' tree:
>> 1510dd5954 (lis3lv02d: avoid divide by zero due to unchecked)
>>
>> However, in the patch applied, the device is disabled (until next
>> reboot) while in yours, the sleep is just skipped. Does it work again
>> after the read of odr fails? If so, maybe I could improve the current
>> version by, after the odr read fails, sleeping a long and safe time and
>> then trying to read the odr again. Then if it fails again, we give up,
>> otherwise the device can be used again.
>
> I guess it's possible to use the device afterward.  The possible
> reason is either the chip is set to an invalid mode or ACPI isn't set
> up properly.  But this path usually means that ACPI does work more or
> less since you could read WHOAMI.
>
>> Do you have such a device yourself? Could you let me know if after a
>> failing read of the odr, the device keeps working?
>
> I have a machine but I'm not quite sure how to reproduce this error.
> It happened casually during the installation of a new system, so it's
> not so trivial to switch the module during it...
Dear Takashi,

I've looked more at the error. Now it seems to me that the fact that 
get_odr() returns 0 is not because there is a problem with the device 
but just because it is powered off: for the "3dc" device, this reflects 
in rate of 0. We assume that the ACPI code does turn the device on, but 
it might not do it, or the device might still need more time to fully 
initialize. So, just turning the device on and waiting a bit long should 
work fine.

However, looking for the spec document of lis3dc (or hp3dc), I couldn't 
find any reference to such device. I've found documents for lis3dh, with 
apparently same registers and WHOAMI value. However, contrarily to what 
is currently expected, it's a 16-bit device, not 8-bit. So my main 
question is: are you sure the device you have is 8-bit? Isn't your 
device a 16-bit lis3dh? Do you have the spec of lis3dc/hp3dc?

If it's indeed a 16-bit device, I'll update the support for this device 
to obtain a better precision.

Éric

Re: [PATCH] lis3lv02d: Avoid zero-division

[Takashi Iwai]

At Fri, 11 Nov 2011 19:11:06 +0100,
Éric Piel wrote:
> 
> Op 08-11-11 07:19, Takashi Iwai schreef:
> > At Tue, 08 Nov 2011 04:57:42 +0100,
> > Éric Piel wrote:
> >>
> >> Op 03-11-11 13:09, Takashi Iwai schreef:
> >>> In some weird situation, HP DriveGuard chip can't read ODR value
> >>> correctly, and it results in a zero-division Oops in lis3lv02d driver.
> >>> This patch fixes the Oops by checking the value appopriately, and skips
> >>> if any weird value is read.
> >> Hi Takashi,
> >> Actually, a similar patch already just landed in linus' tree:
> >> 1510dd5954 (lis3lv02d: avoid divide by zero due to unchecked)
> >>
> >> However, in the patch applied, the device is disabled (until next
> >> reboot) while in yours, the sleep is just skipped. Does it work again
> >> after the read of odr fails? If so, maybe I could improve the current
> >> version by, after the odr read fails, sleeping a long and safe time and
> >> then trying to read the odr again. Then if it fails again, we give up,
> >> otherwise the device can be used again.
> >
> > I guess it's possible to use the device afterward.  The possible
> > reason is either the chip is set to an invalid mode or ACPI isn't set
> > up properly.  But this path usually means that ACPI does work more or
> > less since you could read WHOAMI.
> >
> >> Do you have such a device yourself? Could you let me know if after a
> >> failing read of the odr, the device keeps working?
> >
> > I have a machine but I'm not quite sure how to reproduce this error.
> > It happened casually during the installation of a new system, so it's
> > not so trivial to switch the module during it...
> Dear Takashi,
> 
> I've looked more at the error. Now it seems to me that the fact that 
> get_odr() returns 0 is not because there is a problem with the device 
> but just because it is powered off: for the "3dc" device, this reflects 
> in rate of 0. We assume that the ACPI code does turn the device on, but 
> it might not do it, or the device might still need more time to fully 
> initialize. So, just turning the device on and waiting a bit long should 
> work fine.

Sounds reasonable.  FWIW, I got the zero-division error only in a rare
case, while the installation of a system.

> However, looking for the spec document of lis3dc (or hp3dc), I couldn't 
> find any reference to such device. I've found documents for lis3dh, with 
> apparently same registers and WHOAMI value. However, contrarily to what 
> is currently expected, it's a 16-bit device, not 8-bit. So my main 
> question is: are you sure the device you have is 8-bit? Isn't your 
> device a 16-bit lis3dh? Do you have the spec of lis3dc/hp3dc?

The device works with 8bit mode well, and the values I get from the
joystick device is almost same as the old lis3 chip on other HP
laptops.  So I suppose this is 8bit device.

I have also no spec for this chip.  I was informed from HP a little
bit about their new stuff to add the support in hp_accel driver, but
not in details, unfortunately.


thanks,

Takashi