[PATCH] scsi: core: Disabe CDL by default

[PATCH] scsi: core: Disabe 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 to the user provided bool value.

However, for ATA devices, a drive may spin-up with the CDL feature
either enabled or disabled by default, depending on the drive. But the
scsi device cdl_enable field is always initialized to false (CDL
disabled), regardless of the actual device CDL feature state.

Add a call to scsi_cdl_enable() in scsi_cdl_check() to make sure that
the device-side state of the CDL feature always matches the scsi device
cdl_enable field state, thus avoiding inconsistencies for devices that
have CDL enabled when first scanned. This implies that CDL will always
be disabled, as it should be, when the system first scans the devices.

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>
---
 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] scsi: core: Disabe CDL by default

[Christoph Hellwig]

On Thu, Jun 06, 2024 at 02:46:06PM +0900, Damien Le Moal wrote:
> However, for ATA devices, a drive may spin-up with the CDL feature
> either enabled or disabled by default, depending on the drive. But the
> scsi device cdl_enable field is always initialized to false (CDL
> disabled), regardless of the actual device CDL feature state.

The same should be true for native SCSI as well, right?

The patch itself looks good, though:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] scsi: core: Disabe CDL by default

[Damien Le Moal]

On 6/6/24 18:32, Christoph Hellwig wrote:
> On Thu, Jun 06, 2024 at 02:46:06PM +0900, Damien Le Moal wrote:
>> However, for ATA devices, a drive may spin-up with the CDL feature
>> either enabled or disabled by default, depending on the drive. But the
>> scsi device cdl_enable field is always initialized to false (CDL
>> disabled), regardless of the actual device CDL feature state.
> 
> The same should be true for native SCSI as well, right?

Nope, because SPC does not define a knob to turn CDL on/off on the device. For
SAS drives, if CDL is supported, it is always enabled/usable. This is why
scsi_enable_cdl() is reduced to setting sdev->cdl_enable only (and also why
there that ugly "if (is_ata)".

> 
> The patch itself looks good, though:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi: core: Disabe CDL by default

[Niklas Cassel]

Hello Damien,

s/Disabe/Disable/
in $subject

On Thu, Jun 06, 2024 at 02:46:06PM +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 to the user provided bool value.
> 
> However, for ATA devices, a drive may spin-up with the CDL feature
> either enabled or disabled by default, depending on the drive. But the
> scsi device cdl_enable field is always initialized to false (CDL
> disabled), regardless of the actual device CDL feature state.
> 
> Add a call to scsi_cdl_enable() in scsi_cdl_check() to make sure that
> the device-side state of the CDL feature always matches the scsi device
> cdl_enable field state, thus avoiding inconsistencies for devices that
> have CDL enabled when first scanned. This implies that CDL will always
> be disabled, as it should be, when the system first scans the devices.
> 
> 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>
> ---
>  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;
>  	}

Perhaps I'm missing something here, but since this is only a problem for
ATA devices, where the device might have CDL enabled on the device,
but disabled in sysfs, why isn't this code disabling it:
https://github.com/torvalds/linux/blob/v6.10-rc2/drivers/ata/libata-core.c#L2551-L2572

The whole point of that code is to keep the device in sync with the
device/sysfs value.

Can't we modify ata_dev_config_cdl() such that we can avoid doing basically
the same sync (only needed for ATA devices) in two different functions?


Kind regards,
Niklas

Re: [PATCH] scsi: core: Disabe CDL by default

[Niklas Cassel]

On Thu, Jun 06, 2024 at 03:23:41PM +0200, Niklas Cassel wrote:
> Hello Damien,
>
> s/Disabe/Disable/
> in $subject
>
> On Thu, Jun 06, 2024 at 02:46:06PM +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 to the user provided bool value.
> >
> > However, for ATA devices, a drive may spin-up with the CDL feature
> > either enabled or disabled by default, depending on the drive. But the
> > scsi device cdl_enable field is always initialized to false (CDL
> > disabled), regardless of the actual device CDL feature state.
> >
> > Add a call to scsi_cdl_enable() in scsi_cdl_check() to make sure that
> > the device-side state of the CDL feature always matches the scsi device
> > cdl_enable field state, thus avoiding inconsistencies for devices that
> > have CDL enabled when first scanned. This implies that CDL will always
> > be disabled, as it should be, when the system first scans the devices.
> >
> > 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>
> > ---
> >  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;
> >	}
>
> Perhaps I'm missing something here, but since this is only a problem for
> ATA devices, where the device might have CDL enabled on the device,
> but disabled in sysfs, why isn't this code disabling it:
> https://github.com/torvalds/linux/blob/v6.10-rc2/drivers/ata/libata-core.c#L2551-L2572
>
> The whole point of that code is to keep the device in sync with the
> device/sysfs value.
>
> Can't we modify ata_dev_config_cdl() such that we can avoid doing basically
> the same sync (only needed for ATA devices) in two different functions?

So what I don't see right now is, the libata code:

	val = get_unaligned_le64(&ap->sector_buf[8]);
	cdl_enabled = val & BIT_ULL(63) && val & BIT_ULL(21);
	if (dev->flags & ATA_DFLAG_CDL_ENABLED) {
		if (!cdl_enabled) {
			/* Enable CDL on the device */
			err_mask = ata_dev_set_feature(dev, SETFEATURES_CDL, 1);
			if (err_mask) {
				ata_dev_err(dev,
					    "Enable CDL feature failed\n");
				goto not_supported;
			}
		}
	} else {
		if (cdl_enabled) {
			/* Disable CDL on the device */
			err_mask = ata_dev_set_feature(dev, SETFEATURES_CDL, 0);
			if (err_mask) {
				ata_dev_err(dev,
					    "Disable CDL feature failed\n");
				goto not_supported;
			}
		}
	}


cdl_enabled was from a ata_read_log_page(..., ATA_LOG_CURRENT_SETTINGS, ...)
call, so it should get the value directly from the device, which IIUC,
is enabled by default, so it should be enabled.

ATA_DFLAG_CDL_ENABLED
is from ata_mselect_control_ata_feature(), so translated MODE SELECT,
but no one should have called this at scsi probe time, so I would
expect ATA_DFLAG_CDL_ENABLED to not be set.

Is the problem really when at scsi probe time, or is it when the
user writes the sysfs value the first time where things go wrong?

The commit message mentions "thus avoiding inconsistencies for devices
that have CDL enabled when first scanned", but perhaps you could be
explain in more detail why the current code is not working?


Kind regards,
Niklas

Re: [PATCH] scsi: core: Disabe CDL by default

[Damien Le Moal]

On 6/6/24 22:23, Niklas Cassel wrote:
> Hello Damien,
> 
> s/Disabe/Disable/
> in $subject

Good catch. Will fix that.

> 
> On Thu, Jun 06, 2024 at 02:46:06PM +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 to the user provided bool value.
>>
>> However, for ATA devices, a drive may spin-up with the CDL feature
>> either enabled or disabled by default, depending on the drive. But the
>> scsi device cdl_enable field is always initialized to false (CDL
>> disabled), regardless of the actual device CDL feature state.
>>
>> Add a call to scsi_cdl_enable() in scsi_cdl_check() to make sure that
>> the device-side state of the CDL feature always matches the scsi device
>> cdl_enable field state, thus avoiding inconsistencies for devices that
>> have CDL enabled when first scanned. This implies that CDL will always
>> be disabled, as it should be, when the system first scans the devices.
>>
>> 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>
>> ---
>>  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;
>>  	}
> 
> Perhaps I'm missing something here, but since this is only a problem for
> ATA devices, where the device might have CDL enabled on the device,
> but disabled in sysfs, why isn't this code disabling it:
> https://github.com/torvalds/linux/blob/v6.10-rc2/drivers/ata/libata-core.c#L2551-L2572

The inconsistency happen with ATA devices connected to a SAS HBA. This patch is
for such setup only. libata (and libsas) managed ATA CDL devices are fine thanks
to the code you pointed out.

> 
> The whole point of that code is to keep the device in sync with the
> device/sysfs value.
> 
> Can't we modify ata_dev_config_cdl() such that we can avoid doing basically
> the same sync (only needed for ATA devices) in two different functions?

No need, that is all fine. Again, this patch is for SAS HBA connected ATA
devices only. I will update the commit message to make this clear.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi: core: Disabe CDL by default

[Bart Van Assche]

On 6/5/24 23:46, 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 to the user provided bool value.

Do we really need to disable CDL by default? Has it been considered to
enable CDL by default if the Command Duration Limit mode pages that have
been defined in SPC-5 are supported?

Thanks,

Bart.

Re: [PATCH] scsi: core: Disabe CDL by default

[Damien Le Moal]

On 6/7/24 00:48, Bart Van Assche wrote:
> On 6/5/24 23:46, 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 to the user provided bool value.
> 
> Do we really need to disable CDL by default? Has it been considered to
> enable CDL by default if the Command Duration Limit mode pages that have
> been defined in SPC-5 are supported?

We cannot do that as that would potentially break users of ATA NCQ priority. The
reason is that for ATA drives that support both NCQ priority and CDL, if CDL is
enabled, NCQ priority cannot be used. If we were to change CDL to be enabled by
default, NCQ priority users would need to disable it before enabling NCQ
priority. That can break existing setups.

And this has been like this since kernel 6.5, so I am not going to change this now.

-- 
Damien Le Moal
Western Digital Research