| Message ID | 20240606054606.55624-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: core: Disabe CDL by default | expand |
On Thu, Jun 06, 2024 at 02:46:06PM +0900, Damien Le Moal wrote: > 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. The same should be true for native SCSI as well, right? The patch itself looks good, though: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 6/6/24 18:32, Christoph Hellwig wrote: > On Thu, Jun 06, 2024 at 02:46:06PM +0900, Damien Le Moal wrote: >> 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. > > The same should be true for native SCSI as well, right? Nope, because SPC does not define a knob to turn CDL on/off on the device. For SAS drives, if CDL is supported, it is always enabled/usable. This is why scsi_enable_cdl() is reduced to setting sdev->cdl_enable only (and also why there that ugly "if (is_ata)". > > The patch itself looks good, though: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks.
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
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@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? 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
On 6/5/24 23:46, 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. Do we really need to disable CDL by default? Has it been considered to enable CDL by default if the Command Duration Limit mode pages that have been defined in SPC-5 are supported? Thanks, Bart.
On 6/7/24 00:48, Bart Van Assche wrote: > On 6/5/24 23:46, 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. > > Do we really need to disable CDL by default? Has it been considered to > enable CDL by default if the Command Duration Limit mode pages that have > been defined in SPC-5 are supported? We cannot do that as that would potentially break users of ATA NCQ priority. The reason is that for ATA drives that support both NCQ priority and CDL, if CDL is enabled, NCQ priority cannot be used. If we were to change CDL to be enabled by default, NCQ priority users would need to disable it before enabling NCQ priority. That can break existing setups. And this has been like this since kernel 6.5, so I am not going to change this now.
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
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; }
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(+)