Hello dear kernel-people,
I have a little question, which i hope is right to post here and does
not cause inconveniences.
Well, since about 6 weeks i own a Lenovo Thinkpad X60s which i bought
primarily because thinkpads are rumored to be very well supported by
linux. Sencondly because as a student i got some rebate on thinkpads ;)
Well, the actual question is the following,
I read about HDAPS on thinkWiki. But there is no known-to-work patch for
2.6.18 and above to enable queue-freezing/harddisk parking.
After some googeling and digging in gamne i read that someone said that
there are plans for some generic support for HD-parking in the kernel
and thus making such patches obsolete.
My quesiotn just is if this is true and if there are any chances that
the kernel will support that soonly.
The point is i have to trave quite some distance to my University (about
one and half an hour)
And thus doing some of my work in the train or bus. But well... they
often shake and wobble, hit the brakes suddenly and some of that stuff
which makes me nearly drop my notebook often.
Thats the most point why i would be very pleased to know that my hd
doesn't suffer a headcrash in such a circumstance.
As there are quite some notebooks out there which support this nowadays
(i know of some IBM/lenovo and HP ones), a generic support for that
would be nice and make users of linux on notebooks feel much more
comfortable.
So i hope this issue can be adressed soon. but i also know that most of
you are very busy and i can not evaluate how difficult such a change
would be. However if anyone wants to test some things or more
information, i am ready. Just CC me :)
thanks,
Christoph
Hi!
> Well, the actual question is the following,
> I read about HDAPS on thinkWiki. But there is no known-to-work patch for
> 2.6.18 and above to enable queue-freezing/harddisk parking.
> After some googeling and digging in gamne i read that someone said that
> there are plans for some generic support for HD-parking in the kernel
> and thus making such patches obsolete.
> My quesiotn just is if this is true and if there are any chances that
> the kernel will support that soonly.
...
> So i hope this issue can be adressed soon. but i also know that most of
> you are very busy and i can not evaluate how difficult such a change
> would be. However if anyone wants to test some things or more
> information, i am ready. Just CC me :)
I'm afraid we need your help with development here. Porting old patch
to 2.6.19-rc6 should be easy, and then you can start 'how do I
makethis generic' debate.
Does hdaps work for you, btw? It gave all zeros on my x60, iirc.
--
Thanks for all the (sleeping) penguins.
On 11/21/06, Pavel Machek <[email protected]> wrote:
> I'm afraid we need your help with development here. Porting old patch
> to 2.6.19-rc6 should be easy
http://lkml.org/lkml/2006/10/9/84
http://lkml.org/lkml/2006/10/10/275
> Does hdaps work for you, btw? It gave all zeros on my x60, iirc.
Yes, vanilla hdaps is broken. It blindly issues commands to the
embedded controller without following the protocol or checking the
status. The patched version in the tp_smapi package fixes it.
Shem
On Tue, Nov 21 2006, Pavel Machek wrote:
> Hi!
>
> > Well, the actual question is the following,
> > I read about HDAPS on thinkWiki. But there is no known-to-work patch for
> > 2.6.18 and above to enable queue-freezing/harddisk parking.
> > After some googeling and digging in gamne i read that someone said that
> > there are plans for some generic support for HD-parking in the kernel
> > and thus making such patches obsolete.
> > My quesiotn just is if this is true and if there are any chances that
> > the kernel will support that soonly.
> ...
> > So i hope this issue can be adressed soon. but i also know that most of
> > you are very busy and i can not evaluate how difficult such a change
> > would be. However if anyone wants to test some things or more
> > information, i am ready. Just CC me :)
>
> I'm afraid we need your help with development here. Porting old patch
> to 2.6.19-rc6 should be easy, and then you can start 'how do I
> makethis generic' debate.
2.6.19 will finally have the generic block layer commands, so this can
be implemented properly.
--
Jens Axboe
Jens Axboe <jens.axboe <at> oracle.com> writes:
>
> On Tue, Nov 21 2006, Pavel Machek wrote:
> > Hi!
> >
> > > Well, the actual question is the following,
> > > I read about HDAPS on thinkWiki. But there is no known-to-work patch for
> > > 2.6.18 and above to enable queue-freezing/harddisk parking.
> > > After some googeling and digging in gamne i read that someone said that
> > > there are plans for some generic support for HD-parking in the kernel
> > > and thus making such patches obsolete.
> > > My quesiotn just is if this is true and if there are any chances that
> > > the kernel will support that soonly.
> > ...
> > > So i hope this issue can be adressed soon. but i also know that most of
> > > you are very busy and i can not evaluate how difficult such a change
> > > would be. However if anyone wants to test some things or more
> > > information, i am ready. Just CC me :)
> >
> > I'm afraid we need your help with development here. Porting old patch
> > to 2.6.19-rc6 should be easy, and then you can start 'how do I
> > makethis generic' debate.
>
> 2.6.19 will finally have the generic block layer commands, so this can
> be implemented properly.
>
That's good to know. Sounds like we'll be able to have another attempt at
getting this functionality upstream..
In the meantime, the current code has been cleaned up and updated to work with
2.6.18. Patches are on the hdaps-devel list.
http://sourceforge.net/mailarchive/forum.php?forum=hdaps-devel (or gmane for an
easier view ;)
Regards,
Jon.
Hi!
> >Does hdaps work for you, btw? It gave all zeros on my x60, iirc.
>
> Yes, vanilla hdaps is broken. It blindly issues commands to the
> embedded controller without following the protocol or checking the
> status. The patched version in the tp_smapi package fixes it.
Is there a way to extract minimal patch? ...the kind that is trivial
enough so that akpm does accepts it...?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> >> > After some googeling and digging in gamne i read that someone said that
> >> > there are plans for some generic support for HD-parking in the kernel
> >> > and thus making such patches obsolete.
> [...]
> >> I'm afraid we need your help with development here. Porting old patch
> >> to 2.6.19-rc6 should be easy, and then you can start 'how do I
> >> makethis generic' debate.
> >
> > 2.6.19 will finally have the generic block layer commands, so this can
> > be implemented properly.
>
> Eventually, I've ported the patch to 2.6.19-rc6 (attached). However,
> I'll need some gentle guidance by you developers for the next steps,
> I'm afraid. What exactly do you mean by "making this generic".
> Perhaps, you could give me a hint as to which generic block layer
> commands you have in mind that should be used in such a patch.
>
>
> Here is a short description of what the patch does in its current
> shape:
>
> 1. Adds functions to ide-disk.c and scsi_lib.c that issue an idle
> immediate with head unload or a standby immediate command as
> appropriate and stop the queue on command completion.
Can we get short Documentation/ patch?
> + if (!pending)
> + q->issue_unprotect_fn(q);
Minor tab vs spaces problem here.
> + spin_unlock_irq(q->queue_lock);
> + return queue_var_show(seconds, (page));
And here.
> +static ssize_t queue_protect_store(struct request_queue *q, const char *page, size_t count)
> +{
80 colums would be nice.
> + if(freeze>0) {
...and space between if and (
> +static struct queue_sysfs_entry queue_protect_entry = {
> + .attr = {.name = "protect", .mode = S_IRUGO | S_IWUSR },
And space between { and . .
> + /* create the attribute */
> + error = sysfs_create_file(&q->kobj, &queue_protect_entry.attr);
> + if(error){
if (error) {
> +module_param_named(protect_method, libata_protect_method, int, 0444);
> +MODULE_PARM_DESC(protect_method, "hdaps disk protection method (0=autodetect, 1=unload, 2=standby)");
Should this be configurable by module parameter? Why not tell each
unload what to do?
Is /sys interface right thing to do?
> + if (libata_protect_method == 1) {
> + unload = 1;
> + printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): unload method requested, overriding drive capability check..\n");
> + }
} and else on same line...
> + else if (libata_protect_method == 2) {
> + unload = 0;
> + printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): standby method requested, overriding drive capability check..\n");
> + }
> + else if (ata_id_has_unload(dev->id)) {
> + unload = 1;
> + printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): unload support reported by drive..\n");
> + }
> + else {
> + unload = 0;
> + printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): unload support NOT reported by drive!..\n");
> + }
Can we consolidate the strings somehow?
> --- a/drivers/ide/ide-disk.c
> +++ b/drivers/ide/ide-disk.c
> @@ -72,6 +72,10 @@ #include <asm/uaccess.h>
> #include <asm/io.h>
> #include <asm/div64.h>
>
> +int idedisk_protect_method = 0;
> +module_param_named(protect_method, idedisk_protect_method, int, 0444);
> +MODULE_PARM_DESC(protect_method, "hdaps disk protection method (0=autodetect, 1=unload, 2=standby)");
> +
Oh and do not mention hdaps, there are more different accelerometer
types.
> + /*
> + * Auto-unfreeze state
> + */
> + struct timer_list unfreeze_timer;
> + int max_unfreeze; /* At most this many seconds */
> + struct work_struct unfreeze_work;
> +
> struct backing_dev_info backing_dev_info;
>
Should we have kernel doing auto-unfreeze? Perhaps we can just mlock()
the daemon?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 11/30/06, Pavel Machek <[email protected]> wrote:
> > >Does hdaps work for you, btw? It gave all zeros on my x60, iirc.
> >
> > Yes, vanilla hdaps is broken. It blindly issues commands to the
> > embedded controller without following the protocol or checking the
> > status. The patched version in the tp_smapi package fixes it.
>
> Is there a way to extract minimal patch? ...the kind that is trivial
> enough so that akpm does accepts it...?
I can't think of any such trivial fix. My submitted code includes a
whole new driver, thinkpad_ec, just to get the (fully documented!) EC
protocl right. You could strip a few code paths which hdaps doesn't
invoke, but it's hard to see how you can get away with much less
except by making unwarranted assumptions about the hardware and its
timing characteristics.
Shem
On 11/30/06, Pavel Machek <[email protected]> wrote:
> Should we have kernel doing auto-unfreeze? Perhaps we can just mlock()
> the daemon?
You could be in the middle of suspend with by-now-frozen userspace; or
maybe the daemon had a SEGV or was accidentally killed. Can't trust
that.
Shem
On Mon, Nov 27 2006, Elias Oltmanns wrote:
> Jens Axboe <[email protected]> wrote:
> > On Tue, Nov 21 2006, Pavel Machek wrote:
> >> Hi!
> >>
> [...]
> >> > After some googeling and digging in gamne i read that someone said that
> >> > there are plans for some generic support for HD-parking in the kernel
> >> > and thus making such patches obsolete.
> [...]
> >> I'm afraid we need your help with development here. Porting old patch
> >> to 2.6.19-rc6 should be easy, and then you can start 'how do I
> >> makethis generic' debate.
> >
> > 2.6.19 will finally have the generic block layer commands, so this can
> > be implemented properly.
>
> Eventually, I've ported the patch to 2.6.19-rc6 (attached). However,
> I'll need some gentle guidance by you developers for the next steps,
> I'm afraid. What exactly do you mean by "making this generic".
> Perhaps, you could give me a hint as to which generic block layer
> commands you have in mind that should be used in such a patch.
If you look in <linux/blkdev.h>, you can see the definition of
REQ_TYPE_LINUX_BLOCK and the current sub commands (none are implemented
right now). So you want to add REQ_LB_OPT_PARK (or whatever the name
should be) to implement the freezing operation, and then add support for
the drivers to understand this command and do what they need to do. You
can add block layer helpers to perform the freezing of the actual queue,
to be called when the PARK command completes. You can handle the queue
draining for tagged devices like the barriers do for FUA barriers.
--
Jens Axboe
Hi Pavel,
thanks a lot for your first review. See comments below.
Pavel Machek <[email protected]> wrote:
> Hi!
>
[...]
>> Here is a short description of what the patch does in its current
>> shape:
>>
>> 1. Adds functions to ide-disk.c and scsi_lib.c that issue an idle
>> immediate with head unload or a standby immediate command as
>> appropriate and stop the queue on command completion.
>
> Can we get short Documentation/ patch?
Sure. Would Documentation/block/disk-protection.txt be an appropriate
location?
[...]
>> +module_param_named(protect_method, libata_protect_method, int, 0444);
>> +MODULE_PARM_DESC(protect_method, "hdaps disk protection method (0=autodetect, 1=unload, 2=standby)");
>
> Should this be configurable by module parameter? Why not tell each
> unload what to do?
As I understand, ATA specs expect drives to indicate whether they
support the head unload feature of the idle immediate command or not.
Unfortunately, a whole lot of them doesn't, well, mine doesn't anyway.
Since I know that my drive does actually support head unloading, I'd
like to tell the module so in order to prevent it from falling back to
standby immediate. Applications that issue disk parking requests
should not be bothered with this issue, in my opinion.
>
> Is /sys interface right thing to do?
Probably, you're right here. Since this feature is actually drive
specific, it should not really be set globally as a libata or ide-disk
parameter but specifically for each drive connected. Perhaps we should
add another attribute to /sys/block/*/queue or enhance the scope of
/sys/block/*/queue/protect?
[...]
>> + else if (libata_protect_method == 2) {
>> + unload = 0;
>> + printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): standby method requested, overriding drive capability check..\n");
>> + }
>> + else if (ata_id_has_unload(dev->id)) {
>> + unload = 1;
>> + printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): unload support reported by drive..\n");
>> + }
>> + else {
>> + unload = 0;
>> + printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): unload support NOT reported by drive!..\n");
>> + }
>
> Can we consolidate the strings somehow?
Actually, I'd like to move this to the initialisation sequence of the
drive. I still have to figure out how to do this properly.
[...]
>> + /*
>> + * Auto-unfreeze state
>> + */
>> + struct timer_list unfreeze_timer;
>> + int max_unfreeze; /* At most this many seconds */
>> + struct work_struct unfreeze_work;
>> +
>> struct backing_dev_info backing_dev_info;
>>
>
> Should we have kernel doing auto-unfreeze? Perhaps we can just mlock()
> the daemon?
> Pavel
I'd strongly second Shem's comments on this. Besides, anybody with
root privileges can issue diks park requests, not just hdapsd. Please
note that this is not a hard timeout as userspace can always refresh
the timer before it has actually expired. On the other hand I would
not want to rely on user space to unfreeze my drives.
Regards
Elias
Hi!
> >> 1. Adds functions to ide-disk.c and scsi_lib.c that issue an idle
> >> immediate with head unload or a standby immediate command as
> >> appropriate and stop the queue on command completion.
> >
> > Can we get short Documentation/ patch?
>
> Sure. Would Documentation/block/disk-protection.txt be an appropriate
> location?
Yes.
> >> +module_param_named(protect_method, libata_protect_method, int, 0444);
> >> +MODULE_PARM_DESC(protect_method, "hdaps disk protection method (0=autodetect, 1=unload, 2=standby)");
> >
> > Should this be configurable by module parameter? Why not tell each
> > unload what to do?
>
> As I understand, ATA specs expect drives to indicate whether they
> support the head unload feature of the idle immediate command or not.
> Unfortunately, a whole lot of them doesn't, well, mine doesn't anyway.
> Since I know that my drive does actually support head unloading, I'd
> like to tell the module so in order to prevent it from falling back to
> standby immediate. Applications that issue disk parking requests
> should not be bothered with this issue, in my opinion.
What if you have two disks and one supports head unload and second
does not?
> > Is /sys interface right thing to do?
>
> Probably, you're right here. Since this feature is actually drive
> specific, it should not really be set globally as a libata or ide-disk
> parameter but specifically for each drive connected. Perhaps we should
> add another attribute to /sys/block/*/queue or enhance the scope of
> /sys/block/*/queue/protect?
Certainly better than current solution. Or maybe ioctl similar to wat
hdparm uses?
Pavel
--
Thanks for all the (sleeping) penguins.
Hi Jens,
Elias Oltmanns <[email protected]> wrote:
> So, here is a patch in which your remarks and suggestions have been
> incorporated. Additionally, I've added the requested kernel doc file
> and another sysfs attribute called protect_method. The usage of this
> attribute is described in Documentation/block/disk-protection.txt.
Just forgot to mention that your suggestions haven't been implemented
yet. Thats because I'm only gradually beginning to understand the
reasoning and how it might work out in the end. It will probably take
me another weekend (or more) to come up with something fit for
discussion. Bare with me, I'm rather busy at the moment and still new
to the block layer (or kernel code, for that matter).
Regards,
Elias
On Sun, Dec 10 2006, Elias Oltmanns wrote:
> Hi Jens,
>
> Elias Oltmanns <[email protected]> wrote:
> > So, here is a patch in which your remarks and suggestions have been
> > incorporated. Additionally, I've added the requested kernel doc file
> > and another sysfs attribute called protect_method. The usage of this
> > attribute is described in Documentation/block/disk-protection.txt.
>
> Just forgot to mention that your suggestions haven't been implemented
> yet. Thats because I'm only gradually beginning to understand the
> reasoning and how it might work out in the end. It will probably take
> me another weekend (or more) to come up with something fit for
> discussion. Bare with me, I'm rather busy at the moment and still new
> to the block layer (or kernel code, for that matter).
No worries, take your time. The target for the code wont be 2.6.20
anyways, so you have at least a month to get things designed right and
solid for a 2.6.21 target.
--
Jens Axboe
Hi1
> >> >> +module_param_named(protect_method, libata_protect_method, int, 0444);
> >> >> +MODULE_PARM_DESC(protect_method, "hdaps disk protection method (0=autodetect, 1=unload, 2=standby)");
> >> >
> >> > Should this be configurable by module parameter? Why not tell each
> >> > unload what to do?
> [...]
> >> > Is /sys interface right thing to do?
> >>
> >> Probably, you're right here. Since this feature is actually drive
> >> specific, it should not really be set globally as a libata or ide-disk
> >> parameter but specifically for each drive connected. Perhaps we should
> >> add another attribute to /sys/block/*/queue or enhance the scope of
> >> /sys/block/*/queue/protect?
> >
> > Certainly better than current solution. Or maybe ioctl similar to wat
> > hdparm uses?
>
> I'm not quite sure what you have in mind wrt ioctls. I'm still
> convinced that the administrator should take a conscious decision when
> forcing an idle immediate with unload feature on a drive which doesn't
> announce this capability according to the specs. This is because I
> have no idea as to how drives might react if they don't support it.
> Perhaps we should consult linux-ide on this topic.
Yep, I guess linux-ide would have some comments.
> So, here is a patch in which your remarks and suggestions have been
> incorporated. Additionally, I've added the requested kernel doc file
Additional suggestion is to keep lines < 80 columns.... Sorry it took
me so long to comment.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
Pavel Machek <[email protected]> wrote:
> Hi1
>
>> >> >> +module_param_named(protect_method, libata_protect_method, int, 0444);
>> >> >> +MODULE_PARM_DESC(protect_method, "hdaps disk protection method (0=autodetect, 1=unload, 2=standby)");
>> >> >
>> >> > Should this be configurable by module parameter? Why not tell each
>> >> > unload what to do?
>> [...]
>> >> > Is /sys interface right thing to do?
>> >>
>> >> Probably, you're right here. Since this feature is actually drive
>> >> specific, it should not really be set globally as a libata or ide-disk
>> >> parameter but specifically for each drive connected. Perhaps we should
>> >> add another attribute to /sys/block/*/queue or enhance the scope of
>> >> /sys/block/*/queue/protect?
>> >
>> > Certainly better than current solution. Or maybe ioctl similar to wat
>> > hdparm uses?
>>
>> I'm not quite sure what you have in mind wrt ioctls. I'm still
>> convinced that the administrator should take a conscious decision when
>> forcing an idle immediate with unload feature on a drive which doesn't
>> announce this capability according to the specs. This is because I
>> have no idea as to how drives might react if they don't support it.
>> Perhaps we should consult linux-ide on this topic.
>
> Yep, I guess linux-ide would have some comments.
At least, I can now see the benefits of an ioctl approach. Although
the protect_method attribute allows to configure each drive
individually, that is actually not quite what we want. Since this
attribute is meant to deal with drives that don't comply strictly with
the ATA specs, it is, at least, misleading to associate this with each
queue. Instead, it would be desirable to offer this option to ATA
drives only and to do this consistently, regardless whether the IDE or
the ATA drivers are in use. So, I'm now quite convinced that ioctls
are the easiest way to achieve just that - thanks for the hint.
>
>> So, here is a patch in which your remarks and suggestions have been
>> incorporated. Additionally, I've added the requested kernel doc file
>
> Additional suggestion is to keep lines < 80 columns.... Sorry it took
> me so long to comment.
> Pavel
In the meantime, I've started to implement the generic block layer
commands for queue freezing as Jens suggested in this thread.
Unfortunately, I've been very busy lately and actually I still am but
I'll try hard to get this finished rather sooner thn later.
Thanks for your support.
Elias