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

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));

[ ... ]

> @@ -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?

>  	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?

If these are intentional bug fixes rather than just refactoring, should the
commit message clarify that the previous open-coded tests incorrectly matched
Device Managed drives?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1