On Tue, 2012-09-18 at 16:09 +0800, Aaron Lu wrote:
> On Tue, Sep 18, 2012 at 08:56:55AM +0100, James Bottomley wrote:
> > On Tue, 2012-09-18 at 15:00 +0800, Aaron Lu wrote:
> > > When scsi device received stop command, it will take care of its
> > > internal cache before enters stopped power condition. This command is
> > > translated to standby immediate in libata-scsi, but standby doesn't
> > > imply flush cache for ATA device, so to issue stop command to ATA
> > > device, an additional flush cache has to be issued.
> > >
> > > Introduce this flag so that when we are to stop the ATA disk in scsi
> > > disk driver, also flush its internal cache.
> > >
> > > Signed-off-by: Aaron Lu <aaron.lu@xxxxxxxxx>
> > > ---
> > > include/scsi/scsi_device.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> > > index 4712aa1..26c3621 100644
> > > --- a/include/scsi/scsi_device.h
> > > +++ b/include/scsi/scsi_device.h
> > > @@ -160,6 +160,7 @@ struct scsi_device {
> > > unsigned ready_to_power_off:1; /* Device is ready to be powered off */
> > > unsigned powered_off:1; /* Device is powered off */
> > > unsigned may_power_off:1; /* Power off is allowed by user */
> > > + unsigned sync_before_stop:1; /* Sync cache before stop */
> >
> > Why do you need this?
> >
> > Surely it's all conditioned on the WCE flag. If WCE isn't set, the
> > cache is write through (or uncached) and there's no need for a sync
> > before power down.
>
> The set of this flag doesn't mean we will sync cache for sure.
>
> It's only meaningful when WCE is set, and in that case, it means when we
> are to send a stop command to the device, do we need to send an
> additional flush cache command first?
Then surely it indicates support for ACPI power down and it's wrongly
named?
> In sd_suspend, the cache will be synchronized when:
> 1 For devices do not support start_stop, always;
> 2 For devices support start_stop, if it is standard scsi device, never;
> and if it is an ata device(reflected by this newly introduced flag),
> always.
This doesn't look right to me. I think it's probably just a layering
violation. For sd, we need to use the standard SCSI commands. That
means we treat power states as the SCSI standard says. The fact that
ATA devices may be required to translate START_STOP_UNIT with STANDBY as
a flush followed by one of the ATA standby commands. This is very
important: if we construct a libata SATL that doesn't conform to the
standards, things will eventually explode when we try to interact with
devices with their own internal SATL (like the LSI card, or various USB
devices) because eventually we'll make one unexpected interaction too
many.
> The reason for this is, the scsi's stop command = flush_cache +
> standby
> for ata, so I have to introduce this flag to explicitely do a cache
> flush.
>
> Makes sense?
Really, no: your flag equates to "this is an ATA device, treat it
differently" that's knowledge that's not supposed to be in sd unless
absolutely necessary.
James
--
To unsubscribe from this list: 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
