On 1/17/23 15:06, Christoph Hellwig wrote:
>> void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
>> - u8 sk, u8 asc, u8 ascq)
>> + bool check_condition, u8 sk, u8 asc, u8 ascq)
>> {
>> bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
>>
>> if (!cmd)
>> return;
>>
>> - scsi_build_sense(cmd, d_sense, sk, asc, ascq);
>> + scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
>> + if (check_condition)
>> + set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
>> }
>
> Adding a bool parameter do do something conditional at the end
> of a function is always a bad idea. Just split out a
> __ata_scsi_set_sense helper that doesn't set the flag.
What about passing the SAM_STAT_XXX status to set as an argument ?
Most current call site will be modified to pass SAM_STAT_CHECK_CONDITION,
and we could add a wrapper ata_scsi_set_check_condition_sense() to
simplify this most common case.
--
Damien Le Moal
Western Digital Research
