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
-
: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
