On Wed, Apr 16, 2025 at 05:42:38PM +0900, Damien Le Moal wrote: > With ATA devices supporting the CDL feature, using CDL requires that the > feature be enabled with a SET FEATURES command. This command is issued > as the translated command for the MODE SELECT command issued by > scsi_cdl_enable() when the user enables CDL through the device > cdl_enable sysfs attribute. > > However, the implementation of scsi_cdl_enable() always issues a MODE > SELECT command for ATA devices when the enable argument is true, even if > CDL is already enabled on the device. While this does not cause any > issue with using CDL descriptors with read/write commands (the CDL > feature will be enabled on the drive), issuing the MODE SELECT command > even when the device CDL feature is already enabled will cause a reset > of the ATA device CDL statistics log page (as defined in ACS, any CDL > enable action must reset the device statistics). > > Avoid this needless actions (and the implied statistics log page reset) > by modifying scsi_cdl_enable() to issue the MODE SELECT command to > enable CDL if and only if CDL is not reported as already enabled on the > device. Hi Damien, What happens when a drive spins up with CDL enabled? Last year you sent a patch to make sure that CDL gets disabled by default. Is that still the case? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=52912ca87e2b810e5acdcdc452593d30c9187d8f Thanks, Igor > > And while at it, simplify the initialization of the is_ata boolean > variable and move the declaration of the scsi mode data and sense header > variables to within the scope of ATA device handling. > > 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 | 36 ++++++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index 53daf923ad8e..518a252eb6aa 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -707,26 +707,23 @@ void scsi_cdl_check(struct scsi_device *sdev) > */ > int scsi_cdl_enable(struct scsi_device *sdev, bool enable) > { > - struct scsi_mode_data data; > - struct scsi_sense_hdr sshdr; > - struct scsi_vpd *vpd; > - bool is_ata = false; > char buf[64]; > + bool is_ata; > int ret; > > if (!sdev->cdl_supported) > return -EOPNOTSUPP; > > rcu_read_lock(); > - vpd = rcu_dereference(sdev->vpd_pg89); > - if (vpd) > - is_ata = true; > + is_ata = rcu_dereference(sdev->vpd_pg89); > rcu_read_unlock(); > > /* > * For ATA devices, CDL needs to be enabled with a SET FEATURES command. > */ > if (is_ata) { > + struct scsi_mode_data data; > + struct scsi_sense_hdr sshdr; > char *buf_data; > int len; > > @@ -735,16 +732,30 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable) > if (ret) > return -EINVAL; > > - /* Enable CDL using the ATA feature page */ > + /* Enable or disable CDL using the ATA feature page */ > len = min_t(size_t, sizeof(buf), > data.length - data.header_length - > data.block_descriptor_length); > buf_data = buf + data.header_length + > data.block_descriptor_length; > - if (enable) > - buf_data[4] = 0x02; > - else > - buf_data[4] = 0; > + > + /* > + * If we want to enable CDL and CDL is already enabled on the > + * device, do nothing. This avoids needlessly resetting the CDL > + * statistics on the device as that is implied by the CDL enable > + * action. Similar to this, there is no need to do anything if > + * we want to disable CDL and CDL is already disabled. > + */ > + if (enable) { > + if ((buf_data[4] & 0x03) == 0x02) > + goto out; > + buf_data[4] &= ~0x03; > + buf_data[4] |= 0x02; > + } else { > + if ((buf_data[4] & 0x03) == 0x00) > + goto out; > + buf_data[4] &= ~0x03; > + } > > ret = scsi_mode_select(sdev, 1, 0, buf_data, len, 5 * HZ, 3, > &data, &sshdr); > @@ -756,6 +767,7 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable) > } > } > > +out: > sdev->cdl_enable = enable; > > return 0; > -- > 2.49.0 > >