On Thu, Jun 06, 2024 at 03:23:41PM +0200, Niklas Cassel wrote:
> 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?
So what I don't see right now is, the libata code:
val = get_unaligned_le64(&ap->sector_buf[8]);
cdl_enabled = val & BIT_ULL(63) && val & BIT_ULL(21);
if (dev->flags & ATA_DFLAG_CDL_ENABLED) {
if (!cdl_enabled) {
/* Enable CDL on the device */
err_mask = ata_dev_set_feature(dev, SETFEATURES_CDL, 1);
if (err_mask) {
ata_dev_err(dev,
"Enable CDL feature failed\n");
goto not_supported;
}
}
} else {
if (cdl_enabled) {
/* Disable CDL on the device */
err_mask = ata_dev_set_feature(dev, SETFEATURES_CDL, 0);
if (err_mask) {
ata_dev_err(dev,
"Disable CDL feature failed\n");
goto not_supported;
}
}
}
cdl_enabled was from a ata_read_log_page(..., ATA_LOG_CURRENT_SETTINGS, ...)
call, so it should get the value directly from the device, which IIUC,
is enabled by default, so it should be enabled.
ATA_DFLAG_CDL_ENABLED
is from ata_mselect_control_ata_feature(), so translated MODE SELECT,
but no one should have called this at scsi probe time, so I would
expect ATA_DFLAG_CDL_ENABLED to not be set.
Is the problem really when at scsi probe time, or is it when the
user writes the sysfs value the first time where things go wrong?
The commit message mentions "thus avoiding inconsistencies for devices
that have CDL enabled when first scanned", but perhaps you could be
explain in more detail why the current code is not working?
Kind regards,
Niklas
