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

[Tejun Heo <htejun@gmail.com>]

On Fri, Mar 24, 2006 at 11:03:55PM -0500, Jeff Garzik wrote:
> 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
> 

I'll remove ata_dev_present() for the time being.  It's used in only
one place anyway.  We can do with ata_dev_enabled() ||
ata_dev_disabled() for the time being and resurrect ata_dev_present()
after one or two releases, I think.

> 
> >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.

Agreed, we can revalidate the device and then try to configure the
next lower transfer mode until we succeed.  For the time being, I just
wanna move forward with the current code so that EH changes can be
submitted sooner than later.

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

We don't skip ->post_set_mode() unless @disable_on_err is zero, in
which case the upper layer is responsible for handling the error
condition.  Upper layer will usually reset and reconfigure the whole
thing again, so omitting ->post_set_mode() should be fine there (we
need to force PIO0 before IDENTIFY'ing though).

> 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.

Yeap, my test machine's ICH7 can do ata_piix and my notebook has
ICH6M.

-- 
tejun