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

[Niklas Cassel <cassel@kernel.org>]

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