[PATCH v2] scsi: core: Disable CDL by default

[PATCH v2] scsi: core: Disable CDL by default

[Damien Le Moal]

For scsi devices supporting the Command Duration Limits feature set, the
user can enable/disable this feature use through the sysfs device
attribute cdl_enable. This attribute modification triggers a call to
scsi_cdl_enable() to enable and disable the feature for ATA devices and
set the scsi device cdl_enable field to the user provided bool value.
For SCSI devices supporting CDL, the feature set is always enabled and
scsi_cdl_enable() is reduced to setting the cdl_enable field.

However, for ATA devices, a drive may spin-up with the CDL feature
enabled by default. But the scsi device cdl_enable field is always
initialized to false (CDL disabled), regardless of the actual device
CDL feature state. For ATA devices managed by libata (or libsas),
libata-core always disables the CDL feature set when the device is
attached, thus syncing the state of the CDL feature on the device and of
the scsi device cdl_enable field. However, for ATA devices connected to
a SAS HBA, the CDL feature is not disabled on scan for ATA devices that
have this feature enabled by default, leading to an inconsistent state
of the feature on the device with the scsi device cdl_enable field.

Avoid this inconsistency by adding a call to scsi_cdl_enable() in
scsi_cdl_check() to make sure that the device-side state of the CDL
feature set always matches the scsi device cdl_enable field state.
This implies that CDL will always be disabled for ATA devices connected
to SAS HBAs, which is consistent with libata/libsas initialization of
the device.

Reported-by: Scott McCoy <scott.mccoy@wdc.com>
Fixes: 1b22cfb14142 ("scsi: core: Allow enabling and disabling command duration limits")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---

Changes from v1:
 - Fixed typo in the commit title and improved the commit message

 drivers/scsi/scsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3e0c0381277a..9e9576066e8d 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -666,6 +666,13 @@ void scsi_cdl_check(struct scsi_device *sdev)
 		sdev->use_10_for_rw = 0;
 
 		sdev->cdl_supported = 1;
+
+		/*
+		 * If the device supports CDL, make sure that the current drive
+		 * feature status is consistent with the user controlled
+		 * cdl_enable state.
+		 */
+		scsi_cdl_enable(sdev, sdev->cdl_enable);
 	} else {
 		sdev->cdl_supported = 0;
 	}
-- 
2.45.2

Re: [PATCH v2] scsi: core: Disable CDL by default

[Niklas Cassel]

On Fri, Jun 07, 2024 at 10:25:07AM +0900, Damien Le Moal wrote:
> For scsi devices supporting the Command Duration Limits feature set, the
> user can enable/disable this feature use through the sysfs device
> attribute cdl_enable. This attribute modification triggers a call to
> scsi_cdl_enable() to enable and disable the feature for ATA devices and
> set the scsi device cdl_enable field to the user provided bool value.
> For SCSI devices supporting CDL, the feature set is always enabled and
> scsi_cdl_enable() is reduced to setting the cdl_enable field.
> 
> However, for ATA devices, a drive may spin-up with the CDL feature
> enabled by default. But the scsi device cdl_enable field is always
> initialized to false (CDL disabled), regardless of the actual device
> CDL feature state. For ATA devices managed by libata (or libsas),
> libata-core always disables the CDL feature set when the device is
> attached, thus syncing the state of the CDL feature on the device and of
> the scsi device cdl_enable field. However, for ATA devices connected to
> a SAS HBA, the CDL feature is not disabled on scan for ATA devices that
> have this feature enabled by default, leading to an inconsistent state
> of the feature on the device with the scsi device cdl_enable field.
> 
> Avoid this inconsistency by adding a call to scsi_cdl_enable() in
> scsi_cdl_check() to make sure that the device-side state of the CDL
> feature set always matches the scsi device cdl_enable field state.
> This implies that CDL will always be disabled for ATA devices connected
> to SAS HBAs, which is consistent with libata/libsas initialization of
> the device.
> 
> Reported-by: Scott McCoy <scott.mccoy@wdc.com>
> Fixes: 1b22cfb14142 ("scsi: core: Allow enabling and disabling command duration limits")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH v2] scsi: core: Disable CDL by default

[Igor Pylypiv]

On Fri, Jun 07, 2024 at 10:25:07AM +0900, Damien Le Moal wrote:
> For scsi devices supporting the Command Duration Limits feature set, the
> user can enable/disable this feature use through the sysfs device
> attribute cdl_enable. This attribute modification triggers a call to
> scsi_cdl_enable() to enable and disable the feature for ATA devices and
> set the scsi device cdl_enable field to the user provided bool value.
> For SCSI devices supporting CDL, the feature set is always enabled and
> scsi_cdl_enable() is reduced to setting the cdl_enable field.
> 
> However, for ATA devices, a drive may spin-up with the CDL feature
> enabled by default. But the scsi device cdl_enable field is always
> initialized to false (CDL disabled), regardless of the actual device
> CDL feature state. For ATA devices managed by libata (or libsas),
> libata-core always disables the CDL feature set when the device is
> attached, thus syncing the state of the CDL feature on the device and of
> the scsi device cdl_enable field. However, for ATA devices connected to
> a SAS HBA, the CDL feature is not disabled on scan for ATA devices that
> have this feature enabled by default, leading to an inconsistent state
> of the feature on the device with the scsi device cdl_enable field.
> 
> Avoid this inconsistency by adding a call to scsi_cdl_enable() in
> scsi_cdl_check() to make sure that the device-side state of the CDL
> feature set always matches the scsi device cdl_enable field state.
> This implies that CDL will always be disabled for ATA devices connected
> to SAS HBAs, which is consistent with libata/libsas initialization of
> the device.
> 
> Reported-by: Scott McCoy <scott.mccoy@wdc.com>
> Fixes: 1b22cfb14142 ("scsi: core: Allow enabling and disabling command duration limits")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Thank you!

Re: [PATCH v2] scsi: core: Disable CDL by default

[Hannes Reinecke]

On 6/7/24 03:25, Damien Le Moal wrote:
> For scsi devices supporting the Command Duration Limits feature set, the
> user can enable/disable this feature use through the sysfs device
> attribute cdl_enable. This attribute modification triggers a call to
> scsi_cdl_enable() to enable and disable the feature for ATA devices and
> set the scsi device cdl_enable field to the user provided bool value.
> For SCSI devices supporting CDL, the feature set is always enabled and
> scsi_cdl_enable() is reduced to setting the cdl_enable field.
> 
> However, for ATA devices, a drive may spin-up with the CDL feature
> enabled by default. But the scsi device cdl_enable field is always
> initialized to false (CDL disabled), regardless of the actual device
> CDL feature state. For ATA devices managed by libata (or libsas),
> libata-core always disables the CDL feature set when the device is
> attached, thus syncing the state of the CDL feature on the device and of
> the scsi device cdl_enable field. However, for ATA devices connected to
> a SAS HBA, the CDL feature is not disabled on scan for ATA devices that
> have this feature enabled by default, leading to an inconsistent state
> of the feature on the device with the scsi device cdl_enable field.
> 
> Avoid this inconsistency by adding a call to scsi_cdl_enable() in
> scsi_cdl_check() to make sure that the device-side state of the CDL
> feature set always matches the scsi device cdl_enable field state.
> This implies that CDL will always be disabled for ATA devices connected
> to SAS HBAs, which is consistent with libata/libsas initialization of
> the device.
> 
> Reported-by: Scott McCoy <scott.mccoy@wdc.com>
> Fixes: 1b22cfb14142 ("scsi: core: Allow enabling and disabling command duration limits")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
> 
> Changes from v1:
>   - Fixed typo in the commit title and improved the commit message
> 
>   drivers/scsi/scsi.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2] scsi: core: Disable CDL by default

[Martin K. Petersen]

On Fri, 07 Jun 2024 10:25:07 +0900, Damien Le Moal wrote:

> For scsi devices supporting the Command Duration Limits feature set, the
> user can enable/disable this feature use through the sysfs device
> attribute cdl_enable. This attribute modification triggers a call to
> scsi_cdl_enable() to enable and disable the feature for ATA devices and
> set the scsi device cdl_enable field to the user provided bool value.
> For SCSI devices supporting CDL, the feature set is always enabled and
> scsi_cdl_enable() is reduced to setting the cdl_enable field.
> 
> [...]

Applied to 6.10/scsi-fixes, thanks!

[1/1] scsi: core: Disable CDL by default
      https://git.kernel.org/mkp/scsi/c/52912ca87e2b

-- 
Martin K. Petersen	Oracle Linux Engineering