Re: [PATCH v3 1/5] ata: libata: rename ata_dev_is_zac()

On 7/3/26 13:59, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The patch introduces an undocumented functional change (implicit bug fix) for Device Managed zoned drives, while the commit message falsely claims the refactor merely replaces identical open-coded tests.
> --
> 
> commit c24879d1eda5cc422a0b0da776fb6aef5fe25e22
> Author: Damien Le Moal <[email protected]>
> 
> ata: libata: rename ata_dev_is_zac()
> 
> The helper function ata_dev_is_zac() checks if a device is a ZAC class
> device (host managed zoned disk) or if it is a host aware zoned disk, that
> is, a regular ATA disk that supports the zoned capabilities. So the name
> of this helper function is confusing as it hints at the first case only.
> 
> Rename this helper function to ata_dev_is_zoned() to avoid confusions and
> better reflect the two cases tested. Use this helper in
> ata_scsiop_inq_std(), ata_scsiop_read_cap() and
> ata_scsi_report_supported_opcodes() instead of having the same tests open
> coded.
> 
> While at it, to stay consistent with this renaming, also rename
> ata_dev_config_zac() to ata_dev_config_zoned().
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index d54ec1631e9a6..a86721b5df7f6 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2020,7 +2020,7 @@ static unsigned int ata_scsiop_inq_std(struct ata_device *dev,
>>  	if (rbuf[32] == 0 || rbuf[32] == ' ')
>>  		memcpy(&rbuf[32], "n/a ", 4);
>>  
>> -	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
>> +	if (ata_dev_is_zoned(dev))
> 
> [Severity: Medium]
> Does this introduce an undocumented functional change for Device Managed
> zoned drives?
> 
> The commit message states this is a replacement of "the same tests open coded",
> but the old condition would evaluate to true for Device Managed drives,
> since ata_id_zoned_cap() returns 0x02 for them.
> 
> The new ata_dev_is_zoned() helper checks ata_id_zoned_cap(dev->id) == 0x01,
> which evaluates to false for Device Managed drives.
> 
> This appears to alter SCSI emulation for these drives, as they will no longer
> report ZBC version compatibility here.
> 
> Could the commit message be updated to mention this functional bug fix?
> 
>>  		memcpy(rbuf + 58, versions_zbc, sizeof(versions_zbc));
>>  	else
>>  		memcpy(rbuf + 58, versions, sizeof(versions));

Correct. But I think that we should never had reported that. In any case, with
host aware also now deprecated, we now definitely do *not* want to report that a
drive managed disk is compatible zbc, as we treat such drives no different than
regular disks.

> 
> [ ... ]
> 
>> @@ -2848,7 +2848,7 @@ static unsigned int ata_scsiop_read_cap(struct ata_device *dev,
>>  	rbuf[10] = sector_size >> (8 * 1);
>>  	rbuf[11] = sector_size;
>>  
>> -	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
>> +	if (ata_dev_is_zoned(dev))
>>  		rbuf[12] = (1 << 4); /* RC_BASIS */
> 
> [Severity: Medium]
> Similarly, does this silently fix Device Managed drives incorrectly
> reporting the RC_BASIS bit in READ CAPACITY 16?

The old code was buggy ! we do not want to set rc basis for drive managed devices.

> 
>>  	rbuf[13] = log2_per_phys;
>>  	rbuf[14] = (lowest_aligned >> 8) & 0x3f;
> 
> [ ... ]
> 
>> @@ -3656,8 +3656,7 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
>>  		break;
>>  	case ZBC_IN:
>>  	case ZBC_OUT:
>> -		if (ata_id_zoned_cap(dev->id) ||
>> -		    dev->class == ATA_DEV_ZAC)
>> +		if (ata_dev_is_zoned(dev))
>>  			supported = 3;
> 
> [Severity: Medium]
> Does this also correctly prevent ZBC_IN and ZBC_OUT from being reported as
> supported for Device Managed drives?

Yes it does and that is fine.

-- 
Damien Le Moal
Western Digital Research