Re: [PATCH 2/2] libata: add @disable_on_err argument to ata_set_mode()

[Jeff Garzik <jgarzik@pobox.com>]

Tejun Heo wrote:
> ata_set_mode() used to disable whole port on failure.  This patch adds
> @disable_on_err which makes ata_set_mode() disable failing devices
> when non-zero, and simply return when zero.  Due to the port-wide
> characteristic of ATA xfer mode configuration, ata_mode_set() is the
> final place to determine device offlining; thus, the @disable_on_err
> mechanism to tell it which action to take on failure.
> 
> With this patch, only failing devices are disabled not the whole port.
> Transfer mode configuration must consider all devices on the port
> regardless of failure status; otherwise, device selection timing can
> be violoated resulting in malfunction.  This patch makes
> ata_dev_xfermask() consider disabled but present devices such that
> device timing selection timing is honored.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK, but dropped due to dropping patch #1


> As said in the above comment, this patch makes sure that present but
> disabled devices are taken into account when determining transfer
> mode.  If IDENTIFY data is present, it is used; otherwise, PIO0 is
> forced.  I think this should be enough.

Several follow-up comments:

* Ideally, I think libata should re-read the identify data from the 
device.  This (a) makes sure PIO is working, and (b) tells us for 
certain what mode the device is.  That's fine for a follow-up patch 
though, since few will exercise this code anyway.

* skipping ->post_set_mode() in this error case being discussing is 
probably unwise.

BTW, got any PATA hardware lying about?  Since you're wandering into 
xfer mode territory, it would better to test PATA than SATA, as xfer 
mode matters more in the PATA realm.  Intel PATA should be fairly easy 
to find, covered by ata_piix, and all the docs are on developer.intel.com.

	Jeff