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

[Tejun Heo <tj@kernel.org>]

Hello, Elias.

Ah... we're so close but no ack yet.  Just a few nits.

It would be nice if there's explanation why action pulling is
necessary in the first place.

> +static inline void ata_eh_pull_park_action(struct ata_port *ap)
> +{
> +	struct ata_link *link;
> +	struct ata_device *dev;
> +	unsigned long flags;
> +
> +	/*
> +	 * All write accesses to &ap->park_req_pending through
> +	 * INIT_COMPLETION() (see below) or complete_all() (see
> +	 * ata_scsi_park_store()) are protected by the host lock. As a
> +	 * result we have that park_req_pending.done is zero on exit
> +	 * from this function, i.e. when ATA_EH_PARK actions for *all*
> +	 * devices on port ap have been pulled into the respective
> +	 * eh_context structs. If, and only if, park_req_pending.done
> +	 * is non-zero by the time we reach
> +	 * wait_for_completion_timeout(), another ATA_EH_PARK action
> +	 * has been scheduled for at least one of the devices on port
> +	 * ap and we have to cycle over the do { } while () loop in
> +	 * ata_eh_recover() again.
> +	 */
...
> +	do {
> +		unsigned long now;
> +
> +		ata_eh_pull_park_action(ap);

How about adding the folloiwng to the above line?
						/* clears park_req_pending */

> +static ssize_t ata_scsi_park_store(struct device *device,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t len)
> +{
...
> +		complete_all(&ap->park_req_pending);

Sorry to catching this this late but calling complete_all() twice will
overflow the done counter.  I think complete() should just work here,
no?

Thanks.

-- 
tejun