| Message ID | 20230406113252.41211-10-nks@flawful.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Command Duration Limits support | expand |
On Thu, Apr 06, 2023 at 01:32:38PM +0200, Niklas Cassel wrote: > + /* > + * For ATA devices, CDL needs to be enabled with a SET FEATURES command. > + */ > + if (is_ata) { I don't think these hacks have any business in the SCSI layer. We should probbaly just do this unconditionally for CDL enabled ATA devices at probe time.
On 4/6/23 13:32, Niklas Cassel wrote: > From: Damien Le Moal <damien.lemoal@opensource.wdc.com> > > Add the sysfs scsi device attribute cdl_enable to allow a user to turn > enable or disable a device command duration limits feature. CDL is > disabled by default. This feature must be explicitly enabled by a user by > setting the cdl_enable attribute to 1. > > The new function scsi_cdl_enable() does not do anything beside setting > the cdl_enable field of struct scsi_device in the case of a (real) scsi > device (e.g. a SAS HDD). For ATA devices, the command duration limits > feature needs to be enabled/disabled using the ATA feature sub-page of > the control mode page. To do so, the scsi_cdl_enable() function checks > if this mode page is supported using scsi_mode_sense(). If it is, > scsi_mode_select() is used to enable and disable CDL. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > Co-developed-by: Niklas Cassel <niklas.cassel@wdc.com> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > Documentation/ABI/testing/sysfs-block-device | 13 ++++ > drivers/scsi/scsi.c | 62 ++++++++++++++++++++ > drivers/scsi/scsi_sysfs.c | 28 +++++++++ > include/scsi/scsi_device.h | 2 + > 4 files changed, 105 insertions(+) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 4/11/23 15:16, Christoph Hellwig wrote: > On Thu, Apr 06, 2023 at 01:32:38PM +0200, Niklas Cassel wrote: >> + /* >> + * For ATA devices, CDL needs to be enabled with a SET FEATURES command. >> + */ >> + if (is_ata) { > > I don't think these hacks have any business in the SCSI layer. We should > probbaly just do this unconditionally for CDL enabled ATA devices at > probe time. Yes, this is not pretty, but this has the advantage of not requiring a lot of special code for ata, especially the sysfs attribute does not have to be defined in both scsi and ata. But yes, I guess we could just unconditionally enable CDL for ATA on device scan to be on par with scsi, which has CDL always enabled.
On Tue, Apr 11, 2023 at 04:09:34PM +0900, Damien Le Moal wrote: > But yes, I guess we could just unconditionally enable CDL for ATA on device scan > to be on par with scsi, which has CDL always enabled. I'd prefer that. With a module option to not enable it just to be safe.
On 4/11/23 16:23, Christoph Hellwig wrote: > On Tue, Apr 11, 2023 at 04:09:34PM +0900, Damien Le Moal wrote: >> But yes, I guess we could just unconditionally enable CDL for ATA on device scan >> to be on par with scsi, which has CDL always enabled. > > I'd prefer that. With a module option to not enable it just to be > safe. Then it may be better to move the cdl_enable attribute store definition for ATA devices to libata. That would be less messy that way. Let me see if that can be done cleanly.
On Tue, Apr 11, 2023 at 08:16:48AM +0200, Christoph Hellwig wrote: > On Thu, Apr 06, 2023 at 01:32:38PM +0200, Niklas Cassel wrote: > > + /* > > + * For ATA devices, CDL needs to be enabled with a SET FEATURES command. > > + */ > > + if (is_ata) { > > I don't think these hacks have any business in the SCSI layer. We should > probbaly just do this unconditionally for CDL enabled ATA devices at > probe time. Hello Christoph, While I agree that the pattern isn't especially beautiful, the pattern is used in the SCSI layer already, authored by none other than one of the SCSI maintainers themselves: https://github.com/torvalds/linux/blame/v6.3-rc6/drivers/scsi/sd.c#L3066-L3074 I guess we could try to clean up all the occurrences of this pattern, but considering that the pattern has existed in the SCSI layer for 10 years already, is it something that has to be addressed before this series can be accepted? Kind regards, Niklas
On 4/11/23 16:23, Christoph Hellwig wrote: > On Tue, Apr 11, 2023 at 04:09:34PM +0900, Damien Le Moal wrote: >> But yes, I guess we could just unconditionally enable CDL for ATA on device scan >> to be on par with scsi, which has CDL always enabled. > > I'd prefer that. With a module option to not enable it just to be > safe. Thinking more about this, we cannot unconditionally enable CDL. The reason is that CDL and NCQ priority are mutually exclusive: if CDL is enabled, NCQ priority cannot be used. So with CDL unconditionally enabled, we cannot have NCQ priority enabled, preventing the user from choosing its preferred IO latency control method.
On Tue, Apr 11, 2023 at 04:38:48PM +0900, Damien Le Moal wrote: > On 4/11/23 16:23, Christoph Hellwig wrote: > > On Tue, Apr 11, 2023 at 04:09:34PM +0900, Damien Le Moal wrote: > >> But yes, I guess we could just unconditionally enable CDL for ATA on device scan > >> to be on par with scsi, which has CDL always enabled. > > > > I'd prefer that. With a module option to not enable it just to be > > safe. > > Then it may be better to move the cdl_enable attribute store definition for ATA > devices to libata. That would be less messy that way. Let me see if that can be > done cleanly. I don't think that the SCSI mode select can just be replaced with simple SET FEATURES in libata. If we move the SET FEATURES call to a function in libata, and then use a function pointer in the scsi_host_template, and let only libata set this function pointer (similar to e.g. how the queue_depth sysfs attribute works), then the code will no longer work for SATA devices connected to a SAS HBA. The current code simply checks if VPD page89 (the ATA Information VPD page - which is defined in the SCSI to ATA Translation (SAT) standard) exists. This page (and thus the sdev->vpd_pg89 pointer) will only exist if either: 1) An ATA device is connected to a SATA controller. This page will then be implemented by libata. 2) An ATA device is connected to a SAS HBA. The SAS HB will then provide this page. (The page will not exist for SCSI devices connected to the same SAS HBA.) For case 1) with the current code, scsi.c will call scsi_mode_select() which will be translated by libata before being sent down to the device. For case 2) with the current code, scsi.c will send down a SCSI mode select to the SAS HBA, and the SAS HBA will be responsible for translating the command before sending it to the device. So I actually think that the current way to check if vpd page89 exists is probably the "cleanest" solution that I can think of. If you have a better suggestion that will work for both case 1) and case 2), I will be happy to change the code accordingly. Kind regards, Niklas
On Tue, Apr 11, 2023 at 01:59:44PM +0200, Niklas Cassel wrote: > On Tue, Apr 11, 2023 at 04:38:48PM +0900, Damien Le Moal wrote: > > On 4/11/23 16:23, Christoph Hellwig wrote: > > > On Tue, Apr 11, 2023 at 04:09:34PM +0900, Damien Le Moal wrote: > > >> But yes, I guess we could just unconditionally enable CDL for ATA on device scan > > >> to be on par with scsi, which has CDL always enabled. > > > > > > I'd prefer that. With a module option to not enable it just to be > > > safe. > > > > Then it may be better to move the cdl_enable attribute store definition for ATA > > devices to libata. That would be less messy that way. Let me see if that can be > > done cleanly. > > I don't think that the SCSI mode select can just be replaced with simple > SET FEATURES in libata. > > If we move the SET FEATURES call to a function in libata, and then use a > function pointer in the scsi_host_template, and let only libata set this > function pointer (similar to e.g. how the queue_depth sysfs attribute works), > then the code will no longer work for SATA devices connected to a SAS HBA. > > > > The current code simply checks if VPD page89 (the ATA Information VPD > page - which is defined in the SCSI to ATA Translation (SAT) standard) > exists. This page (and thus the sdev->vpd_pg89 pointer) will only exist > if either: > 1) An ATA device is connected to a SATA controller. This page will then > be implemented by libata. > 2) An ATA device is connected to a SAS HBA. The SAS HB will then provide > this page. (The page will not exist for SCSI devices connected to the > same SAS HBA.) > > For case 1) with the current code, scsi.c will call scsi_mode_select() > which will be translated by libata before being sent down to the device. > > For case 2) with the current code, scsi.c will send down a SCSI mode > select to the SAS HBA, and the SAS HBA will be responsible for translating > the command before sending it to the device. > > So I actually think that the current way to check if vpd page89 exists > is probably the "cleanest" solution that I can think of. > > If you have a better suggestion that will work for both case 1) and > case 2), I will be happy to change the code accordingly. In addition to: https://github.com/torvalds/linux/blame/v6.3-rc6/drivers/scsi/sd.c#L3066-L3074 checking for the existence of VPD page89 is also currently done by e.g.: https://github.com/torvalds/linux/blob/v6.3-rc6/drivers/scsi/mpt3sas/mpt3sas_scsih.c#L12607 and https://github.com/torvalds/linux/blob/v6.3-rc6/drivers/hwmon/drivetemp.c#L340 Kind regards, Niklas
On 4/11/23 20:59, Niklas Cassel wrote: > On Tue, Apr 11, 2023 at 04:38:48PM +0900, Damien Le Moal wrote: >> On 4/11/23 16:23, Christoph Hellwig wrote: >>> On Tue, Apr 11, 2023 at 04:09:34PM +0900, Damien Le Moal wrote: >>>> But yes, I guess we could just unconditionally enable CDL for ATA on device scan >>>> to be on par with scsi, which has CDL always enabled. >>> >>> I'd prefer that. With a module option to not enable it just to be >>> safe. >> >> Then it may be better to move the cdl_enable attribute store definition for ATA >> devices to libata. That would be less messy that way. Let me see if that can be >> done cleanly. > > I don't think that the SCSI mode select can just be replaced with simple > SET FEATURES in libata. > > If we move the SET FEATURES call to a function in libata, and then use a > function pointer in the scsi_host_template, and let only libata set this > function pointer (similar to e.g. how the queue_depth sysfs attribute works), > then the code will no longer work for SATA devices connected to a SAS HBA. > > > > The current code simply checks if VPD page89 (the ATA Information VPD > page - which is defined in the SCSI to ATA Translation (SAT) standard) > exists. This page (and thus the sdev->vpd_pg89 pointer) will only exist > if either: > 1) An ATA device is connected to a SATA controller. This page will then > be implemented by libata. > 2) An ATA device is connected to a SAS HBA. The SAS HB will then provide > this page. (The page will not exist for SCSI devices connected to the > same SAS HBA.) > > For case 1) with the current code, scsi.c will call scsi_mode_select() > which will be translated by libata before being sent down to the device. > > For case 2) with the current code, scsi.c will send down a SCSI mode > select to the SAS HBA, and the SAS HBA will be responsible for translating > the command before sending it to the device. > > So I actually think that the current way to check if vpd page89 exists > is probably the "cleanest" solution that I can think of. > > If you have a better suggestion that will work for both case 1) and > case 2), I will be happy to change the code accordingly. Good point. If we move the code for cdl_enable to libata, then we will not be covering the SAS HBA cases. Christoph, I do not see a cleaner solution... Can we keep this patch as is ? Any other idea ?
On Wed, Apr 12, 2023 at 09:59:30AM +0900, Damien Le Moal wrote: > Good point. If we move the code for cdl_enable to libata, then we will not be > covering the SAS HBA cases. > > Christoph, > > I do not see a cleaner solution... Can we keep this patch as is ? Any other idea ? I guess we have to. But it's really ugly and someone in T13 really needs to be slapped..
On 4/12/23 13:43, Christoph Hellwig wrote: > On Wed, Apr 12, 2023 at 09:59:30AM +0900, Damien Le Moal wrote: >> Good point. If we move the code for cdl_enable to libata, then we will not be >> covering the SAS HBA cases. >> >> Christoph, >> >> I do not see a cleaner solution... Can we keep this patch as is ? Any other idea ? > > I guess we have to. But it's really ugly and someone in T13 really needs > to be slapped.. It is T10 rather than T13 that screwed up: if SPC also defined a CDL feature on/off through mode sense/select, we would not need any "if (is_ata)"... For ATA/T13, the on/off makes sense because of the mutual exclusion with NCQ priority.
On Wed, Apr 12, 2023 at 02:32:30PM +0900, Damien Le Moal wrote: > It is T10 rather than T13 that screwed up: if SPC also defined a CDL feature > on/off through mode sense/select, we would not need any "if (is_ata)"... > For ATA/T13, the on/off makes sense because of the mutual exclusion with NCQ > priority. Same persons really, so they should know better..
diff --git a/Documentation/ABI/testing/sysfs-block-device b/Documentation/ABI/testing/sysfs-block-device index ee3610a25845..626d48ac504b 100644 --- a/Documentation/ABI/testing/sysfs-block-device +++ b/Documentation/ABI/testing/sysfs-block-device @@ -104,3 +104,16 @@ Contact: linux-scsi@vger.kernel.org Description: (RO) Indicates if the device supports the command duration limits feature found in some ATA and SCSI devices. + + +What: /sys/block/*/device/cdl_enable +Date: Mar, 2023 +KernelVersion: v6.4 +Contact: linux-scsi@vger.kernel.org +Description: + (RW) For a device supporting the command duration limits + feature, write to the file to turn on or off the feature. + By default this feature is turned off. + Writing "1" to this file enables the use of command duration + limits for read and write commands in the kernel and turns on + the feature on the device. Writing "0" disables the feature. diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index c03814ce23ca..c4bf99a842f3 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -651,6 +651,68 @@ void scsi_cdl_check(struct scsi_device *sdev) kfree(buf); } +/** + * scsi_cdl_enable - Enable or disable a SCSI device supports for Command + * Duration Limits + * @sdev: The target device + * @enable: the target state + */ +int scsi_cdl_enable(struct scsi_device *sdev, bool enable) +{ + struct scsi_mode_data data; + struct scsi_sense_hdr sshdr; + struct scsi_vpd *vpd; + bool is_ata = false; + char buf[64]; + int ret; + + if (!sdev->cdl_supported) + return -EOPNOTSUPP; + + rcu_read_lock(); + vpd = rcu_dereference(sdev->vpd_pg89); + if (vpd) + is_ata = true; + rcu_read_unlock(); + + /* + * For ATA devices, CDL needs to be enabled with a SET FEATURES command. + */ + if (is_ata) { + char *buf_data; + int len; + + ret = scsi_mode_sense(sdev, 0x08, 0x0a, 0xf2, buf, sizeof(buf), + 5 * HZ, 3, &data, NULL); + if (ret) + return -EINVAL; + + /* Enable CDL using the ATA feature page */ + len = min_t(size_t, sizeof(buf), + data.length - data.header_length - + data.block_descriptor_length); + buf_data = buf + data.header_length + + data.block_descriptor_length; + if (enable) + buf_data[4] = 0x02; + else + buf_data[4] = 0; + + ret = scsi_mode_select(sdev, 1, 0, buf_data, len, 5 * HZ, 3, + &data, &sshdr); + if (ret) { + if (scsi_sense_valid(&sshdr)) + scsi_print_sense_hdr(sdev, + dev_name(&sdev->sdev_gendev), &sshdr); + return ret; + } + } + + sdev->cdl_enable = enable; + + return 0; +} + /** * scsi_device_get - get an additional reference to a scsi_device * @sdev: device to get a reference to diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 4994148e685b..0b155e52bb4d 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1222,6 +1222,33 @@ static DEVICE_ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR, sdev_show_queue_ramp_up_period, sdev_store_queue_ramp_up_period); +static ssize_t sdev_show_cdl_enable(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct scsi_device *sdev = to_scsi_device(dev); + + return sysfs_emit(buf, "%d\n", (int)sdev->cdl_enable); +} + +static ssize_t sdev_store_cdl_enable(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int ret; + bool v; + + if (kstrtobool(buf, &v)) + return -EINVAL; + + ret = scsi_cdl_enable(to_scsi_device(dev), v); + if (ret) + return ret; + + return count; +} +static DEVICE_ATTR(cdl_enable, S_IRUGO | S_IWUSR, + sdev_show_cdl_enable, sdev_store_cdl_enable); + static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj, struct attribute *attr, int i) { @@ -1302,6 +1329,7 @@ static struct attribute *scsi_sdev_attrs[] = { #endif &dev_attr_queue_ramp_up_period.attr, &dev_attr_cdl_supported.attr, + &dev_attr_cdl_enable.attr, REF_EVT(media_change), REF_EVT(inquiry_change_reported), REF_EVT(capacity_change_reported), diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 6b8df9e253a0..b2cdb078b7bd 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -219,6 +219,7 @@ struct scsi_device { unsigned no_vpd_size:1; /* No VPD size reported in header */ unsigned cdl_supported:1; /* Command duration limits supported */ + unsigned cdl_enable:1; /* Enable/disable Command duration limits */ unsigned int queue_stopped; /* request queue is quiesced */ bool offline_already; /* Device offline message logged */ @@ -367,6 +368,7 @@ extern void scsi_remove_device(struct scsi_device *); extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh); void scsi_attach_vpd(struct scsi_device *sdev); void scsi_cdl_check(struct scsi_device *sdev); +int scsi_cdl_enable(struct scsi_device *sdev, bool enable); extern struct scsi_device *scsi_device_from_queue(struct request_queue *q); extern int __must_check scsi_device_get(struct scsi_device *);