linux-kernel - Re: hp accelerometer: add freefall detection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <4992C57E.9050109@tremplin-utc.net>
Date:	Wed, 11 Feb 2009 13:33:02 +0100
From:	Éric Piel <eric.piel@...mplin-utc.net>
To:	pavel@...e.cz
Cc:	akpm@...ux-foundation.org, LKML <linux-kernel@...r.kernel.org>,
	trenn@...e.de
Subject: Re: hp accelerometer: add freefall detection

From: Pavel Machek <pavel@...e.cz>
> 
> This adds freefall handling to hp_accel driver.  According to HP, it
> should just work, without us having to set the chip up by hand.
> 
> hpfall.c is example .c program that parks the disk when accelerometer
> detects free fall.  It should work; for now, it uses fixed 20seconds
> protection period.
> 
> Signed-off-by: Pavel Machek <pavel@...e.cz>
> Cc: Thomas Renninger <trenn@...e.de>
> Cc: �ric Piel <eric.piel@...mplin-utc.net>
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
Hi Pavel,
Sorry for being amazingly late, reading the latest patches from
Giuseppe, I noticed some parts in this patch which are strange.
Basically you scrap all the init and all the poweroff. Some of the
things there were put for some purpose, and I wonder if you've verified
everything keeps working correctly.


> @@ -110,26 +114,13 @@ static void lis3lv02d_get_xyz(acpi_handl
>  void lis3lv02d_poweroff(acpi_handle handle)
>  {
>  	adev.is_on = 0;
> -	/* disable X,Y,Z axis and power down */
> -	adev.write(handle, CTRL_REG1, 0x00);
So we are not turning off the device on poweroff()?! Why? Did you notice
problems when we were doing it?

Actually I've always wondered if it was worthy to turn it off because
I've never been able to measure a difference in power consumption. Have
you done the measurement and noticed there is no effect? I'd be glad we
can get rid of this power management thing, but then we can scratch much
more than just this line!


> -	/*
> -	 * BDU: LSB and MSB values are not updated until both have been read.
> -	 *      So the value read will always be correct.
> -	 * IEN: Interrupt for free-fall and DD, not for data-ready.
> -	 */
> -	adev.read(handle, CTRL_REG2, &val);
> -	val |= CTRL2_BDU | CTRL2_IEN;
> -	adev.write(handle, CTRL_REG2, val);
This part is rather scary. Did you read the comment? If you don't
activate BDU you cannot be sure that the data you read is valid. I see
no reason why this would interfere with the free-fall detection so
unless you have a really good case, let's keep it. On the other hand, I
don't mind removing IEN if you've noticed that it's not useful or
interfering.

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