| Message ID | 20250416084238.258169-2-dlemoal@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CDL Feature control improvements | expand |
On Wed, Apr 16, 2025 at 05:42:36PM +0900, Damien Le Moal wrote: > For the ATA features subpage of the control mode page, the T10 SAT-6 > specifications state that: > > For a MODE SENSE command, the SATL shall return the CDL_CTRL field value > that was last set by an application client. > > However, the function ata_msense_control_ata_feature() always sets the > CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and > T2B pages. This is thus incorrect and the value 0x02 must be reported > only after the user enables the CDL feature, which is indicated with the > ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the > CDL_CTRL field of the ATA feature subpage of the control mode page must > report a value of 0x00. > > Fix ata_msense_control_ata_feature() to report the correct values for > the CDL_CTRL field, according to the enable/disable state of the device > CDL feature. > > Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-scsi.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 2796c0da8257..e6c652b8a541 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev, > */ > put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]); > > - if (dev->flags & ATA_DFLAG_CDL) > - buf[4] = 0x02; /* Support T2A and T2B pages */ > + if ((dev->flags & ATA_DFLAG_CDL) && Do we need to check the ATA_DFLAG_CDL flag? If ATA_DFLAG_CDL_ENABLED is set then ATA_DFLAG_CDL must be set as well? ata_mselect_control_ata_feature() only checks the ATA_DFLAG_CDL_ENABLED flag. > + (dev->flags & ATA_DFLAG_CDL_ENABLED)) > + buf[4] = 0x02; /* T2A and T2B pages enabled */ > else > buf[4] = 0; > > -- > 2.49.0 > > Thanks, Igor
On 4/18/25 08:50, Igor Pylypiv wrote: > On Wed, Apr 16, 2025 at 05:42:36PM +0900, Damien Le Moal wrote: >> For the ATA features subpage of the control mode page, the T10 SAT-6 >> specifications state that: >> >> For a MODE SENSE command, the SATL shall return the CDL_CTRL field value >> that was last set by an application client. >> >> However, the function ata_msense_control_ata_feature() always sets the >> CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and >> T2B pages. This is thus incorrect and the value 0x02 must be reported >> only after the user enables the CDL feature, which is indicated with the >> ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the >> CDL_CTRL field of the ATA feature subpage of the control mode page must >> report a value of 0x00. >> >> Fix ata_msense_control_ata_feature() to report the correct values for >> the CDL_CTRL field, according to the enable/disable state of the device >> CDL feature. >> >> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") >> Cc: stable@vger.kernel.org >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > > Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > >> --- >> drivers/ata/libata-scsi.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 2796c0da8257..e6c652b8a541 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev, >> */ >> put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]); >> >> - if (dev->flags & ATA_DFLAG_CDL) >> - buf[4] = 0x02; /* Support T2A and T2B pages */ >> + if ((dev->flags & ATA_DFLAG_CDL) && > > Do we need to check the ATA_DFLAG_CDL flag? If ATA_DFLAG_CDL_ENABLED is set > then ATA_DFLAG_CDL must be set as well? Yes, we can remove it. It does not hurt though. > ata_mselect_control_ata_feature() only checks the ATA_DFLAG_CDL_ENABLED flag. Indeed, but that actually is wrong I think since we should not attempt to issue the SET FEATURES command if the drive does not support CDL. This will not happen for a control initiated through sysfs, but this code also handles user passtrhough commands. I will add a patch to correct that. > >> + (dev->flags & ATA_DFLAG_CDL_ENABLED)) >> + buf[4] = 0x02; /* T2A and T2B pages enabled */ >> else >> buf[4] = 0; >> >> -- >> 2.49.0 >> >> > > Thanks, > Igor
On 4/18/25 08:50, Igor Pylypiv wrote: > On Wed, Apr 16, 2025 at 05:42:36PM +0900, Damien Le Moal wrote: >> For the ATA features subpage of the control mode page, the T10 SAT-6 >> specifications state that: >> >> For a MODE SENSE command, the SATL shall return the CDL_CTRL field value >> that was last set by an application client. >> >> However, the function ata_msense_control_ata_feature() always sets the >> CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and >> T2B pages. This is thus incorrect and the value 0x02 must be reported >> only after the user enables the CDL feature, which is indicated with the >> ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the >> CDL_CTRL field of the ATA feature subpage of the control mode page must >> report a value of 0x00. >> >> Fix ata_msense_control_ata_feature() to report the correct values for >> the CDL_CTRL field, according to the enable/disable state of the device >> CDL feature. >> >> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") >> Cc: stable@vger.kernel.org >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > > Reviewed-by: Igor Pylypiv <ipylypiv@google.com> > >> --- >> drivers/ata/libata-scsi.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 2796c0da8257..e6c652b8a541 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev, >> */ >> put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]); >> >> - if (dev->flags & ATA_DFLAG_CDL) >> - buf[4] = 0x02; /* Support T2A and T2B pages */ >> + if ((dev->flags & ATA_DFLAG_CDL) && > > Do we need to check the ATA_DFLAG_CDL flag? If ATA_DFLAG_CDL_ENABLED is set > then ATA_DFLAG_CDL must be set as well? In fact, MODE SENSE accesses to the f2h subpage of the control page is already checked in ata_scsiop_mode_sense() and failed if the device does not have CDL. So the check here is indeed useless. I dropped it. > > ata_mselect_control_ata_feature() only checks the ATA_DFLAG_CDL_ENABLED flag. I added a patch to check ATA_DFLAG_CDL for this function as we were not failing the mode select command for devices that do not support CDL. > >> + (dev->flags & ATA_DFLAG_CDL_ENABLED)) >> + buf[4] = 0x02; /* T2A and T2B pages enabled */ >> else >> buf[4] = 0; >> >> -- >> 2.49.0 >> >> > > Thanks, > Igor
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 2796c0da8257..e6c652b8a541 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2453,8 +2453,9 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev, */ put_unaligned_be16(ATA_FEATURE_SUB_MPAGE_LEN - 4, &buf[2]); - if (dev->flags & ATA_DFLAG_CDL) - buf[4] = 0x02; /* Support T2A and T2B pages */ + if ((dev->flags & ATA_DFLAG_CDL) && + (dev->flags & ATA_DFLAG_CDL_ENABLED)) + buf[4] = 0x02; /* T2A and T2B pages enabled */ else buf[4] = 0;
For the ATA features subpage of the control mode page, the T10 SAT-6 specifications state that: For a MODE SENSE command, the SATL shall return the CDL_CTRL field value that was last set by an application client. However, the function ata_msense_control_ata_feature() always sets the CDL_CTRL field to the 0x02 value to indicate support for the CDL T2A and T2B pages. This is thus incorrect and the value 0x02 must be reported only after the user enables the CDL feature, which is indicated with the ATA_DFLAG_CDL_ENABLED device flag. When this flag is not set, the CDL_CTRL field of the ATA feature subpage of the control mode page must report a value of 0x00. Fix ata_msense_control_ata_feature() to report the correct values for the CDL_CTRL field, according to the enable/disable state of the device CDL feature. Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-scsi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)