Re: [PATCH v1 4/6] ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands support

On 7/2/26 11:07, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] ZBC_IN is incorrectly evaluated as not having service actions because ZI_REPORT_ZONES equals 0, breaking all REPORT SUPPORTED OPERATION CODES (RSOC) formats.
> --
> 
> commit 6af9854276b3609a524ea350c87e6e2924aee340
> Author: Damien Le Moal <[email protected]>
> 
> ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands support
> 
> Improve ata_scsi_cmd_is_supported() to check the service action for the
> ZBC_IN and ZBC_OUT opcodes, to correctly report supported zone management
> commands for zoned disks.
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index e432e521ad318..f9e6bda6833e4 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3708,8 +3708,17 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
>>  		}
>>  		return true;
>>  	case ZBC_IN:
>> +		return sa == ZI_REPORT_ZONES && ata_dev_is_zoned(dev);
> 
> [Severity: Medium]
> Does this inadvertently break REPORT SUPPORTED OPERATION CODES (RSOC) formats
> for ZBC_IN?
> 
> Because ZI_REPORT_ZONES evaluates to 0, the helper
> ata_scsi_supported_cmd_has_sa() will evaluate this as not having service
> actions:
> 
> drivers/ata/libata-scsi.c:ata_scsi_supported_cmd_has_sa() {
>     return cmd->sa != 0;
> }
> 
> This appears to cause incorrect behavior during RSOC handling for ZBC_IN:
> 
> - Format 0 (ata_scsi_report_all_supported_opcodes) fails to set the SERVACTV
>   (Service Action Valid) bit because cmd->sa evaluates to 0.
> 
> - Format 1 incorrectly accepts ZBC_IN.
> 
> - Format 2 (ata_scsi_report_supported_opcodes) incorrectly rejects valid
>   queries for ZBC_IN with CHECK CONDITION (ILLEGAL REQUEST), preventing
>   userspace from probing ZBC_IN service actions.

Yes, these observations are correct if one assumes that "0" is a valid service
action. But it is not: the SPC specifications actually define service action "0"
as "reserved". So this is a mess, but the specifications are responsible here.

With these changes, report zones is still reported as supported and everything
works.

-- 
Damien Le Moal
Western Digital Research