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

[Igor Pylypiv <ipylypiv@google.com>]

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
> 
>