Re: [PATCH 3/4] libata: make ata_set_mode() responsible for failure handling

[Tejun Heo <htejun@gmail.com>]

On Mon, Mar 13, 2006 at 04:56:14AM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >On Mon, Mar 13, 2006 at 03:37:17AM -0500, Jeff Garzik wrote:
> >
> >>I have the following concern with this patch (#3) however:
> >>
> >>
> >>>-static void ata_dev_set_mode(struct ata_port *ap, struct ata_device 
> >>>*dev)
> >>>+static int ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
> >>>{
> >>>-	if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
> >>>-		return;
> >>
> >>I think you drop too many ATA_FLAG_PORT_DISABLED tests in this patch, 
> >>leading the code to potentially miss a previously-flagged PORT_DISABLED 
> >>(perhaps by an LLDD).
> >>
> >
> >
> >Hmmm... the plan is to disallow LLDD's take ports or devices offline
> >from low level callbacks.  They should just let upper layer know by
> >returning failure code.
> 
> Long term that plan is fine, but you still have to deal with the 
> existing API one way or another.  ata_port_disable() is called directly 
> by a bunch of Alan's PATA drivers, and by ata_piix and sata_mv.
> 
> Thus you would either need to keep the PORT_DISABLED checks or convert 
> the drivers in question to a better API.
> 
> So just check those ata_port_disable() cases...
> 

AFAICS, ata_piix doesn't call ata_port_disable() directly, but
sata_mv() does, through mv_host_intr() -> mv_err_intr()
->mv_stop_and_reset() -> __mv_phy_reset().  I'll check the code path
such that disabled ports are handled properly.  Thanks for pointing
out.

-- 
tejun