[PATCH v2] scsi: Remember if a device is an ATA device

[PATCH v2] scsi: Remember if a device is an ATA device

[Damien Le Moal]

scsi_add_lun() tests the device vendor string of SCSI devices to detect
if a SCSI device is in fact an ATA device, in order to correctly handle
SATL power management. The function scsi_cdl_enable() also requires
knowing if a SCSI device is an ATA device to control the state of the
device CDL feature but this function does that by testing for the
presence of the VPD page 89h (ATA INFORMATION page).
sd_read_write_same() also has a similar test.

Simplify these different methods by adding the is_ata field to struct
scsi_device to remember that a SCSI device is in fact an ATA one based
on the device vendor name test. This filed can also allow low level
SCSI host adapter drivers to take special actions for ATA devices
(e.g. to better handle ATA NCQ errors).

With this, simplify scsi_cdl_enable() and sd_read_write_same().

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
Changes from v1:
 - Add simplification of sd_read_write_same()

 drivers/scsi/scsi.c        |  7 +------
 drivers/scsi/scsi_scan.c   |  3 ++-
 drivers/scsi/sd.c          | 13 ++++---------
 include/scsi/scsi_device.h |  5 +++++
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 518a252eb6aa..534310224e8f 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -708,20 +708,15 @@ void scsi_cdl_check(struct scsi_device *sdev)
 int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
 {
 	char buf[64];
-	bool is_ata;
 	int ret;
 
 	if (!sdev->cdl_supported)
 		return -EOPNOTSUPP;
 
-	rcu_read_lock();
-	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) {
+	if (sdev->is_ata) {
 		struct scsi_mode_data data;
 		struct scsi_sense_hdr sshdr;
 		char *buf_data;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 4833b8fe251b..160c2f74c7e7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -909,7 +909,8 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	sdev->model = (char *) (sdev->inquiry + 16);
 	sdev->rev = (char *) (sdev->inquiry + 32);
 
-	if (strncmp(sdev->vendor, "ATA     ", 8) == 0) {
+	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
+	if (sdev->is_ata) {
 		/*
 		 * sata emulation layer device.  This is a hack to work around
 		 * the SATL power management specifications which state that
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3f6e87705b62..daddef2e9e87 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3459,19 +3459,14 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
 	}
 
 	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, INQUIRY, 0) < 0) {
-		struct scsi_vpd *vpd;
-
 		sdev->no_report_opcodes = 1;
 
-		/* Disable WRITE SAME if REPORT SUPPORTED OPERATION
-		 * CODES is unsupported and the device has an ATA
-		 * Information VPD page (SAT).
+		/*
+		 * Disable WRITE SAME if REPORT SUPPORTED OPERATION CODES is
+		 * unsupported and this is an ATA device.
 		 */
-		rcu_read_lock();
-		vpd = rcu_dereference(sdev->vpd_pg89);
-		if (vpd)
+		if (sdev->is_ata)
 			sdev->no_write_same = 1;
-		rcu_read_unlock();
 	}
 
 	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16, 0) == 1)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 68dd49947d04..6d6500148c4b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -184,6 +184,11 @@ struct scsi_device {
 	 */
 	unsigned force_runtime_start_on_system_start:1;
 
+	/*
+	 * Set if the device is an ATA device.
+	 */
+	unsigned is_ata:1;
+
 	unsigned removable:1;
 	unsigned changed:1;	/* Data invalid due to media change */
 	unsigned busy:1;	/* Used to prevent races */
-- 
2.49.0

Re: [PATCH v2] scsi: Remember if a device is an ATA device

[Bart Van Assche]

On 6/11/25 2:34 AM, Damien Le Moal wrote:
> scsi_add_lun() tests the device vendor string of SCSI devices to detect
> if a SCSI device is in fact an ATA device, in order to correctly handle
> SATL power management. The function scsi_cdl_enable() also requires
> knowing if a SCSI device is an ATA device to control the state of the
> device CDL feature but this function does that by testing for the
> presence of the VPD page 89h (ATA INFORMATION page).
> sd_read_write_same() also has a similar test.
> 
> Simplify these different methods by adding the is_ata field to struct
> scsi_device to remember that a SCSI device is in fact an ATA one based
> on the device vendor name test. This filed can also allow low level
> SCSI host adapter drivers to take special actions for ATA devices
> (e.g. to better handle ATA NCQ errors).
> 
> With this, simplify scsi_cdl_enable() and sd_read_write_same().
Hi Damien,

There is only one "if (is_ata)" check in the SCSI core as far as I can
see. Can it be avoided that ATA code leaks into the SCSI core by
introducing a new function pointer, e.g. in struct Scsi_Host, and by
calling that new function pointer if it has been set from
scsi_cdl_enable()?

Thanks,

Bart.

Re: [PATCH v2] scsi: Remember if a device is an ATA device

[Damien Le Moal]

On 6/12/25 00:46, Bart Van Assche wrote:
> On 6/11/25 2:34 AM, Damien Le Moal wrote:
>> scsi_add_lun() tests the device vendor string of SCSI devices to detect
>> if a SCSI device is in fact an ATA device, in order to correctly handle
>> SATL power management. The function scsi_cdl_enable() also requires
>> knowing if a SCSI device is an ATA device to control the state of the
>> device CDL feature but this function does that by testing for the
>> presence of the VPD page 89h (ATA INFORMATION page).
>> sd_read_write_same() also has a similar test.
>>
>> Simplify these different methods by adding the is_ata field to struct
>> scsi_device to remember that a SCSI device is in fact an ATA one based
>> on the device vendor name test. This filed can also allow low level
>> SCSI host adapter drivers to take special actions for ATA devices
>> (e.g. to better handle ATA NCQ errors).
>>
>> With this, simplify scsi_cdl_enable() and sd_read_write_same().
> Hi Damien,
> 
> There is only one "if (is_ata)" check in the SCSI core as far as I can
> see. Can it be avoided that ATA code leaks into the SCSI core by
> introducing a new function pointer, e.g. in struct Scsi_Host, and by
> calling that new function pointer if it has been set from
> scsi_cdl_enable()?

You are off by 2 on the count:

git grep "\->is_ata" drivers/scsi/*

drivers/scsi/scsi.c:    if (sdev->is_ata) {
drivers/scsi/scsi_scan.c:       if (sdev->is_ata) {
drivers/scsi/sd.c:              if (sdev->is_ata)

And no, we cannot avoid trying to detect if we are dealing with an ATA/SAT
device or a real SCSI device in all 3 places where this "if" is done.
2 of these used VPD page dereference under rcu lock before, which is rather
heavy handed. The point of this patch is to simplify that.

> 
> Thanks,
> 
> Bart.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] scsi: Remember if a device is an ATA device

[Igor Pylypiv]

On Wed, Jun 11, 2025 at 06:34:21PM +0900, Damien Le Moal wrote:
> scsi_add_lun() tests the device vendor string of SCSI devices to detect
> if a SCSI device is in fact an ATA device, in order to correctly handle
> SATL power management. The function scsi_cdl_enable() also requires
> knowing if a SCSI device is an ATA device to control the state of the
> device CDL feature but this function does that by testing for the
> presence of the VPD page 89h (ATA INFORMATION page).
> sd_read_write_same() also has a similar test.
> 
> Simplify these different methods by adding the is_ata field to struct
> scsi_device to remember that a SCSI device is in fact an ATA one based
> on the device vendor name test. This filed can also allow low level
> SCSI host adapter drivers to take special actions for ATA devices
> (e.g. to better handle ATA NCQ errors).
> 
> With this, simplify scsi_cdl_enable() and sd_read_write_same().
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Thanks,
Igor

Re: [PATCH v2] scsi: Remember if a device is an ATA device

[Bart Van Assche]

On 6/11/25 4:14 PM, Damien Le Moal wrote:
> And no, we cannot avoid trying to detect if we are dealing with an ATA/SAT
> device or a real SCSI device in all 3 places where this "if" is done.
> 2 of these used VPD page dereference under rcu lock before, which is rather
> heavy handed. The point of this patch is to simplify that.

Hi Damien,

The code under "if (is_ata)" in scsi_cdl_enable() seems very 
ATA-specific to me. Shouldn't that code be moved under drivers/ata/?

Regarding the following code in scsi_add_lun():

  	if (strncmp(sdev->vendor, "ATA     ", 8) == 0)
		sdev->allow_restart = 1;

Other SCSI LLDs set the 'allow_restart' flag in their sdev_configure
callback. Can this be done for ATA disks too?

Regarding the code that sets the no_write_same flag:

	if (sdev->is_ata)
		sdev->no_write_same = 1;

Why is the RCU reader lock held around that code, a lock that protects
reading from the VPD pages while the above code does not access the
contents of any VPD page?

Other SCSI LLDs set the 'no_write_same' flag in their sdev_configure
callback. Can this be done for ATA disks too?

Thanks,

Bart.

Re: [PATCH v2] scsi: Remember if a device is an ATA device

[Damien Le Moal]

On 6/13/25 01:13, Bart Van Assche wrote:
> On 6/11/25 4:14 PM, Damien Le Moal wrote:
>> And no, we cannot avoid trying to detect if we are dealing with an ATA/SAT
>> device or a real SCSI device in all 3 places where this "if" is done.
>> 2 of these used VPD page dereference under rcu lock before, which is rather
>> heavy handed. The point of this patch is to simplify that.
> 
> Hi Damien,
> 
> The code under "if (is_ata)" in scsi_cdl_enable() seems very 
> ATA-specific to me. Shouldn't that code be moved under drivers/ata/?

You seem to be forgetting that when ATA devices are attached to a SAS HBA (one
that does not use libsas), then the code in drivers/ata is irrelevant as it is
not used at all.

All these "if (is_ata)" cases are addressing shortcomings of the SAT
specifications regarding incompatibilities between SPC/SBC and ACS
specifications. E.g., for CDL, the CDL feature has an enable/disable action
defined in ACS but not in SPC (CDL is always enabled if it is supported).

> 
> Regarding the following code in scsi_add_lun():
> 
>   	if (strncmp(sdev->vendor, "ATA     ", 8) == 0)
> 		sdev->allow_restart = 1;
> 
> Other SCSI LLDs set the 'allow_restart' flag in their sdev_configure
> callback. Can this be done for ATA disks too?
> 
> Regarding the code that sets the no_write_same flag:
> 
> 	if (sdev->is_ata)
> 		sdev->no_write_same = 1;
> 
> Why is the RCU reader lock held around that code, a lock that protects
> reading from the VPD pages while the above code does not access the
> contents of any VPD page?
> 
> Other SCSI LLDs set the 'no_write_same' flag in their sdev_configure
> callback. Can this be done for ATA disks too?

See above. If this is moved to ATA code, then it will never be done for ATA
drives connected to SAS HBAs.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] scsi: Remember if a device is an ATA device

[Martin K. Petersen]

Damien,

> Simplify these different methods by adding the is_ata field to struct
> scsi_device to remember that a SCSI device is in fact an ATA one based
> on the device vendor name test. This filed can also allow low level
> SCSI host adapter drivers to take special actions for ATA devices
> (e.g. to better handle ATA NCQ errors).

Applied to 6.17/scsi-staging, thanks!

-- 
Martin K. Petersen

Re: [PATCH v2] scsi: Remember if a device is an ATA device

[Martin K. Petersen]

On Wed, 11 Jun 2025 18:34:21 +0900, Damien Le Moal wrote:

> scsi_add_lun() tests the device vendor string of SCSI devices to detect
> if a SCSI device is in fact an ATA device, in order to correctly handle
> SATL power management. The function scsi_cdl_enable() also requires
> knowing if a SCSI device is an ATA device to control the state of the
> device CDL feature but this function does that by testing for the
> presence of the VPD page 89h (ATA INFORMATION page).
> sd_read_write_same() also has a similar test.
> 
> [...]

Applied to 6.17/scsi-queue, thanks!

[1/1] scsi: Remember if a device is an ATA device
      https://git.kernel.org/mkp/scsi/c/b1ba03c49a71

-- 
Martin K. Petersen	Oracle Linux Engineering