[PATCH 0/3] CDL Feature control improvements

[PATCH 0/3] CDL Feature control improvements

[Damien Le Moal]

Control of the enable/disable state of an ATA device Command Duration
Limits (CDL) features has issues:
1) Incorrect feature state report translation in libata-scsi
2) The state reported when enabling the feature was being ignored, which
caused needless SET FEATURES commands to be issued to the device, thus
causing an unwanted reset of the CDL statistics log page (which is
implied by any CDL activation action).

These patches address these 2 issues.

Martin,

I can take the scsi patch if you are OK with it. Or the reverse, you can
take all patches through the scsi tree if you prefer. Please let me
know.

Damien Le Moal (3):
  ata: libata-scsi: Fix ata_msense_control_ata_feature()
  ata: libata-scsi: Improve CDL control
  scsi: Improve CDL control

 drivers/ata/libata-scsi.c | 19 +++++++++++++++----
 drivers/scsi/scsi.c       | 36 ++++++++++++++++++++++++------------
 2 files changed, 39 insertions(+), 16 deletions(-)

-- 
2.49.0

[PATCH 1/3] ata: libata-scsi: Fix ata_msense_control_ata_feature()

[Damien Le Moal]

For the ATA features subpage of the control mode page, the T10 SAT-6
specifications state that:

For a MODE SENSE command, the SATL shall return the CDL_CTRL field value
that was last set by an application client.

However, the function ata_msense_control_ata_feature() always sets the
CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and
T2B pages. This is thus incorrect and the value 0x02 must be reported
only after the user enables the CDL feature, which is indicated with the
ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the
CDL_CTRL field of the ATA feature subpage of the control mode page must
report a value of 0x00.

Fix ata_msense_control_ata_feature() to report the correct values for
the CDL_CTRL field, according to the enable/disable state of the device
CDL feature.

Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2796c0da8257..e6c652b8a541 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev,
 	 */
 	put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]);
 
-	if (dev->flags & ATA_DFLAG_CDL)
-		buf[4] = 0x02; /* Support T2A and T2B pages */
+	if ((dev->flags & ATA_DFLAG_CDL) &&
+	    (dev->flags & ATA_DFLAG_CDL_ENABLED))
+		buf[4] = 0x02; /* T2A and T2B pages enabled */
 	else
 		buf[4] = 0;
 
-- 
2.49.0

[PATCH 2/3] ata: libata-scsi: Improve CDL control

[Damien Le Moal]

With ATA devices supporting the CDL feature, using CDL requires that the
feature be enabled with a SET FEATURES command. This command is issued
as the translated command for the MODE SELECT command issued by
scsi_cdl_enable() when the user enables CDL through the device
cdl_enable sysfs attribute.

Currently, ata_mselect_control_ata_feature() always translates a MODE
SELECT command for the ATA features subpage of the control mode page to
a SET FEATURES command to enable or disable CDL based on the cdl_ctrl
field. However, there is no need to issue the SET FEATURES command if:
1) The MODE SELECT command requests disabling CDL and CDL is already
   disabled.
2) The MODE SELECT command requests enabling CDL and CDL is already
   enabled.

Fix ata_mselect_control_ata_feature() to issue the SET FEATURES command
only when necessary. Since enabling CDL also implies a reset of the CDL
statistics log page, avoiding useless CDL enable operations also avoids
clearing the CDL statistics log.

Also add debug messages to clearly signal when CDL is being enabled or
disabled using a SET FEATURES command.

Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e6c652b8a541..6c73a8066381 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3910,17 +3910,27 @@ static unsigned int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
 	/* Check cdl_ctrl */
 	switch (buf[0] & 0x03) {
 	case 0:
-		/* Disable CDL */
+		/* Disable CDL if it is enabled */
+		if (!(dev->flags & ATA_DFLAG_CDL_ENABLED))
+			return 0;
+		ata_dev_dbg(dev, "Disabling CDL\n");
 		cdl_action = 0;
 		dev->flags &= ~ATA_DFLAG_CDL_ENABLED;
 		break;
 	case 0x02:
-		/* Enable CDL T2A/T2B: NCQ priority must be disabled */
+		/*
+		 * Enable CDL T2A/T2B if not already enabled. Since this is
+		 * mutually exclusive with NCQ priority, allow this only if NCQ
+		 * priority is disabled.
+		 */
+		if (dev->flags & ATA_DFLAG_CDL_ENABLED)
+			return 0;
 		if (dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED) {
 			ata_dev_err(dev,
 				"NCQ priority must be disabled to enable CDL\n");
 			return -EINVAL;
 		}
+		ata_dev_dbg(dev, "Enabling CDL\n");
 		cdl_action = 1;
 		dev->flags |= ATA_DFLAG_CDL_ENABLED;
 		break;
-- 
2.49.0

[PATCH 3/3] scsi: Improve CDL control

[Damien Le Moal]

With ATA devices supporting the CDL feature, using CDL requires that the
feature be enabled with a SET FEATURES command. This command is issued
as the translated command for the MODE SELECT command issued by
scsi_cdl_enable() when the user enables CDL through the device
cdl_enable sysfs attribute.

However, the implementation of scsi_cdl_enable() always issues a MODE
SELECT command for ATA devices when the enable argument is true, even if
CDL is already enabled on the device. While this does not cause any
issue with using CDL descriptors with read/write commands (the CDL
feature will be enabled on the drive), issuing the MODE SELECT command
even when the device CDL feature is already enabled will cause a reset
of the ATA device CDL statistics log page (as defined in ACS, any CDL
enable action must reset the device statistics).

Avoid this needless actions (and the implied statistics log page reset)
by modifying scsi_cdl_enable() to issue the MODE SELECT command to
enable CDL if and only if CDL is not reported as already enabled on the
device.

And while at it, simplify the initialization of the is_ata boolean
variable and move the declaration of the scsi mode data and sense header
variables to within the scope of ATA device handling.

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 | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 53daf923ad8e..518a252eb6aa 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -707,26 +707,23 @@ void scsi_cdl_check(struct scsi_device *sdev)
  */
 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];
+	bool is_ata;
 	int ret;
 
 	if (!sdev->cdl_supported)
 		return -EOPNOTSUPP;
 
 	rcu_read_lock();
-	vpd = rcu_dereference(sdev->vpd_pg89);
-	if (vpd)
-		is_ata = true;
+	is_ata = rcu_dereference(sdev->vpd_pg89);
 	rcu_read_unlock();
 
 	/*
 	 * For ATA devices, CDL needs to be enabled with a SET FEATURES command.
 	 */
 	if (is_ata) {
+		struct scsi_mode_data data;
+		struct scsi_sense_hdr sshdr;
 		char *buf_data;
 		int len;
 
@@ -735,16 +732,30 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
 		if (ret)
 			return -EINVAL;
 
-		/* Enable CDL using the ATA feature page */
+		/* Enable or disable 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;
+
+		/*
+		 * If we want to enable CDL and CDL is already enabled on the
+		 * device, do nothing. This avoids needlessly resetting the CDL
+		 * statistics on the device as that is implied by the CDL enable
+		 * action. Similar to this, there is no need to do anything if
+		 * we want to disable CDL and CDL is already disabled.
+		 */
+		if (enable) {
+			if ((buf_data[4] & 0x03) == 0x02)
+				goto out;
+			buf_data[4] &= ~0x03;
+			buf_data[4] |= 0x02;
+		} else {
+			if ((buf_data[4] & 0x03) == 0x00)
+				goto out;
+			buf_data[4] &= ~0x03;
+		}
 
 		ret = scsi_mode_select(sdev, 1, 0, buf_data, len, 5 * HZ, 3,
 				       &data, &sshdr);
@@ -756,6 +767,7 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
 		}
 	}
 
+out:
 	sdev->cdl_enable = enable;
 
 	return 0;
-- 
2.49.0

Re: [PATCH 1/3] ata: libata-scsi: Fix ata_msense_control_ata_feature()

[Niklas Cassel]

On Wed, Apr 16, 2025 at 05:42:36PM +0900, Damien Le Moal wrote:
> For the ATA features subpage of the control mode page, the T10 SAT-6
> specifications state that:
> 
> For a MODE SENSE command, the SATL shall return the CDL_CTRL field value
> that was last set by an application client.
> 
> However, the function ata_msense_control_ata_feature() always sets the
> CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and
> T2B pages. This is thus incorrect and the value 0x02 must be reported
> only after the user enables the CDL feature, which is indicated with the
> ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the
> CDL_CTRL field of the ATA feature subpage of the control mode page must
> report a value of 0x00.
> 
> Fix ata_msense_control_ata_feature() to report the correct values for
> the CDL_CTRL field, according to the enable/disable state of the device
> CDL feature.
> 
> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

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

Re: [PATCH 2/3] ata: libata-scsi: Improve CDL control

[Niklas Cassel]

On Wed, Apr 16, 2025 at 05:42:37PM +0900, Damien Le Moal wrote:
> With ATA devices supporting the CDL feature, using CDL requires that the
> feature be enabled with a SET FEATURES command. This command is issued
> as the translated command for the MODE SELECT command issued by
> scsi_cdl_enable() when the user enables CDL through the device
> cdl_enable sysfs attribute.
> 
> Currently, ata_mselect_control_ata_feature() always translates a MODE
> SELECT command for the ATA features subpage of the control mode page to
> a SET FEATURES command to enable or disable CDL based on the cdl_ctrl
> field. However, there is no need to issue the SET FEATURES command if:
> 1) The MODE SELECT command requests disabling CDL and CDL is already
>    disabled.
> 2) The MODE SELECT command requests enabling CDL and CDL is already
>    enabled.
> 
> Fix ata_mselect_control_ata_feature() to issue the SET FEATURES command
> only when necessary. Since enabling CDL also implies a reset of the CDL
> statistics log page, avoiding useless CDL enable operations also avoids
> clearing the CDL statistics log.
> 
> Also add debug messages to clearly signal when CDL is being enabled or
> disabled using a SET FEATURES command.
> 
> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

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

Re: [PATCH 3/3] scsi: Improve CDL control

[Niklas Cassel]

On Wed, Apr 16, 2025 at 05:42:38PM +0900, Damien Le Moal wrote:
> With ATA devices supporting the CDL feature, using CDL requires that the
> feature be enabled with a SET FEATURES command. This command is issued
> as the translated command for the MODE SELECT command issued by
> scsi_cdl_enable() when the user enables CDL through the device
> cdl_enable sysfs attribute.
> 
> However, the implementation of scsi_cdl_enable() always issues a MODE
> SELECT command for ATA devices when the enable argument is true, even if
> CDL is already enabled on the device. While this does not cause any
> issue with using CDL descriptors with read/write commands (the CDL
> feature will be enabled on the drive), issuing the MODE SELECT command
> even when the device CDL feature is already enabled will cause a reset
> of the ATA device CDL statistics log page (as defined in ACS, any CDL
> enable action must reset the device statistics).
> 
> Avoid this needless actions (and the implied statistics log page reset)
> by modifying scsi_cdl_enable() to issue the MODE SELECT command to
> enable CDL if and only if CDL is not reported as already enabled on the
> device.
> 
> And while at it, simplify the initialization of the is_ata boolean
> variable and move the declaration of the scsi mode data and sense header
> variables to within the scope of ATA device handling.
> 
> 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 3/3] scsi: Improve CDL control

[Igor Pylypiv]

On Wed, Apr 16, 2025 at 05:42:38PM +0900, Damien Le Moal wrote:
> With ATA devices supporting the CDL feature, using CDL requires that the
> feature be enabled with a SET FEATURES command. This command is issued
> as the translated command for the MODE SELECT command issued by
> scsi_cdl_enable() when the user enables CDL through the device
> cdl_enable sysfs attribute.
> 
> However, the implementation of scsi_cdl_enable() always issues a MODE
> SELECT command for ATA devices when the enable argument is true, even if
> CDL is already enabled on the device. While this does not cause any
> issue with using CDL descriptors with read/write commands (the CDL
> feature will be enabled on the drive), issuing the MODE SELECT command
> even when the device CDL feature is already enabled will cause a reset
> of the ATA device CDL statistics log page (as defined in ACS, any CDL
> enable action must reset the device statistics).
> 
> Avoid this needless actions (and the implied statistics log page reset)
> by modifying scsi_cdl_enable() to issue the MODE SELECT command to
> enable CDL if and only if CDL is not reported as already enabled on the
> device.

Hi Damien,

What happens when a drive spins up with CDL enabled? Last year you sent
a patch to make sure that CDL gets disabled by default. Is that still
the case?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=52912ca87e2b810e5acdcdc452593d30c9187d8f

Thanks,
Igor

> 
> And while at it, simplify the initialization of the is_ata boolean
> variable and move the declaration of the scsi mode data and sense header
> variables to within the scope of ATA device handling.
> 
> 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 | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 53daf923ad8e..518a252eb6aa 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -707,26 +707,23 @@ void scsi_cdl_check(struct scsi_device *sdev)
>   */
>  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];
> +	bool is_ata;
>  	int ret;
>  
>  	if (!sdev->cdl_supported)
>  		return -EOPNOTSUPP;
>  
>  	rcu_read_lock();
> -	vpd = rcu_dereference(sdev->vpd_pg89);
> -	if (vpd)
> -		is_ata = true;
> +	is_ata = rcu_dereference(sdev->vpd_pg89);
>  	rcu_read_unlock();
>  
>  	/*
>  	 * For ATA devices, CDL needs to be enabled with a SET FEATURES command.
>  	 */
>  	if (is_ata) {
> +		struct scsi_mode_data data;
> +		struct scsi_sense_hdr sshdr;
>  		char *buf_data;
>  		int len;
>  
> @@ -735,16 +732,30 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
>  		if (ret)
>  			return -EINVAL;
>  
> -		/* Enable CDL using the ATA feature page */
> +		/* Enable or disable 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;
> +
> +		/*
> +		 * If we want to enable CDL and CDL is already enabled on the
> +		 * device, do nothing. This avoids needlessly resetting the CDL
> +		 * statistics on the device as that is implied by the CDL enable
> +		 * action. Similar to this, there is no need to do anything if
> +		 * we want to disable CDL and CDL is already disabled.
> +		 */
> +		if (enable) {
> +			if ((buf_data[4] & 0x03) == 0x02)
> +				goto out;
> +			buf_data[4] &= ~0x03;
> +			buf_data[4] |= 0x02;
> +		} else {
> +			if ((buf_data[4] & 0x03) == 0x00)
> +				goto out;
> +			buf_data[4] &= ~0x03;
> +		}
>  
>  		ret = scsi_mode_select(sdev, 1, 0, buf_data, len, 5 * HZ, 3,
>  				       &data, &sshdr);
> @@ -756,6 +767,7 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
>  		}
>  	}
>  
> +out:
>  	sdev->cdl_enable = enable;
>  
>  	return 0;
> -- 
> 2.49.0
> 
> 

Re: [PATCH 3/3] scsi: Improve CDL control

[Damien Le Moal]

On 4/17/25 12:37, Igor Pylypiv wrote:
> On Wed, Apr 16, 2025 at 05:42:38PM +0900, Damien Le Moal wrote:
>> With ATA devices supporting the CDL feature, using CDL requires that the
>> feature be enabled with a SET FEATURES command. This command is issued
>> as the translated command for the MODE SELECT command issued by
>> scsi_cdl_enable() when the user enables CDL through the device
>> cdl_enable sysfs attribute.
>>
>> However, the implementation of scsi_cdl_enable() always issues a MODE
>> SELECT command for ATA devices when the enable argument is true, even if
>> CDL is already enabled on the device. While this does not cause any
>> issue with using CDL descriptors with read/write commands (the CDL
>> feature will be enabled on the drive), issuing the MODE SELECT command
>> even when the device CDL feature is already enabled will cause a reset
>> of the ATA device CDL statistics log page (as defined in ACS, any CDL
>> enable action must reset the device statistics).
>>
>> Avoid this needless actions (and the implied statistics log page reset)
>> by modifying scsi_cdl_enable() to issue the MODE SELECT command to
>> enable CDL if and only if CDL is not reported as already enabled on the
>> device.
> 
> Hi Damien,
> 
> What happens when a drive spins up with CDL enabled? Last year you sent
> a patch to make sure that CDL gets disabled by default. Is that still
> the case?

Yes, that is unchanged so that we keep being consistent with the fact that the
scsi layer starts with the sysfs cdl_enabled attribute set to false. So if an
ATA device starts with CDL enabled, it will be disabled.

That does not cause any issue with the CDL statistics log page because that page
is not persistent and cleared) on power-on-reset events and this change has no
effect on that. The CDL statistics log page will always be cleared on boot/reboot.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] ata: libata-scsi: Fix ata_msense_control_ata_feature()

[Igor Pylypiv]

On Wed, Apr 16, 2025 at 05:42:36PM +0900, Damien Le Moal wrote:
> For the ATA features subpage of the control mode page, the T10 SAT-6
> specifications state that:
> 
> For a MODE SENSE command, the SATL shall return the CDL_CTRL field value
> that was last set by an application client.
> 
> However, the function ata_msense_control_ata_feature() always sets the
> CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and
> T2B pages. This is thus incorrect and the value 0x02 must be reported
> only after the user enables the CDL feature, which is indicated with the
> ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the
> CDL_CTRL field of the ATA feature subpage of the control mode page must
> report a value of 0x00.
> 
> Fix ata_msense_control_ata_feature() to report the correct values for
> the CDL_CTRL field, according to the enable/disable state of the device
> CDL feature.
> 
> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

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

> ---
>  drivers/ata/libata-scsi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 2796c0da8257..e6c652b8a541 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev,
>  	 */
>  	put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]);
>  
> -	if (dev->flags & ATA_DFLAG_CDL)
> -		buf[4] = 0x02; /* Support T2A and T2B pages */
> +	if ((dev->flags & ATA_DFLAG_CDL) &&

Do we need to check the ATA_DFLAG_CDL flag? If ATA_DFLAG_CDL_ENABLED is set
then ATA_DFLAG_CDL must be set as well?

ata_mselect_control_ata_feature() only checks the ATA_DFLAG_CDL_ENABLED flag.

> +	    (dev->flags & ATA_DFLAG_CDL_ENABLED))
> +		buf[4] = 0x02; /* T2A and T2B pages enabled */
>  	else
>  		buf[4] = 0;
>  
> -- 
> 2.49.0
> 
> 

Thanks,
Igor

Re: [PATCH 2/3] ata: libata-scsi: Improve CDL control

[Igor Pylypiv]

On Wed, Apr 16, 2025 at 05:42:37PM +0900, Damien Le Moal wrote:
> With ATA devices supporting the CDL feature, using CDL requires that the
> feature be enabled with a SET FEATURES command. This command is issued
> as the translated command for the MODE SELECT command issued by
> scsi_cdl_enable() when the user enables CDL through the device
> cdl_enable sysfs attribute.
> 
> Currently, ata_mselect_control_ata_feature() always translates a MODE
> SELECT command for the ATA features subpage of the control mode page to
> a SET FEATURES command to enable or disable CDL based on the cdl_ctrl
> field. However, there is no need to issue the SET FEATURES command if:
> 1) The MODE SELECT command requests disabling CDL and CDL is already
>    disabled.
> 2) The MODE SELECT command requests enabling CDL and CDL is already
>    enabled.
> 
> Fix ata_mselect_control_ata_feature() to issue the SET FEATURES command
> only when necessary. Since enabling CDL also implies a reset of the CDL
> statistics log page, avoiding useless CDL enable operations also avoids
> clearing the CDL statistics log.
> 
> Also add debug messages to clearly signal when CDL is being enabled or
> disabled using a SET FEATURES command.
> 
> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

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

Re: [PATCH 3/3] scsi: Improve CDL control

[Igor Pylypiv]

On Thu, Apr 17, 2025 at 08:08:00PM +0900, Damien Le Moal wrote:
> On 4/17/25 12:37, Igor Pylypiv wrote:
> > On Wed, Apr 16, 2025 at 05:42:38PM +0900, Damien Le Moal wrote:
> >> With ATA devices supporting the CDL feature, using CDL requires that the
> >> feature be enabled with a SET FEATURES command. This command is issued
> >> as the translated command for the MODE SELECT command issued by
> >> scsi_cdl_enable() when the user enables CDL through the device
> >> cdl_enable sysfs attribute.
> >>
> >> However, the implementation of scsi_cdl_enable() always issues a MODE
> >> SELECT command for ATA devices when the enable argument is true, even if
> >> CDL is already enabled on the device. While this does not cause any
> >> issue with using CDL descriptors with read/write commands (the CDL
> >> feature will be enabled on the drive), issuing the MODE SELECT command
> >> even when the device CDL feature is already enabled will cause a reset
> >> of the ATA device CDL statistics log page (as defined in ACS, any CDL
> >> enable action must reset the device statistics).
> >>
> >> Avoid this needless actions (and the implied statistics log page reset)
> >> by modifying scsi_cdl_enable() to issue the MODE SELECT command to
> >> enable CDL if and only if CDL is not reported as already enabled on the
> >> device.
> > 
> > Hi Damien,
> > 
> > What happens when a drive spins up with CDL enabled? Last year you sent
> > a patch to make sure that CDL gets disabled by default. Is that still
> > the case?
> 
> Yes, that is unchanged so that we keep being consistent with the fact that the
> scsi layer starts with the sysfs cdl_enabled attribute set to false. So if an
> ATA device starts with CDL enabled, it will be disabled.
> 
> That does not cause any issue with the CDL statistics log page because that page
> is not persistent and cleared) on power-on-reset events and this change has no
> effect on that. The CDL statistics log page will always be cleared on boot/reboot.
> 

Sounds good. Thank you for confirming, Damien!

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

> 
> -- 
> Damien Le Moal
> Western Digital Research

Re: [PATCH 1/3] ata: libata-scsi: Fix ata_msense_control_ata_feature()

[Damien Le Moal]

On 4/18/25 08:50, Igor Pylypiv wrote:
> On Wed, Apr 16, 2025 at 05:42:36PM +0900, Damien Le Moal wrote:
>> For the ATA features subpage of the control mode page, the T10 SAT-6
>> specifications state that:
>>
>> For a MODE SENSE command, the SATL shall return the CDL_CTRL field value
>> that was last set by an application client.
>>
>> However, the function ata_msense_control_ata_feature() always sets the
>> CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and
>> T2B pages. This is thus incorrect and the value 0x02 must be reported
>> only after the user enables the CDL feature, which is indicated with the
>> ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the
>> CDL_CTRL field of the ATA feature subpage of the control mode page must
>> report a value of 0x00.
>>
>> Fix ata_msense_control_ata_feature() to report the correct values for
>> the CDL_CTRL field, according to the enable/disable state of the device
>> CDL feature.
>>
>> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> 
>> ---
>>  drivers/ata/libata-scsi.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 2796c0da8257..e6c652b8a541 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev,
>>  	 */
>>  	put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]);
>>  
>> -	if (dev->flags & ATA_DFLAG_CDL)
>> -		buf[4] = 0x02; /* Support T2A and T2B pages */
>> +	if ((dev->flags & ATA_DFLAG_CDL) &&
> 
> Do we need to check the ATA_DFLAG_CDL flag? If ATA_DFLAG_CDL_ENABLED is set
> then ATA_DFLAG_CDL must be set as well?

Yes, we can remove it. It does not hurt though.

> ata_mselect_control_ata_feature() only checks the ATA_DFLAG_CDL_ENABLED flag.

Indeed, but that actually is wrong I think since we should not attempt to issue
the SET FEATURES command if the drive does not support CDL. This will not happen
for a control initiated through sysfs, but this code also handles user
passtrhough commands. I will add a patch to correct that.

> 
>> +	    (dev->flags & ATA_DFLAG_CDL_ENABLED))
>> +		buf[4] = 0x02; /* T2A and T2B pages enabled */
>>  	else
>>  		buf[4] = 0;
>>  
>> -- 
>> 2.49.0
>>
>>
> 
> Thanks,
> Igor


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] ata: libata-scsi: Fix ata_msense_control_ata_feature()

[Damien Le Moal]

On 4/18/25 08:50, Igor Pylypiv wrote:
> On Wed, Apr 16, 2025 at 05:42:36PM +0900, Damien Le Moal wrote:
>> For the ATA features subpage of the control mode page, the T10 SAT-6
>> specifications state that:
>>
>> For a MODE SENSE command, the SATL shall return the CDL_CTRL field value
>> that was last set by an application client.
>>
>> However, the function ata_msense_control_ata_feature() always sets the
>> CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and
>> T2B pages. This is thus incorrect and the value 0x02 must be reported
>> only after the user enables the CDL feature, which is indicated with the
>> ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the
>> CDL_CTRL field of the ATA feature subpage of the control mode page must
>> report a value of 0x00.
>>
>> Fix ata_msense_control_ata_feature() to report the correct values for
>> the CDL_CTRL field, according to the enable/disable state of the device
>> CDL feature.
>>
>> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> 
>> ---
>>  drivers/ata/libata-scsi.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 2796c0da8257..e6c652b8a541 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev,
>>  	 */
>>  	put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]);
>>  
>> -	if (dev->flags & ATA_DFLAG_CDL)
>> -		buf[4] = 0x02; /* Support T2A and T2B pages */
>> +	if ((dev->flags & ATA_DFLAG_CDL) &&
> 
> Do we need to check the ATA_DFLAG_CDL flag? If ATA_DFLAG_CDL_ENABLED is set
> then ATA_DFLAG_CDL must be set as well?

In fact, MODE SENSE accesses to the f2h subpage of the control page is already
checked in ata_scsiop_mode_sense() and failed if the device does not have CDL.
So the check here is indeed useless. I dropped it.

> 
> ata_mselect_control_ata_feature() only checks the ATA_DFLAG_CDL_ENABLED flag.

I added a patch to check ATA_DFLAG_CDL for this function as we were not failing
the mode select command for devices that do not support CDL.

> 
>> +	    (dev->flags & ATA_DFLAG_CDL_ENABLED))
>> +		buf[4] = 0x02; /* T2A and T2B pages enabled */
>>  	else
>>  		buf[4] = 0;
>>  
>> -- 
>> 2.49.0
>>
>>
> 
> Thanks,
> Igor


-- 
Damien Le Moal
Western Digital Research