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

[Damien Le Moal <dlemoal@kernel.org>]

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