Skip to contentSkip to conversation

Laptop shock detection and harddisk protection

DISCUSSION Tejun Heo 10 participants 31 messages Aug 17, 2008 – Oct 7, 2008 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

Hello, all.

Elias has been sending libata/ide shock protection patches[1] and for libata I think it's only one or two iterations from being included. The interface is pretty simple, it only has to write timeout values to sysfs nodes for all disks.

Thomas is now working on implementing shock detection for HP laptops where the interface is much simpler than the HDAPS one. Interrupts for danger imminent and danger over plus control for warning LED.

I browsed a little bit for HDAPS one and it seems all the pieces are there but scattered. The latest effort seems tp_smapi which Shem Multinymous is working on.

So, overall, all the pieces are falling into places but many pieces are not upstream yet and there also are interface differences for different detection drivers. The original hdaps uses polling on sysfs nodes, the HP thing Thomas is working on is likely to use sysfs + sysfs_notify_event() and probably another node to control LED. The tp_smapi seems to use joystick input device to transfer the data along all the axises.

So, I think it's about time to decide how to proceed on this acceleration detection and shock protection thing.

1. How should the shock interface look like? As we're gonna need userland daemon one way or the other, we can use the userland daemon to glue all the interfaces but it would be much better to have a unified interface. Although there seem to be several different variants, they don't differ all that much and creating a new interface every time is painful. I think we can get by with a sysfs interface with notification.

2. If we're gonna unify interface, how much can we unify the backend? Some devices are based on polling, others interrupt. For polling, is it better to delegate the whole polling to userland or is it better to do some of it in kernel (tp_smapi seems to be doing this)?

3. What about the userland daemon? It would be best to have a unified daemon which can handle all instead of one for hdaps and another for hp (and so on). If we can unify the interface, this will be much easier.

Thanks.

Signature · 3 lines
tejun

[1] http://thread.gmane.org/gmane.linux.ide/34103
#1 lore raw

Hi Tejun,

Quoted 6 lines · Tejun Heo

The original hdaps uses polling on sysfs
nodes, the HP thing Thomas is working on is likely to use sysfs + sysfs_notify_event() and probably another node to control LED. The tp_smapi seems to use joystick input device to transfer the data along all the axises.

Quoted 7 lines

1. How should the shock interface look like? As we're gonna need userland daemon one way or the other, we can use the userland daemon to glue all the interfaces but it would be much better to have a unified interface. Although there seem to be several different variants, they don't differ all that much and creating a new interface every time is painful. I think we can get by with a sysfs interface with notification.

Using the input device interface for the accelerometer (as done by tp_smapi's hdaps + latest hdapsd) greatly reduces the number of accelerometer-related timer interrupts on tickless kernels, as measured by powertop. With syscall polling you have the kernal polling the hardware at ~50Hz and then the userspace hdapsd polling the kernel at ~50Hz. When they're out of phase so you can get up to 100 interrupts/sec. With an input device you're always at 50Hz. The phase difference also induces a small extra delay in the shock handling response.

Quoted 5 lines

2. If we're gonna unify interface, how much can we unify the backend? Some devices are based on polling, others interrupt. For polling, is it better to delegate the whole polling to userland or is it better to do some of it in kernel (tp_smapi seems to be doing this)?

The ThinkPad accelerometer needs to be polled at very regular intervals (max jitter on the order of 10ms), which sounds like a job for the kernel.
This is because in ThinkPads we actually have a 4-level pile:
[hdapsd userspace] -> [hdaps kernel] -> [embedded controller] -> [accelerometer A2D]
What the kernel polls is actually is the H8S embedded controller (EC) chip, which in turn does its own polling of the accelerometer A2D. Now, the EC has a tiny buffer and strange buffering semantics, and it has its own internal clock, so the software->EC polling should be very regular to minmize EC buffer overflows/underruns.

Quoted 4 lines

3. What about the userland daemon? It would be best to have a unified daemon which can handle all instead of one for hdaps and another for hp (and so on). If we can unify the interface, this will be much easier.

Assuming the funky shock detection algorithms are done in userspace, we need two interfaces: one for the acceleration data (an input device works nicely) and one for "unload now" events.
On HP, the kernel can trigger the "unload now" event directly. On ThinkPads, there's still the question of what the userspace shock detection daemon should do when it detects a shock: should it unloads the heads by itself, or trigger the generic "unload now" event and let someone else do the actual unloading.

Shem

author
#2 lore raw

Hello, Shem Multinymous.

Quoted 9 lines · Shem Multinymous

Hi Tejun,

Quoted 7 lines

1. How should the shock interface look like? As we're gonna need userland daemon one way or the other, we can use the userland daemon to glue all the interfaces but it would be much better to have a unified interface. Although there seem to be several different variants, they don't differ all that much and creating a new interface every time is painful. I think we can get by with a sysfs interface with notification.

Quoted 9 lines

Using the input device interface for the accelerometer (as done by tp_smapi's hdaps + latest hdapsd) greatly reduces the number of accelerometer-related timer interrupts on tickless kernels, as measured by powertop. With syscall polling you have the kernal polling the hardware at ~50Hz and then the userspace hdapsd polling the kernel at ~50Hz. When they're out of phase so you can get up to 100 interrupts/sec. With an input device you're always at 50Hz. The phase difference also induces a small extra delay in the shock handling response.

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Quoted 9 lines
Quoted 5 lines

2. If we're gonna unify interface, how much can we unify the backend? Some devices are based on polling, others interrupt. For polling, is it better to delegate the whole polling to userland or is it better to do some of it in kernel (tp_smapi seems to be doing this)?

The ThinkPad accelerometer needs to be polled at very regular intervals (max jitter on the order of 10ms), which sounds like a job for the kernel.

Yes, I agree.

Quoted 8 lines

This is because in ThinkPads we actually have a 4-level pile:
[hdapsd userspace] -> [hdaps kernel] -> [embedded controller] -> [accelerometer A2D]
What the kernel polls is actually is the H8S embedded controller (EC) chip, which in turn does its own polling of the accelerometer A2D. Now, the EC has a tiny buffer and strange buffering semantics, and it has its own internal clock, so the software->EC polling should be very regular to minmize EC buffer overflows/underruns.

So, I think the whole polling should be implemented inside the kernel and the kernel should notify userland when new data is available, which is about what the current joystick implementation does and can be achieved using sysfs_notify_event().

Quoted 5 lines
Quoted 4 lines

3. What about the userland daemon? It would be best to have a unified daemon which can handle all instead of one for hdaps and another for hp (and so on). If we can unify the interface, this will be much easier.

Quoted 8 lines

Assuming the funky shock detection algorithms are done in userspace, we need two interfaces: one for the acceleration data (an input device works nicely) and one for "unload now" events.
On HP, the kernel can trigger the "unload now" event directly. On ThinkPads, there's still the question of what the userspace shock detection daemon should do when it detects a shock: should it unloads the heads by itself, or trigger the generic "unload now" event and let someone else do the actual unloading.

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

Thanks.

Signature · 1 line
tejun
#3 lore raw

Hi Tejun,

Quoted 11 lines · Tejun Heo

Hello, Shem Multinymous.

Quoted 9 lines

Using the input device interface for the accelerometer (as done by tp_smapi's hdaps + latest hdapsd) greatly reduces the number of accelerometer-related timer interrupts on tickless kernels, as measured by powertop. With syscall polling you have the kernal polling the hardware at ~50Hz and then the userspace hdapsd polling the kernel at ~50Hz. When they're out of phase so you can get up to 100 interrupts/sec. With an input device you're always at 50Hz. The phase difference also induces a small extra delay in the shock handling response.

Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Agreed.
There's another issue with the current sysfs interface, though: hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Quoted 3 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Shem

#4 lore raw

Hi!

Quoted 5 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer.

I'm willing to maintain this.
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#5 lore raw
In reply to Shem Multinymous
Quoted 11 lines · Shem Multinymous
Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Agreed.
There's another issue with the current sysfs interface, though: hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Yes, right. Forgot about the atomicity part altogether. Thanks for bringing it up.

Quoted 9 lines
Quoted 3 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Hmmm... I can't (yet) see the benefit of having two separate userland daemons.

Quoted 9 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Thanks.

Signature · 1 line
tejun
#6 lore raw
Quoted 39 lines · Tejun Heo
Quoted 11 lines · Shem Multinymous
Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Agreed.
There's another issue with the current sysfs interface, though: hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Yes, right. Forgot about the atomicity part altogether. Thanks for bringing it up.

Quoted 9 lines
Quoted 3 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Hmmm... I can't (yet) see the benefit of having two separate userland daemons.

Quoted 9 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Quoted 3 lines

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

We went over this before a number of years ago, that's why this code isn't in mainline :(

In short, "Signed-off-by:" from people who are known to be anonymous is not allowed.

thanks,

greg k-h

author
#7 lore raw
Quoted 12 lines · Greg KH
Quoted 11 lines
Quoted 8 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Quoted 6 lines

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

We went over this before a number of years ago, that's why this code isn't in mainline :(

Aieee... didn't know that and was wondering why the hell this wasn't in mainline. :-(

Quoted 2 lines

In short, "Signed-off-by:" from people who are known to be anonymous is not allowed.

Shem Multinymous, I suppose nothing really has changd since the last time?

Thanks.

Signature · 1 line
tejun
#8 lore raw

Hi,

Quoted 9 lines · Tejun Heo
Quoted 6 lines

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

We went over this before a number of years ago, that's why this code isn't in mainline :(

Shem Multinymous, I suppose nothing really has changd since the last time?

The only pertinent change is that I've added even more documentation to thinkpad_ec.c, explaining how we're just following the published H8S specs (plus some workarounds for experimentally observed bugs).

As noted before, I'd be more than happy to assign away copyrights or discuss things further in private if this would help.

Shem

author
#9 lore raw
Quoted 10 lines · Shem Multinymous
Quoted 8 lines · Tejun Heo
Quoted 6 lines

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

We went over this before a number of years ago, that's why this code isn't in mainline :(

Shem Multinymous, I suppose nothing really has changd since the last time?

Quoted 6 lines

The only pertinent change is that I've added even more documentation to thinkpad_ec.c, explaining how we're just following the published H8S specs (plus some workarounds for experimentally observed bugs).

As noted before, I'd be more than happy to assign away copyrights or discuss things further in private if this would help.

I bet you had enough discussion about this last time but any chance you can break out of the anonymity? :-)

Thanks.

Signature · 1 line
tejun
#10 lore raw
In reply to Greg KH
Quoted 40 lines · Greg KH
Quoted 39 lines · Tejun Heo
Quoted 11 lines · Shem Multinymous
Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Agreed.
There's another issue with the current sysfs interface, though: hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Yes, right. Forgot about the atomicity part altogether. Thanks for bringing it up.

Quoted 9 lines
Quoted 3 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Hmmm... I can't (yet) see the benefit of having two separate userland daemons.

Quoted 9 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Quoted 7 lines
Quoted 3 lines

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

So... what are you worried about?

Quoted 2 lines

In short, "Signed-off-by:" from people who are known to be anonymous is not allowed.

That's okay. Signed-off-by: by original author is not required for mainline merge. All that is required is submitter legally getting it under GPL, and signing that off.

Now.. what are you worried about?

a) Copyrights?

b) Patents?

c) Trademarks?

d) Trade secrets?

e) Contracts between Shem and ...?

f) Some other contracts?

g) any other applicable laws? which?

h) anything else?

i) anything you can't talk about because NDAs or similar?

Based on the answer, we can try to figure way forward.

('power_now' is really useful for me; useful enough that I think about reverse-engineering it from tp_smapi and reimplementation. Maybe even doing it chinese-wall way if I can find second interested person. But I need to know what the concerns are. ...plus, I guess Ciaran can help with legal questions....)

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#11 lore raw
Quoted 45 lines · Pavel Machek
Quoted 44 lines · Greg KH
Quoted 42 lines · Tejun Heo
Quoted 11 lines · Shem Multinymous
Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Agreed.
There's another issue with the current sysfs interface, though: hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Yes, right. Forgot about the atomicity part altogether. Thanks for bringing it up.

Quoted 9 lines
Quoted 3 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Hmmm... I can't (yet) see the benefit of having two separate userland daemons.

Quoted 9 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Quoted 5 lines
Quoted 3 lines

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

So... what are you worried about?

Code created by access to specs that were not allowed to be published in GPL form by someone who wants to remain anonymous.

Come on, we went over this the last time this came up a few years ago. It's just not going to happen due to the legal issues involved, sorry.

If people want to get this kind of code into the tree, they can write a new driver from scratch, based on public information on these chips. And you will have to defend exactly where this information was found as you can not do it by information found in this "tainted" driver, sorry, that's not a legally viable way forward.

thanks,

greg k-h

#12 lore raw
Quoted 46 lines · Greg KH
Quoted 45 lines · Pavel Machek
Quoted 44 lines · Greg KH
Quoted 42 lines · Tejun Heo
Quoted 11 lines · Shem Multinymous
Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Agreed.
There's another issue with the current sysfs interface, though: hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Yes, right. Forgot about the atomicity part altogether. Thanks for bringing it up.

Quoted 9 lines
Quoted 3 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Hmmm... I can't (yet) see the benefit of having two separate userland daemons.

Quoted 9 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

So... what are you worried about?

Code created by access to specs that were not allowed to be published in GPL form by someone who wants to remain anonymous.

That anonymous person may have problems if they signed NDA.

I don't think they did, they even list the sources:

 *  The embedded controller on ThinkPad laptops has a non-standard interface,
 *  where LPC channel 3 of the H8S EC chip is hooked up to IO ports
 *  0x1600-0x161F and implements (a special case of) the H8S LPC protocol.
 *  The EC LPC interface provides various system management services (currently
 *  known: battery information and accelerometer readouts). This driver
 *  provides access and mutual exclusion for the EC interface.
*
 *  The LPC protocol and terminology is documented here:
 *  "H8S/2104B Group Hardware Manual",
 *  http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf

H8S chip seems to be documented.

...even if you are right, why is it problem for _us_. We are not covered by NDAs we did not sign.

So can you list your concerns for _us_? Copyrights? Patents? Trade secrets? Contracts?

Quoted 4 lines

If people want to get this kind of code into the tree, they can write a new driver from scratch, based on public information on these chips. And you will have to defend exactly where this information was found as

These are rather bigger requirements than signed-off-by and then what Novell legal people require before putting stuff in distribution. So you should explain why this bigger requirements are warranted.

Quoted 2 lines

you can not do it by information found in this "tainted" driver, sorry, that's not a legally viable way forward.

As far as I can see, using any information I find anywhere is perfectly legal for writing a driver, as long as I don't sign & violate a NDA. sourceforge.net is rather well-known, public source of information. Why should it be treated specially?!
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#13 lore raw
Quoted 75 lines · On Thursday 18 September 2008 13:18:45 Pavel Machek wrote:
Quoted 48 lines · Greg KH
Quoted 47 lines · Pavel Machek
Quoted 46 lines · Greg KH
Quoted 44 lines · Tejun Heo
Quoted 5 lines · Shem Multinymous
Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Quoted 7 lines

Agreed.
There's another issue with the current sysfs interface, though:
hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Yes, right. Forgot about the atomicity part altogether. Thanks for bringing it up.

Quoted 10 lines
Quoted 4 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Hmmm... I can't (yet) see the benefit of having two separate userland daemons.

Quoted 9 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

So... what are you worried about?

Code created by access to specs that were not allowed to be published in GPL form by someone who wants to remain anonymous.

That anonymous person may have problems if they signed NDA.

I don't think they did, they even list the sources:

* The embedded controller on ThinkPad laptops has a non-standard
interface, * where LPC channel 3 of the H8S EC chip is hooked up to IO ports * 0x1600-0x161F and implements (a special case of) the H8S LPC protocol. * The EC LPC interface provides various system management services (currently * known: battery information and accelerometer readouts). This driver * provides access and mutual exclusion for the EC interface.
*
* The LPC protocol and terminology is documented here:
* "H8S/2104B Group Hardware Manual",
*
http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf

Quoted 1 line

H8S chip seems to be documented.

Hmm, the EC is not directly used, but ACPI functions of the HP device are used.
For the HP ACPI device: the ACPI functions can *very easily* be re-engineered (which is common for all laptop_acpi.ko drivers):
ALRD -> is used by the driver to read out registers of the accelerometer ALWR -> is used by the driver to write a registers of the accelerometer
BTW: HP likes to have support for their device.

The acceleromter chip itself is docuemented in detail here:
http://www.st.com/stonline/products/literature/ds/12094/lis3lv02dl.pdf

I also do not see any concerns.
Greg: Can you please add this one or explain in more detail what else you like to see to get this integerated.

Thanks,

Thomas

#14 lore raw
Quoted 85 lines · Thomas Renninger
Quoted 75 lines · On Thursday 18 September 2008 13:18:45 Pavel Machek wrote:
Quoted 48 lines · Greg KH
Quoted 47 lines · Pavel Machek
Quoted 46 lines · Greg KH
Quoted 44 lines · Tejun Heo
Quoted 5 lines · Shem Multinymous
Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Quoted 7 lines

Agreed.
There's another issue with the current sysfs interface, though:
hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Yes, right. Forgot about the atomicity part altogether. Thanks for bringing it up.

Quoted 10 lines
Quoted 4 lines

Unloading heads will be simple. Just echoing timeout in ms to sysfs nodes, so I don't think it's a good idea to push out actual unloading to another process especially as fork doesn't inherit mlockall.

I had in mind another daemon listening for "unload now" events, so no forking needed.
This second daemon might make sense if we push the logic of deciding *which* disks to unload into userspace, since this logic is the same for the ThinkPad style and the HP style.

Hmmm... I can't (yet) see the benefit of having two separate userland daemons.

Quoted 9 lines
Quoted 3 lines

On a related note, is there any plan to merge tp_smapi to mainline? It seems you put a lot of work into it and I don't really see why it should stay out of tree.

The only issue I'm aware of is finding a reasonably-named maintainer. On the technical side, the reviews on my lkml submission of thinkpad_ec+hdaps seemed good and all technical comments are since addressed. The code has been stable, well-tested and packaged by major distros for years.

Cool, can you please post the patch to the lkml and cc Greg Kroah-Hartman <[email protected]>, Andrew Morton <[email protected]> and me?

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Sorry, but no, I can't accept this code as it is coming from a "known anonymous" person containing information that it is not known where it came from.

So... what are you worried about?

Code created by access to specs that were not allowed to be published in GPL form by someone who wants to remain anonymous.

That anonymous person may have problems if they signed NDA.

I don't think they did, they even list the sources:

* The embedded controller on ThinkPad laptops has a non-standard
interface, * where LPC channel 3 of the H8S EC chip is hooked up to IO ports * 0x1600-0x161F and implements (a special case of) the H8S LPC protocol. * The EC LPC interface provides various system management services (currently * known: battery information and accelerometer readouts). This driver * provides access and mutual exclusion for the EC interface.
*
* The LPC protocol and terminology is documented here:
* "H8S/2104B Group Hardware Manual",
*
http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf

Quoted 1 line

H8S chip seems to be documented.

Hmm, the EC is not directly used, but ACPI functions of the HP device are used.
For the HP ACPI device: the ACPI functions can *very easily* be re-engineered (which is common for all laptop_acpi.ko drivers):
ALRD -> is used by the driver to read out registers of the accelerometer ALWR -> is used by the driver to write a registers of the accelerometer
BTW: HP likes to have support for their device.

Quoted 6 lines

The acceleromter chip itself is docuemented in detail here:
http://www.st.com/stonline/products/literature/ds/12094/lis3lv02dl.pdf

I also do not see any concerns.
Greg: Can you please add this one or explain in more detail what else you like to see to get this integerated.

If you, or anyone else, writes a new driver from the published documents, that driver can be accepted. It can not be based on the existing code written by Shem in any form.

We can not accept this driver for reasons I've detailed numerous times in the past. That fact isn't going to change, sorry.

thanks,

greg k-h

#15 lore raw
Quoted 33 lines
Quoted 33 lines
Quoted 21 lines
Quoted 3 lines

Code created by access to specs that were not allowed to be published in GPL form by someone who wants to remain anonymous.

That anonymous person may have problems if they signed NDA.

I don't think they did, they even list the sources:

* The embedded controller on ThinkPad laptops has a non-standard
interface, * where LPC channel 3 of the H8S EC chip is hooked up to IO ports * 0x1600-0x161F and implements (a special case of) the H8S LPC protocol. * The EC LPC interface provides various system management services (currently * known: battery information and accelerometer readouts). This driver * provides access and mutual exclusion for the EC interface.
*
* The LPC protocol and terminology is documented here:
* "H8S/2104B Group Hardware Manual",
*
http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf

Quoted 1 line

H8S chip seems to be documented.

Hmm, the EC is not directly used, but ACPI functions of the HP device are used.
For the HP ACPI device: the ACPI functions can *very easily* be re-engineered (which is common for all laptop_acpi.ko drivers):
ALRD -> is used by the driver to read out registers of the accelerometer ALWR -> is used by the driver to write a registers of the accelerometer
BTW: HP likes to have support for their device.

The acceleromter chip itself is docuemented in detail here:
http://www.st.com/stonline/products/literature/ds/12094/lis3lv02dl.pdf

Quoted 7 lines
Quoted 3 lines

I also do not see any concerns.
Greg: Can you please add this one or explain in more detail what else you like to see to get this integerated.

If you, or anyone else, writes a new driver from the published documents, that driver can be accepted. It can not be based on the existing code written by Shem in any form.

Can you detail what "published" means?

Either I can take his sources on sourceforge.net (quite well known place, right) as published information, or I could not use other well known sources such as wikipedia.

Sources on sourceforge.net seem published-enough to me, and if you insist they can't be used, you should provide some reasons...

[And no, just calling it "tainted" is not enough.]
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#16 lore raw
Quoted 37 lines · Pavel Machek
Quoted 37 lines
Quoted 36 lines
Quoted 21 lines
Quoted 3 lines

Code created by access to specs that were not allowed to be published in GPL form by someone who wants to remain anonymous.

That anonymous person may have problems if they signed NDA.

I don't think they did, they even list the sources:

* The embedded controller on ThinkPad laptops has a non-standard
interface, * where LPC channel 3 of the H8S EC chip is hooked up to IO ports * 0x1600-0x161F and implements (a special case of) the H8S LPC protocol. * The EC LPC interface provides various system management services (currently * known: battery information and accelerometer readouts). This driver * provides access and mutual exclusion for the EC interface.
*
* The LPC protocol and terminology is documented here:
* "H8S/2104B Group Hardware Manual",
*
http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf

Quoted 1 line

H8S chip seems to be documented.

Hmm, the EC is not directly used, but ACPI functions of the HP device are used.
For the HP ACPI device: the ACPI functions can *very easily* be re-engineered (which is common for all laptop_acpi.ko drivers):
ALRD -> is used by the driver to read out registers of the accelerometer ALWR -> is used by the driver to write a registers of the accelerometer
BTW: HP likes to have support for their device.

The acceleromter chip itself is docuemented in detail here:
http://www.st.com/stonline/products/literature/ds/12094/lis3lv02dl.pdf

I also do not see any concerns.
Greg: Can you please add this one or explain in more detail what else you like to see to get this integerated.

Quoted 5 lines
Quoted 3 lines

If you, or anyone else, writes a new driver from the published documents, that driver can be accepted. It can not be based on the existing code written by Shem in any form.

Can you detail what "published" means?

Published in a way that has NOTHING to do with these source files.

Quoted 3 lines

Either I can take his sources on sourceforge.net (quite well known place, right) as published information, or I could not use other well known sources such as wikipedia.

If the wikipedia information was written based on these source files, no, we can't use that, sorry.

Quoted 4 lines

Sources on sourceforge.net seem published-enough to me, and if you insist they can't be used, you should provide some reasons...

[And no, just calling it "tainted" is not enough.]

{sigh}

Again, for the last time:
- this code was written by an anonymous person, using documents or information that was obtained and used in a manner that was not legal according to their employment agreement.
- because of this, we can not use this code, because we KNOW the information was obtained in a improper manner.
- so, to get something like this into the kernel, we need to rewrite the code, using information obtained LEGALLY from either the manufacturer of the chips or computers, or from another TOTALLY SEPARATE location.

Does that help explain this?

thanks,

greg k-h

#17 lore raw
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines

If you, or anyone else, writes a new driver from the published documents, that driver can be accepted. It can not be based on the existing code written by Shem in any form.

Can you detail what "published" means?

Quoted 8 lines

Published in a way that has NOTHING to do with these source files.

Quoted 3 lines

Either I can take his sources on sourceforge.net (quite well known place, right) as published information, or I could not use other well known sources such as wikipedia.

If the wikipedia information was written based on these source files, no, we can't use that, sorry.

How do I know?

Quoted 11 lines
Quoted 4 lines

Sources on sourceforge.net seem published-enough to me, and if you insist they can't be used, you should provide some reasons...

[And no, just calling it "tainted" is not enough.]

{sigh}

Again, for the last time:
- this code was written by an anonymous person, using documents or information that was obtained and used in a manner that was not legal according to their employment agreement.

This code is written by anonymous person. He may have used documents improperly, but I see no signs of that, and don't see why I should believe you saying so.

If have proof of that, you should talk to sourceforge to take that code down... or probably their employer should ask sourceforge to do that.

The documents are on the web from more than year now, on well-known. That seems to indicate that your theory is not true.

Quoted 2 lines

- because of this, we can not use this code, because we KNOW the information was obtained in a improper manner.

Whole wikipedia is written by "anonymous" people. Does that mean that all information in wikipedia was obtained in a improper manner?

Quoted 4 lines

- so, to get something like this into the kernel, we need to rewrite the code, using information obtained LEGALLY from either the manufacturer of the chips or computers, or from another TOTALLY SEPARATE location.

I'm LEGALLY obtaining the information from sourceforge.net. That is rather well-known, and non-anonymous source. They continue to publish this information.

Quoted 1 line

Does that help explain this?

Unfortunately, no :-(.
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#18 lore raw
Quoted 9 lines · Pavel Machek
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines

If you, or anyone else, writes a new driver from the published documents, that driver can be accepted. It can not be based on the existing code written by Shem in any form.

Can you detail what "published" means?

Published in a way that has NOTHING to do with these source files.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

Either I can take his sources on sourceforge.net (quite well known place, right) as published information, or I could not use other well known sources such as wikipedia.

If the wikipedia information was written based on these source files, no, we can't use that, sorry.

How do I know?

Unfortunatly, you can't, so don't use it.

Quoted 16 lines
Quoted 11 lines
Quoted 4 lines

Sources on sourceforge.net seem published-enough to me, and if you insist they can't be used, you should provide some reasons...

[And no, just calling it "tainted" is not enough.]

{sigh}

Again, for the last time:
- this code was written by an anonymous person, using documents or information that was obtained and used in a manner that was not legal according to their employment agreement.

This code is written by anonymous person. He may have used documents improperly, but I see no signs of that, and don't see why I should believe you saying so.

Quoted 6 lines

If have proof of that, you should talk to sourceforge to take that code down... or probably their employer should ask sourceforge to do that.

The documents are on the web from more than year now, on well-known. That seems to indicate that your theory is not true.

Yes, I know this is true, for a variety of reasons of what people (including the individual in question) told me in private.

And I'm not going to go around asking for code to be taken down, as I'm not the one whose contract was invalidated.

All I can say is that I can not accept such code into the Linux kernel as it is known to be created in an illegal manner.

This has been discussed with both the Linux Foundations lawyers (actually it was OSDL at the time), and with Novell's lawyers.

And as a Novell employee, you aren't allowed to put this code into the kernel either, sorry, that's the rule we were told.

Quoted 8 lines
Quoted 4 lines

- so, to get something like this into the kernel, we need to rewrite the code, using information obtained LEGALLY from either the manufacturer of the chips or computers, or from another TOTALLY SEPARATE location.

I'm LEGALLY obtaining the information from sourceforge.net. That is rather well-known, and non-anonymous source. They continue to publish this information.

If the information there is known to be posted in a manner that was obtained illegally, it does not make the fact that you take it any more "legal".

Ok, once again:
- we know this code was created illegally.
- that is why we (Linux Foundation / Novell) can not accept such code.

If some other company wants to step up, and put their "Signed-off-by:" on it, I will be glad to have the Linux Foundation lawyers contact them to find out if this is now acceptable, and from what background they have gotten the information.

So can we please just drop this? Or do you want me to point you at the lawyers?

thanks,

greg k-h

#19 lore raw

Hi!

Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

Either I can take his sources on sourceforge.net (quite well known place, right) as published information, or I could not use other well known sources such as wikipedia.

If the wikipedia information was written based on these source files, no, we can't use that, sorry.

How do I know?

Unfortunatly, you can't, so don't use it.

Not being able to use wikipedia is little strong, don't you think so? What's next?

Quoted 8 lines
Quoted 8 lines

This code is written by anonymous person. He may have used documents improperly, but I see no signs of that, and don't see why I should believe you saying so.

If have proof of that, you should talk to sourceforge to take that code down... or probably their employer should ask sourceforge to do that.

Quoted 5 lines
Quoted 2 lines

The documents are on the web from more than year now, on well-known. That seems to indicate that your theory is not true.

Yes, I know this is true, for a variety of reasons of what people (including the individual in question) told me in private.

Are you saying Shem told you he violated some contracts?

Ok, so can you tell us the details, so that we can evaluate this ourselves, and (more importantly) so that he can defend himself? He seems willing to point where specific pieces of code come from.

Quoted 2 lines

And I'm not going to go around asking for code to be taken down, as I'm not the one whose contract was invalidated.

Right. So why do you care if this code goes into kernel? Your contract is not violated, and neither me nor you signed any contract preventing us from putting that code into kernel.

If you know the details, can you tell us which/whose contract is being violated here? Maybe we can just get permission of those parties. Or maybe your information is wrong...

Quoted 2 lines

Ok, once again:
- we know this code was created illegally.

If you know this, please share details.

Quoted 2 lines

So can we please just drop this? Or do you want me to point you at the lawyers?

I'll talk to lawyers if I have to. I'd like to understand this.
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#20 lore raw
In reply to Greg KH

I've been staying off the public discussion to avoid fuelling the flame; my position has been stated earlier and in private communication. But at this point I must go on record:

Quoted 3 lines · Greg KH

- this code was written by an anonymous person, using documents or information that was obtained and used in a manner that was not legal according to their employment agreement.

The above statement is false. I have never had access to non-public ThinkPad specifications as part of my employment, nor have I been employed by anyone who (to my knowledge) even possesses non-public ThinkPad specifications.

I have contacted Greg KH privately in an effort to clarify the source of his false impression.

Shem

#21 lore raw
In reply to Shem Multinymous
Quoted 22 lines · Shem Multinymous

Hi Tejun,

Quoted 12 lines · Tejun Heo

Hello, Shem Multinymous.

Quoted 10 lines

Using the input device interface for the accelerometer (as done by tp_smapi's hdaps + latest hdapsd) greatly reduces the number of accelerometer-related timer interrupts on tickless kernels, as measured by powertop. With syscall polling you have the kernal polling the hardware at ~50Hz and then the userspace hdapsd polling the kernel at ~50Hz. When they're out of phase so you can get up to 100 interrupts/sec. With an input device you're always at 50Hz. The phase difference also induces a small extra delay in the shock handling response.

Quoted 5 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

Quoted 7 lines

Agreed.
There's another issue with the current sysfs interface, though: hdapsd needs to read (x,y,timestamp) tuples, whereas sysfs provides just x and y in separate attributes which cannot be read atomically together. We can add a sysfs file with "x y timestamp" readouts, though this is unusual for sysfs (and certainly incompatible with hwmon).

Assuming timestamp is always updated when the x,y values change, you can do:

	do {
		ts = read_timestamp();
		x = read_x();
		y = read_y();
		ts2 = read_timestamp();
	} while(ts != ts2);






    J
#22 lore raw
In reply to Tejun Heo
Quoted 3 lines · Tejun Heo

Hello, Shem Multinymous.

Shem Multinymous wrote:

[...]

Quoted 21 lines
Quoted 9 lines
Quoted 5 lines

2. If we're gonna unify interface, how much can we unify the backend? Some devices are based on polling, others interrupt. For polling, is it better to delegate the whole polling to userland or is it better to do some of it in kernel (tp_smapi seems to be doing this)?

The ThinkPad accelerometer needs to be polled at very regular intervals (max jitter on the order of 10ms), which sounds like a job for the kernel.

Yes, I agree.

Quoted 8 lines

This is because in ThinkPads we actually have a 4-level pile:
[hdapsd userspace] -> [hdaps kernel] -> [embedded controller] -> [accelerometer A2D]
What the kernel polls is actually is the H8S embedded controller (EC) chip, which in turn does its own polling of the accelerometer A2D. Now, the EC has a tiny buffer and strange buffering semantics, and it has its own internal clock, so the software->EC polling should be very regular to minmize EC buffer overflows/underruns.

Quoted 4 lines

So, I think the whole polling should be implemented inside the kernel and the kernel should notify userland when new data is available, which is about what the current joystick implementation does and can be achieved using sysfs_notify_event().

Short of a satisfying proposition regarding the questions raised, I just want to add two things that would be nice to solve in the future one way or another and should perhaps be taken into consideration from the beginning:

1. Disable polling completely when it isn't required: once the hd has spun down, there is no need to keep polling the sensors at all. Only when the first request requiring the hd to spin up arrives, the kernel needs to hold back for a short while to gather enough data from the sensors, so shock protection is up and running again.
2. Make shock protection interact nicely with suspend operations:
currently, we are out of luck if anything should happen after processes have been frozen. This is particularly unfortunatel in the case of s2disk.

As far as 1. is concerned, I'm not quite sure yet how to determine when the disk has spun down since this may have happened according to vendor specific timing rules. Once we have established that the disk is sleeping safe and sound, sensor drivers can be notified to stop polling the hardware and possibly notify clients (like hdapsd) about that fact.

Something along the lines of the disk_shock module approach suggested by Bart in [1] sounds promising, but it still needs some careful thinking when designing the interfaces to all components involved. As for the suspend problem, we could either put some simplified logic into disk_shock so it can take over once hdapsd has been frozen, or we can find a way to keep hdapsd unfrozen until the hd has been put to sleep. Of course, we'd have to think about resume as well, although it'll be impossible to protect right from the start in the s2disk case.

Regards,

Elias

[1] http://marc.info/?l=linux-kernel&m=122021163324843&w=2

#23 lore raw

Hi!

Quoted 10 lines

Short of a satisfying proposition regarding the questions raised, I just want to add two things that would be nice to solve in the future one way or another and should perhaps be taken into consideration from the beginning:

1. Disable polling completely when it isn't required: once the hd has spun down, there is no need to keep polling the sensors at all. Only when the first request requiring the hd to spin up arrives, the kernel needs to hold back for a short while to gather enough data from the sensors, so shock protection is up and running again.

hdparm can already tell if disk is spinning or not. As userland is polling, already, that may be enough?

Quoted 4 lines

2. Make shock protection interact nicely with suspend operations:
currently, we are out of luck if anything should happen after processes have been frozen. This is particularly unfortunatel in the case of s2disk.

I'd say that s2disk is similar to early boot... no protection there.

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#24 lore raw
Quoted 7 lines · Pavel Machek

Hi!

Quoted 5 lines

Short of a satisfying proposition regarding the questions raised, I just want to add two things that would be nice to solve in the future one way or another and should perhaps be taken into consideration from the beginning:

Quoted 8 lines
Quoted 5 lines

1. Disable polling completely when it isn't required: once the hd has spun down, there is no need to keep polling the sensors at all. Only when the first request requiring the hd to spin up arrives, the kernel needs to hold back for a short while to gather enough data from the sensors, so shock protection is up and running again.

hdparm can already tell if disk is spinning or not. As userland is polling, already, that may be enough?

Yes, I know it *is* possible, I just think it may not be completely trivial to get it right. The only way I can think of, right now, is some sort of polling in the hdparm way, i.e. issuing a ATA_CMD_CHK_POWER from time to time. The question is how we are to decide when and how often we should poll for the current power state of the HD.

Quoted 7 lines
Quoted 5 lines

2. Make shock protection interact nicely with suspend operations:
currently, we are out of luck if anything should happen after processes have been frozen. This is particularly unfortunatel in the case of s2disk.

I'd say that s2disk is similar to early boot... no protection there.

Well, this is true for resume. However, I can't help thinking that we may do better than that until the disk spins down, particularly while writing the image to disk.

Regards,

Elias

#25 lore raw
Quoted 10 lines · Elias Oltmanns
Quoted 6 lines
Quoted 4 lines

2. Make shock protection interact nicely with suspend operations:
currently, we are out of luck if anything should happen after processes have been frozen. This is particularly unfortunatel in the case of s2disk.

I'd say that s2disk is similar to early boot... no protection there.

Well, this is true for resume. However, I can't help thinking that we may do better than that until the disk spins down, particularly while writing the image to disk.

Agreed.
Practically, disk protection during suspend-to-disk sounds very useful, given the typical usage scenario: you press button to hibernate and start shuffling around your chair/desk in preparation for leaving the room. Dropping the laptop is more likely than ever at this point. Resume-from-disk tends to be done after settling down into your chair/desk, so the drop risk is lower.

Shem

#26 lore raw
In reply to Tejun Heo
Quoted 10 lines · Tejun Heo
Quoted 9 lines

Using the input device interface for the accelerometer (as done by tp_smapi's hdaps + latest hdapsd) greatly reduces the number of accelerometer-related timer interrupts on tickless kernels, as measured by powertop. With syscall polling you have the kernal polling the hardware at ~50Hz and then the userspace hdapsd polling the kernel at ~50Hz. When they're out of phase so you can get up to 100 interrupts/sec. With an input device you're always at 50Hz. The phase difference also induces a small extra delay in the shock handling response.

Quoted 4 lines

That reduction comes because input device supports poll and sysfs_notify_event() does about the same thing. The uesrland daemon can just poll on a node and read data nodes when poll event on the node triggeres.

If you guys are going to bother with the accelerometer interface (which is a completely separate issue from the "queue-freeze-and-park-heads" APIs and sysfs interface), you better be aware that there IS an accelerometer class in the works, that cathers for directly-attached devices.

I'd look there first. And a generic sysfs interface for accelerometers IS a reasonably hard problem, so I would have it well separate from the disk-park side of things and while it gets sorted out, I'd leave it for userspace to deal with the issue (it is not like there is much userspace to worry about right now, just hdapsd which is only interested on the hdaps accel interface. Everyone else can do it in-kernel, because their firmware already does the imminent-fall detection).

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
#27 lore raw
In reply to Tejun Heo

Hi!

Quoted 21 lines
Quoted 9 lines
Quoted 5 lines

2. If we're gonna unify interface, how much can we unify the backend? Some devices are based on polling, others interrupt. For polling, is it better to delegate the whole polling to userland or is it better to do some of it in kernel (tp_smapi seems to be doing this)?

The ThinkPad accelerometer needs to be polled at very regular intervals (max jitter on the order of 10ms), which sounds like a job for the kernel.

Yes, I agree.

Quoted 8 lines

This is because in ThinkPads we actually have a 4-level pile:
[hdapsd userspace] -> [hdaps kernel] -> [embedded controller] -> [accelerometer A2D]
What the kernel polls is actually is the H8S embedded controller (EC) chip, which in turn does its own polling of the accelerometer A2D. Now, the EC has a tiny buffer and strange buffering semantics, and it has its own internal clock, so the software->EC polling should be very regular to minmize EC buffer overflows/underruns.

Quoted 4 lines

So, I think the whole polling should be implemented inside the kernel and the kernel should notify userland when new data is available, which is about what the current joystick implementation does and can be achieved using sysfs_notify_event().

I like joystick/input interface slightly better. In some cases, machines with accelerometers (openmoko) use them for input primarily.

HP interface will be more specialized (but less useful); still userland daemon can handle the differences...

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#28 lore raw
In reply to Tejun Heo

Tejun Heo escreveu:

Quoted 3 lines

I browsed a little bit for HDAPS one and it seems all the pieces are there but scattered. The latest effort seems tp_smapi which Shem Multinymous is working on.

I have a Lenovo Thinkpad T61 running tp_smapi and hdapsd over 2.6.26.2 Kernel with Elias patch:
<http://article.gmane.org/gmane.linux.drivers.hdaps.devel/1324/raw>

If you need some info, pls let me know.

Quoted 7 lines

1. How should the shock interface look like? As we're gonna need userland daemon one way or the other, we can use the userland daemon to glue all the interfaces but it would be much better to have a unified interface. Although there seem to be several different variants, they don't differ all that much and creating a new interface every time is painful. I think we can get by with a sysfs interface with notification.

IMHO, userland daemon need only inform users when hdd heads was parking. This is what KHDAPSMonitor do (<http://roy.marples.name/node/269>).
A daemon like this is used in Windows, but one more feature is present:
Is possible choose "Manual Unlock" (hdd heads is kept parked until I click over daemon to unlock).

Best regards,
Renato S. Yamane

#29 lore raw
In reply to Tejun Heo
Quoted 5 lines · Tejun Heo

2. If we're gonna unify interface, how much can we unify the backend? Some devices are based on polling, others interrupt. For polling, is it better to delegate the whole polling to userland or is it better to do some of it in kernel (tp_smapi seems to be doing this)?

Shock protection should be time-sensitive, if we put the whole polling into userland, will it be possible that the damage had happened before userland app can signal ATA idle command timely?

Quoted 6 lines

3. What about the userland daemon? It would be best to have a unified daemon which can handle all instead of one for hdaps and another for hp (and so on). If we can unify the interface, this will be much easier.

Thanks.

Can this process "acceleration-detect --> inform ATA shock protect --> issue idle command" be done totally in kernel, avoiding to consume too many time for "acceleration-detect --> sysfs --> userland app --> sysfs --> inform ATA shock protect --> issue idle command" before HD was damaged? The userland daemon should be just a indicator (but of course it can pass params to driver) for the protection status rather than a judge.

author
#30 lore raw
Quoted 8 lines · Austin Zhang
Quoted 5 lines

2. If we're gonna unify interface, how much can we unify the backend? Some devices are based on polling, others interrupt. For polling, is it better to delegate the whole polling to userland or is it better to do some of it in kernel (tp_smapi seems to be doing this)?

Shock protection should be time-sensitive, if we put the whole polling into userland, will it be possible that the damage had happened before userland app can signal ATA idle command timely?

Yeah, it's time sensitive but it seems latency of tens of millisecs is good enough and with mlocked user process, it's really not a problem.

Quoted 5 lines
Quoted 5 lines

3. What about the userland daemon? It would be best to have a unified daemon which can handle all instead of one for hdaps and another for hp (and so on). If we can unify the interface, this will be much easier.

Quoted 8 lines
Quoted 1 line

Thanks.

Can this process "acceleration-detect --> inform ATA shock protect --> issue idle command" be done totally in kernel, avoiding to consume too many time for "acceleration-detect --> sysfs --> userland app --> sysfs --> inform ATA shock protect --> issue idle command" before HD was damaged? The userland daemon should be just a indicator (but of course it can pass params to driver) for the protection status rather than a judge.

Again, it doesn't have to be that fast and the judgement part involves complex floating arithmetics + user usage patterns (has the user typed something recently, is lid closed kind of stuff). I don't think it fits in kernel.

Thanks.

Signature · 1 line
tejun