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