| Message ID | 20250416084238.258169-4-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] ata: libata-scsi: Fix ata_msense_control_ata_feature() | expand |
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@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > 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 > >
On Thu, Apr 17, 2025 at 08:08:00PM +0900, Damien Le Moal wrote: > On 4/17/25 12:37, Igor Pylypiv wrote: > > 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? > > Yes, that is unchanged so that we keep being consistent with the fact that the > scsi layer starts with the sysfs cdl_enabled attribute set to false. So if an > ATA device starts with CDL enabled, it will be disabled. > > That does not cause any issue with the CDL statistics log page because that page > is not persistent and cleared) on power-on-reset events and this change has no > effect on that. The CDL statistics log page will always be cleared on boot/reboot. > Sounds good. Thank you for confirming, Damien! Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > > -- > Damien Le Moal > Western Digital Research
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;
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. 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@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/scsi/scsi.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-)