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

Damien Le Moal
2025-06-12T22:32:59Z

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