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

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

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