| Message ID | 1347951631-1592-2-git-send-email-aaron.lu@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Tuesday 18 September 2012 15:00:28 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. Why not just set WCE? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 18, 2012 at 09:30:11AM +0200, Oliver Neukum wrote: > On Tuesday 18 September 2012 15:00:28 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. > > Why not just set WCE? This flag is used for devices whose WCE bit is set. This flag means, when we are to issue a scsi stop command, we need to issue an additional sync cache command first. For scsi device, per the spec, there is no such need as the device will take care of its internal cache when going to stop power condition. But for ata device, the stop command is translated to standby immediate, and we have to flush the internal cache before enter standby. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> > --- > 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. James -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> > > --- > > 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? 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. 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? -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> > > > --- > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 18, 2012 at 09:20:20AM +0100, James Bottomley wrote: > 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@intel.com> > > > > --- > > > > 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? It's generic ATA requirement that before standby, cache has to be flushed and unlike scsi, standby does not imply cache flush. > > > 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. I agree that it is better handled in libata's SALT, I tried to do this but didn't find a good way so I introduced this flag. The SALT is 1-1 mapping, I'm not sure how to handle this 1-2 mapping. I'll check this again to see how to do it there. Thanks for your comments. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/18/2012 04:31 AM, Aaron Lu wrote: > I agree that it is better handled in libata's SALT, I tried to do this > but didn't find a good way so I introduced this flag. The SALT is 1-1 > mapping, I'm not sure how to handle this 1-2 mapping. That is the crux of the matter: it is rather difficult within the current code, if there is not a 1:1 correspondence between SCSI command and ATA command. The solution that works within libata-scsi.c involves creating two ata_queued_cmd's, and managing the issuance and completion yourself. But really -- the most practical route seems to be simply to set WCE, to trigger the desired behavior. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */ struct list_head event_list; /* asserted events */
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@intel.com> --- include/scsi/scsi_device.h | 1 + 1 file changed, 1 insertion(+)