[PATCH 00/10] lis3: various fixes and enhancements

[PATCH 00/10] lis3: various fixes and enhancements

[Éric Piel]

Hello,

Here are a couple of patches from my backlog for the lis3. Since the 
move out of hwmon, I'm not entirely sure to who I should send the 
patches for the part which lies in driver/misc. Maybe Andrew can pick 
them, or should I send them directly to Linus?

Patch 1 avoids crashes with a buggy hardware (which is actually seen in 
the wild). Patch 7 fixes a missing free(). The rest of the patches are 
more enhancements. Iilka had actually sent another patch, coming as 
number 11, to allow multiple devices at the same time. But Cascardo 
noticed that it can cause problems with the misc device, so I'm not 
including it. Iilka, maybe have you found a solution since then?

Cheers,
Éric

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

[Éric Piel]

After an "unexpected" reboot, I found this Oops in my logs:

divide error: 0000 [#1] PREEMPT SMP=20
CPU 0=20
Modules linked in: lis3lv02d hp_wmi input_polldev [...]
Pid: 390, comm: modprobe Tainted: G         C  2.6.39-rc7-wl+=20
RIP: 0010:[<ffffffffa014b427>]  [<ffffffffa014b427>]
		 lis3lv02d_poweron+0x4e/0x94 [lis3lv02d]
RSP: 0018:ffff8801d6407cf8  EFLAGS: 00010246
RAX: 0000000000000bb8 RBX: ffffffffa014e000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffea00066e4708 RDI: ffff8801df002700
RBP: ffff8801d6407d18 R08: ffffea00066c5a30 R09: ffffffff812498c9
R10: ffff8801d7bfcea0 R11: ffff8801d7bfce10 R12: 0000000000000bb8
R13: 00000000ffffffda R14: ffffffffa0154120 R15: ffffffffa0154030
=46S:  00007fc0705db700(0000) GS:ffff8801dfa00000(0000) knlGS:0
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f33549174f0 CR3: 00000001d65c9000 CR4: 00000000000406f0
Process modprobe (pid: 390, threadinfo ffff8801d6406000, task ffff8801d6b40=
000)
Stack:
 ffffffffa0154120 62ffffffa0154030 ffffffffa014e000 00000000ffffffea
 ffff8801d6407d58 ffffffffa014bcc1 0000000000000000 0000000000000048
 ffff8801d8bae800 00000000ffffffea 00000000ffffffda ffffffffa0154120
Call Trace:
 [<ffffffffa014bcc1>] lis3lv02d_init_device+0x1ce/0x496 [lis3lv02d]
 [<ffffffffa01522ff>] lis3lv02d_add+0x10f/0x17c [hp_accel]
 [<ffffffff81233e11>] acpi_device_probe+0x49/0x117
[...]
Code: 3a 75 06 80 4d ef 50 eb 04 80 4d ef 40 0f b6 55 ef be 21
00 00 00 48 89 df ff 53 18 44 8b 63 6c e8 3e fc ff ff 89 c1 44
89 e0 99 <f7> f9 89 c7 e8 93 82 ef e0 48 83 7b 30 00 74 2d 45
31 e4 80 7b=20
RIP  [<ffffffffa014b427>] lis3lv02d_poweron+0x4e/0x94 [lis3lv02d]
 RSP <ffff8801d6407cf8>

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.

For now, I fixed it the easiest and most obvious way:
Check if the result is sane and if it isn't use a sane default
instead. I went for "100" in the latter case, simply because
/sys/devices/platform/lis3lv02d/rate returns it on a successful
boot.

Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 drivers/misc/lis3lv02d/lis3lv02d.c |   36 ++++++++++++++++++++++++++++++------
 drivers/misc/lis3lv02d/lis3lv02d.h |    2 +-
 drivers/platform/x86/hp_accel.c    |    3 +--
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
index b928bc1..a9ee5f7 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d.c
@@ -206,6 +206,18 @@ static int lis3lv02d_get_odr(void)
 	return lis3_dev.odrs[(ctrl >> shift)];
 }
 
+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;
+}
+
 static int lis3lv02d_set_odr(int rate)
 {
 	u8 ctrl;
@@ -266,7 +278,9 @@ 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());
+	ret = lis3lv02d_get_pwron_wait(lis3);
+	if (ret)
+		goto fail;
 
 	/* Read directly to avoid axis remap */
 	x = lis3->read_data(lis3, OUTX);
@@ -275,7 +289,9 @@ 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());
+	ret = lis3lv02d_get_pwron_wait(lis3);
+	if (ret)
+		goto fail;
 
 	results[0] = x - lis3->read_data(lis3, OUTX);
 	results[1] = y - lis3->read_data(lis3, OUTY);
@@ -363,8 +379,9 @@ void lis3lv02d_poweroff(struct lis3lv02d *lis3)
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
 
-void lis3lv02d_poweron(struct lis3lv02d *lis3)
+int lis3lv02d_poweron(struct lis3lv02d *lis3)
 {
+	int err;
 	u8 reg;
 
 	lis3->init(lis3);
@@ -382,11 +399,14 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
 		reg |= CTRL2_BOOT_8B;
 	lis3->write(lis3, CTRL_REG2, reg);
 
-	/* LIS3 power on delay is quite long */
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	err = lis3lv02d_get_pwron_wait(lis3);
+	if (err)
+		return err;
 
 	if (lis3->reg_ctrl)
 		lis3_context_restore(lis3);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweron);
 
@@ -926,7 +946,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 	atomic_set(&dev->wake_thread, 0);
 
 	lis3lv02d_add_fs(dev);
-	lis3lv02d_poweron(dev);
+	err = lis3lv02d_poweron(dev);
+	if (err) {
+		lis3lv02d_remove_fs(dev);
+		return err;
+	}
 
 	if (dev->pm_dev) {
 		pm_runtime_set_active(dev->pm_dev);
diff --git a/drivers/misc/lis3lv02d/lis3lv02d.h b/drivers/misc/lis3lv02d/lis3lv02d.h
index a193958..57c64bb 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.h
+++ b/drivers/misc/lis3lv02d/lis3lv02d.h
@@ -285,7 +285,7 @@ int lis3lv02d_init_device(struct lis3lv02d *lis3);
 int lis3lv02d_joystick_enable(void);
 void lis3lv02d_joystick_disable(void);
 void lis3lv02d_poweroff(struct lis3lv02d *lis3);
-void lis3lv02d_poweron(struct lis3lv02d *lis3);
+int lis3lv02d_poweron(struct lis3lv02d *lis3);
 int lis3lv02d_remove_fs(struct lis3lv02d *lis3);
 
 extern struct lis3lv02d lis3_dev;
diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_accel.c
index 1b52d00..891e71f 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -354,8 +354,7 @@ static int lis3lv02d_suspend(struct acpi_device *device, pm_message_t state)
 
 static int lis3lv02d_resume(struct acpi_device *device)
 {
-	lis3lv02d_poweron(&lis3_dev);
-	return 0;
+	return lis3lv02d_poweron(&lis3_dev);
 }
 #else
 #define lis3lv02d_suspend NULL
-- 
1.7.6

[PATCH 02/10] lis3: update maintainer information

[Éric Piel]

In the move of the lis3 driver, the hp_accel.c file got dropped from the
MAINTAINER file. Make it explicit again that this file is tied to lis3
again.

Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 MAINTAINERS |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 187282d..00f7325 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3937,6 +3937,7 @@ M:	Eric Piel <eric.piel@tremplin-utc.net>
 S:	Maintained
 F:	Documentation/misc-devices/lis3lv02d
 F:	drivers/misc/lis3lv02d/
+F:	drivers/platform/x86/hp_accel.c
 
 LLC (802.2)
 M:	Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
-- 
1.7.6

[PATCH 03/10] lis3: add support for HP EliteBook 2730p

[Éric Piel]

Add axis correction for HP EliteBook 2730p.

Tested-by: Witold Pilat <witold.pilat@gmail.com>
Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 drivers/platform/x86/hp_accel.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_accel.c
index 891e71f..76b24e6 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -209,6 +209,7 @@ static struct dmi_system_id lis3lv02d_dmi_ids[] = {
 	AXIS_DMI_MATCH("NC6715x", "HP Compaq 6715", y_inverted),
 	AXIS_DMI_MATCH("NC693xx", "HP EliteBook 693", xy_rotated_right),
 	AXIS_DMI_MATCH("NC693xx", "HP EliteBook 853", xy_swap),
+	AXIS_DMI_MATCH("NC273xx", "HP EliteBook 273", y_inverted),
 	/* Intel-based HP Pavilion dv5 */
 	AXIS_DMI_MATCH2("HPDV5_I",
 			PRODUCT_NAME, "HP Pavilion dv5",
-- 
1.7.6

[PATCH 04/10] lis3: add support for HP EliteBook 8540w

[Éric Piel]

Add axis correction for HP EliteBook 8540w.

Reported-by: Lyall Pearce <lyall.pearce@hp.com>
Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 drivers/platform/x86/hp_accel.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_accel.c
index 76b24e6..626b162 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -209,6 +209,7 @@ static struct dmi_system_id lis3lv02d_dmi_ids[] = {
 	AXIS_DMI_MATCH("NC6715x", "HP Compaq 6715", y_inverted),
 	AXIS_DMI_MATCH("NC693xx", "HP EliteBook 693", xy_rotated_right),
 	AXIS_DMI_MATCH("NC693xx", "HP EliteBook 853", xy_swap),
+	AXIS_DMI_MATCH("NC854xx", "HP EliteBook 854", y_inverted),
 	AXIS_DMI_MATCH("NC273xx", "HP EliteBook 273", y_inverted),
 	/* Intel-based HP Pavilion dv5 */
 	AXIS_DMI_MATCH2("HPDV5_I",
-- 
1.7.6

[PATCH 05/10] hp_accel: Add HP ProBook 655x

[Éric Piel]

Add axis correction for HP ProBook 6555b.

Signed-off-by: Malte Starostik <m-starostik@versanet.de>
Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 drivers/platform/x86/hp_accel.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_accel.c
index 626b162..89c5cb5 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -229,6 +229,7 @@ static struct dmi_system_id lis3lv02d_dmi_ids[] = {
 	AXIS_DMI_MATCH("HPB452x", "HP ProBook 452", y_inverted),
 	AXIS_DMI_MATCH("HPB522x", "HP ProBook 522", xy_swap),
 	AXIS_DMI_MATCH("HPB532x", "HP ProBook 532", y_inverted),
+	AXIS_DMI_MATCH("HPB655x", "HP ProBook 655", xy_swap_inverted),
 	AXIS_DMI_MATCH("Mini510x", "HP Mini 510", xy_rotated_left_usd),
 	{ NULL, }
 /* Laptop models without axis info (yet):
-- 
1.7.6

[PATCH 06/10] CONFIG_HP_ACCEL: Fix help text

[Éric Piel]

Adapt the help text for CONFIG_HP_ACCEL to the move of
Documentation/hwmon/lis3lv02d to Documentation/misc-devices/ in
ff606677f6a47c63329cf8e6c7cf978c29f2d736

Signed-off-by: Malte Starostik <m-starostik@versanet.de>
Signed-off-by: Éric Piel <eric.piel@tremplin-utc.net>
---
 drivers/platform/x86/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 45e0191..d25f486 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -164,7 +164,7 @@ config HP_ACCEL
 
 	  Support for a led indicating disk protection will be provided as
 	  hp::hddprotect. For more information on the feature, refer to
-	  Documentation/hwmon/lis3lv02d.
+	  Documentation/misc-devices/lis3lv02d.
 
 	  To compile this driver as a module, choose M here: the module will
 	  be called hp_accel.
-- 
1.7.6

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

[Andrew Morton]

On Mon, 25 Jul 2011 17:16:23 +0200
__ric Piel <eric.piel@tremplin-utc.net> 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?

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

[Christian Lamparter]

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@tremplin-utc.net> 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?

Regards,
	Chr

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

[Andrew Morton]

On Mon, 1 Aug 2011 23:11:17 +0200
Christian Lamparter <chunkeey@googlemail.com> 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@tremplin-utc.net> 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?

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@googlemail.com>  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@tremplin-utc.net>  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