ata: libata-scsi: Fix CDL control

[PATCH v2] ata: libata-scsi: Fix CDL control

Posted by Igor Pylypiv 1 month, 3 weeks ago

Delete extra checks for the ATA_DFLAG_CDL_ENABLED flag that prevent
SET FEATURES command from being issued to a drive when NCQ commands
are active.

ata_mselect_control_ata_feature() sets / clears the ATA_DFLAG_CDL_ENABLED
flag during the translation of MODE SELECT to SET FEATURES. If SET FEATURES
gets deferred due to outstanding NCQ commands, the original MODE SELECT
command will be re-queued. When the re-queued MODE SELECT goes through
the ata_mselect_control_ata_feature() translation again, SET FEATURES
will not be issued because ATA_DFLAG_CDL_ENABLED has been already set or
cleared by the initial translation of MODE SELECT.

The ATA_DFLAG_CDL_ENABLED checks in ata_mselect_control_ata_feature()
are safe to remove because scsi_cdl_enable() implements a similar logic
that avoids enabling CDL if it has been enabled already.

Fixes: 17e897a45675 ("ata: libata-scsi: Improve CDL control")
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---

Changes from v1:
- Changed the patch from revert to fixup.
- Restored debug logs and the comment about mutual exclusivity with
  NCQ priority.
- Dropped cc to stable and added fixes tag instead.

 drivers/ata/libata-scsi.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 57f674f51b0c..2ded5e476d6e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3904,21 +3904,16 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
 	/* Check cdl_ctrl */
 	switch (buf[0] & 0x03) {
 	case 0:
-		/* Disable CDL if it is enabled */
-		if (!(dev->flags & ATA_DFLAG_CDL_ENABLED))
-			return 0;
+		/* Disable CDL */
 		ata_dev_dbg(dev, "Disabling CDL\n");
 		cdl_action = 0;
 		dev->flags &= ~ATA_DFLAG_CDL_ENABLED;
 		break;
 	case 0x02:
 		/*
-		 * Enable CDL if not already enabled. Since this is mutually
-		 * exclusive with NCQ priority, allow this only if NCQ priority
-		 * is disabled.
+		 * Enable CDL. Since CDL 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");
-- 
2.51.0.rc0.215.g125493bb4a-goog

Re: [PATCH v2] ata: libata-scsi: Fix CDL control

Posted by Damien Le Moal 1 month, 3 weeks ago

On 8/14/25 11:22 AM, Igor Pylypiv wrote:
> Delete extra checks for the ATA_DFLAG_CDL_ENABLED flag that prevent
> SET FEATURES command from being issued to a drive when NCQ commands
> are active.
> 
> ata_mselect_control_ata_feature() sets / clears the ATA_DFLAG_CDL_ENABLED
> flag during the translation of MODE SELECT to SET FEATURES. If SET FEATURES
> gets deferred due to outstanding NCQ commands, the original MODE SELECT
> command will be re-queued. When the re-queued MODE SELECT goes through
> the ata_mselect_control_ata_feature() translation again, SET FEATURES
> will not be issued because ATA_DFLAG_CDL_ENABLED has been already set or
> cleared by the initial translation of MODE SELECT.
> 
> The ATA_DFLAG_CDL_ENABLED checks in ata_mselect_control_ata_feature()
> are safe to remove because scsi_cdl_enable() implements a similar logic
> that avoids enabling CDL if it has been enabled already.
> 
> Fixes: 17e897a45675 ("ata: libata-scsi: Improve CDL control")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

Applied to for-6.17-fixes. Thanks !
(Note: I added Cc: stable@vger.kernel.org)

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: libata-scsi: Fix CDL control

Posted by Niklas Cassel 1 month, 3 weeks ago

On Wed, Aug 13, 2025 at 07:22:56PM -0700, Igor Pylypiv wrote:
> Delete extra checks for the ATA_DFLAG_CDL_ENABLED flag that prevent
> SET FEATURES command from being issued to a drive when NCQ commands
> are active.
> 
> ata_mselect_control_ata_feature() sets / clears the ATA_DFLAG_CDL_ENABLED
> flag during the translation of MODE SELECT to SET FEATURES. If SET FEATURES
> gets deferred due to outstanding NCQ commands, the original MODE SELECT
> command will be re-queued. When the re-queued MODE SELECT goes through
> the ata_mselect_control_ata_feature() translation again, SET FEATURES
> will not be issued because ATA_DFLAG_CDL_ENABLED has been already set or
> cleared by the initial translation of MODE SELECT.
> 
> The ATA_DFLAG_CDL_ENABLED checks in ata_mselect_control_ata_feature()
> are safe to remove because scsi_cdl_enable() implements a similar logic
> that avoids enabling CDL if it has been enabled already.
> 
> Fixes: 17e897a45675 ("ata: libata-scsi: Improve CDL control")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

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

However, like Damien said, we really need to improve the API so that the
SAT can know if the command was success or failure.

That way we could set the flag only after the command was successful.


Kind regards,
Niklas

Re: [PATCH v2] ata: libata-scsi: Fix CDL control

Posted by Damien Le Moal 1 month, 3 weeks ago

On 8/14/25 11:22 AM, Igor Pylypiv wrote:
> Delete extra checks for the ATA_DFLAG_CDL_ENABLED flag that prevent
> SET FEATURES command from being issued to a drive when NCQ commands
> are active.
> 
> ata_mselect_control_ata_feature() sets / clears the ATA_DFLAG_CDL_ENABLED
> flag during the translation of MODE SELECT to SET FEATURES. If SET FEATURES
> gets deferred due to outstanding NCQ commands, the original MODE SELECT
> command will be re-queued. When the re-queued MODE SELECT goes through
> the ata_mselect_control_ata_feature() translation again, SET FEATURES
> will not be issued because ATA_DFLAG_CDL_ENABLED has been already set or
> cleared by the initial translation of MODE SELECT.
> 
> The ATA_DFLAG_CDL_ENABLED checks in ata_mselect_control_ata_feature()
> are safe to remove because scsi_cdl_enable() implements a similar logic
> that avoids enabling CDL if it has been enabled already.
> 
> Fixes: 17e897a45675 ("ata: libata-scsi: Improve CDL control")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

Thanks. This looks good.

Of note though, is that this is all due to the fact that we set or clear the
flag irrespective of the result of the SET_FEATURE command, which may actually
fail... But we do not have a simple way to wait for that command to complete
first before manipulating the flag. This will need some bigger fixes later :)

> ---
> 
> Changes from v1:
> - Changed the patch from revert to fixup.
> - Restored debug logs and the comment about mutual exclusivity with
>   NCQ priority.
> - Dropped cc to stable and added fixes tag instead.
> 
>  drivers/ata/libata-scsi.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 57f674f51b0c..2ded5e476d6e 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3904,21 +3904,16 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
>  	/* Check cdl_ctrl */
>  	switch (buf[0] & 0x03) {
>  	case 0:
> -		/* Disable CDL if it is enabled */
> -		if (!(dev->flags & ATA_DFLAG_CDL_ENABLED))
> -			return 0;
> +		/* Disable CDL */
>  		ata_dev_dbg(dev, "Disabling CDL\n");
>  		cdl_action = 0;
>  		dev->flags &= ~ATA_DFLAG_CDL_ENABLED;
>  		break;
>  	case 0x02:
>  		/*
> -		 * Enable CDL if not already enabled. Since this is mutually
> -		 * exclusive with NCQ priority, allow this only if NCQ priority
> -		 * is disabled.
> +		 * Enable CDL. Since CDL 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");


-- 
Damien Le Moal
Western Digital Research