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

[Damien Le Moal <dlemoal@kernel.org>]

On 6/6/24 22:23, Niklas Cassel wrote:
> Hello Damien,
> 
> s/Disabe/Disable/
> in $subject

Good catch. Will fix that.

> 
> 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 inconsistency happen with ATA devices connected to a SAS HBA. This patch is
for such setup only. libata (and libsas) managed ATA CDL devices are fine thanks
to the code you pointed out.

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

No need, that is all fine. Again, this patch is for SAS HBA connected ATA
devices only. I will update the commit message to make this clear.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research