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

[Tejun Heo <htejun@gmail.com>]

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.

Currently, during probing, the only place where disabling a port is
allowed is ->phy_reset (for compatibility), which ata_bus_probe()
interprets as no active device and reenables the port.  Later,
->error_handler() would have that power too but I don't think any
other callback should be allowed to do that.  Also, no current
in-kernel LLDD does that during probing process.

If we're gonna allow some callbacks to disable ports or devices.  I
think we need to come up with clear rules such that checks can be
performed exactly where needed.

Thanks.

-- 
tejun