Re: [PATCH 2/4] libata: Implement disk shock protection support

[Tejun Heo <htejun@gmail.com>]

Hello, Elias.

Elias Oltmanns wrote:
> Does the following patch look like what you've had in mind (still
> applies to next-20080903)?

Yes, mostly.  Just a few points.

> +static unsigned long ata_eh_park_devs(struct ata_port *ap)
> +{
> +	struct ata_link *link;
> +	struct ata_device *dev;
> +	struct ata_taskfile tf;
> +	unsigned int err_mask;
> +	unsigned long deadline = jiffies;
> +
> +	ata_port_for_each_link(link, ap) {
> +		ata_link_for_each_dev(dev, link) {
> +			struct ata_eh_context *ehc = &link->eh_context;
> +			struct ata_eh_info *ehi = &link->eh_info;
> +
> +			if (dev->class != ATA_DEV_ATA ||
> +			    dev->flags & ATA_DFLAG_NO_UNLOAD)
> +				continue;
> +
> +			if (ehc->i.dev_action[dev->devno] & ATA_EH_PARK ||
> +			    ehi->dev_action[dev->devno] & ATA_EH_PARK) {
> +				unsigned long tmp = dev->unpark_deadline;

The correct way to do this is ata_eh_about_to_do().  After that, you
can just look at ehc->i.dev_action[].  Also, you'll need to call
ata_eh_done() later.

> +				if (time_before(deadline, tmp))
> +					deadline = tmp;
> +				else if (time_before_eq(tmp, jiffies))
> +					continue;
> +			}
> +
> +			if (ehc->did_unload_mask & (1 << dev->devno))
> +				continue;
> +
> +			ata_tf_init(dev, &tf);
> +			tf.command = ATA_CMD_IDLEIMMEDIATE;
> +			tf.feature = 0x44;
> +			tf.lbal = 0x4c;
> +			tf.lbam = 0x4e;
> +			tf.lbah = 0x55;
> +			tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> +			tf.protocol |= ATA_PROT_NODATA;
> +			err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE,
> +						     NULL, 0, 0);
> +			if (err_mask || tf.lbal != 0xc4)
> +				ata_dev_printk(dev, KERN_ERR,
> +					       "head unload failed!\n");
> +			else
> +				ehc->did_unload_mask |= 1 << dev->devno;
...
> +static void ata_eh_unpark_devs(struct ata_port *ap)
> +{
> +	struct ata_link *link;
> +	struct ata_device *dev;
> +	struct ata_taskfile tf;
> +
> +	ata_port_for_each_link(link, ap) {
> +		ata_link_for_each_dev(dev, link) {
> +			struct ata_eh_context *ehc = &link->eh_context;
> +
> +			if (!(ehc->did_unload_mask & (1 << dev->devno)))
> +				continue;
> +
> +			ata_tf_init(dev, &tf);
> +			tf.command = ATA_CMD_CHK_POWER;
> +			tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> +			tf.protocol |= ATA_PROT_NODATA;
> +			ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);

And it's probably better to have ehc->unloaded_mask instead of
ehc->did_unload_mask and clear it here so that if unload is scheduled
after this point but before EH completes, it does unloading again.
ie. Something like the following.

	ata_eh_done(ATA_EH_UNLOAD);
	ehc->i.unloaded_mask &= ~(1 << dev->devno);

> @@ -2830,6 +2904,19 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  		}
>  	}
>  
> +	do {
> +		unsigned long now;
> +
> +		deadline = ata_eh_park_devs(ap);
> +		now = jiffies;
> +		if (time_before_eq(deadline, now))
> +			break;
> +		prepare_to_wait(&ata_scsi_park_wq, &wait, TASK_UNINTERRUPTIBLE);
> +		deadline = schedule_timeout_uninterruptible(deadline - now);
> +	} while (deadline);
> +	finish_wait(&ata_scsi_park_wq, &wait);
> +	ata_eh_unpark_devs(ap);

I think it would be better to put timeout computation and handling out
here instead of inside ata_eh_park_devs().  ata_eh_park_devs() just
parks the heads if ATA_DEV_UNLOAD and the outer loop controls when it
can continue.

> +static ssize_t ata_scsi_park_store(struct device *device,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t len)
> +{
...

> +		switch (input) {
> +		case -1:
> +			dev->flags &= ~ATA_DFLAG_NO_UNLOAD;
> +			break;
> +		case -2:
> +			dev->flags |= ATA_DFLAG_NO_UNLOAD;
> +			break;

Can't we just drop ATA_DFLAG_NO_UNLOAD?  It doesn't provide any real
functionality anymore.

Thanks.

-- 
tejun