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@xxxxxxx>
> Fixes: 1b22cfb14142 ("scsi: core: Allow enabling and disabling command duration limits")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> 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
