LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Pavel Machek <pavel@ucw.cz>
To: Elias Oltmanns <eo@nebensachen.de>
Cc: Jens Axboe <jens.axboe@oracle.com>,
	Christoph Schmid <chris@schlagmichtod.de>,
	linux-kernel@vger.kernel.org
Subject: Re: is there any Hard-disk shock-protection for 2.6.18 and above?
Date: Thu, 30 Nov 2006 18:19:10 +0100	[thread overview]
Message-ID: <20061130171910.GD1860@elf.ucw.cz> (raw)
In-Reply-To: <87d5783fms.fsf@denkblock.local>

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

  reply	other threads:[~2006-11-30 17:19 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <7ibks-1fg-15@gated-at.bofh.it>
     [not found] ` <7kpjn-7th-23@gated-at.bofh.it>
     [not found]   ` <7kDFF-8rd-29@gated-at.bofh.it>
2006-11-27 18:31     ` Elias Oltmanns
2006-11-30 17:19       ` Pavel Machek [this message]
2006-11-30 17:51         ` Shem Multinymous
2006-12-01 14:19         ` Elias Oltmanns
2006-12-02 11:57           ` Pavel Machek
2006-12-10  1:02             ` Elias Oltmanns
2006-12-10  1:16               ` Elias Oltmanns
2006-12-11  8:26                 ` Jens Axboe
2007-02-04 20:41               ` Pavel Machek
2007-02-08 10:04                 ` Elias Oltmanns
2006-11-30 18:43       ` Jens Axboe
2006-11-17 12:47 Christoph Schmid
2006-11-21 20:51 ` Pavel Machek
2006-11-23 18:26   ` Shem Multinymous
2006-11-30 17:11     ` Pavel Machek
2006-11-30 17:47       ` Shem Multinymous
2006-11-24  7:21   ` Jens Axboe
2006-11-26 23:14     ` Jon Escombe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20061130171910.GD1860@elf.ucw.cz \
    --to=pavel@ucw.cz \
    --cc=chris@schlagmichtod.de \
    --cc=eo@nebensachen.de \
    --cc=jens.axboe@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: is there any Hard-disk shock-protection for 2.6.18 and above?' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).