Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

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

On 6/18/24 09:02, Igor Pylypiv wrote:
> On Mon, Jun 17, 2024 at 08:25:54AM +0900, Damien Le Moal wrote:
>> On 6/15/24 04:18, Igor Pylypiv wrote:
>>> Do not generate sense data from ATA status/error registers
>>> if valid sense data is already present.
>>
>> This kind of contradicts what you said in patch 2... So I am really confused now.
> 
> Sorry about the confustion. I think the problem is that I was using "sense data"
> to describe two different things:
> #1. SK/ASC/ASCQ
> #2. ATA Status Return sense data descriptor
> 
> Both #1 and #2 need to be populated into sense buffer. The problem with
> the current code is that we can only have either valid #1 or valid #2 but
> not both at the same time.
> 
>> Though this patch actually looks good to me, modulo the comment below.
>> But shouldn't this be squashed with patch 2 ?
> 
> Yes, that's a good point. Let me factor out the sense data descriptor
> population code into a separate function and then squash this patch with
> the patch 2.
> 
>>
>>>
>>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>>> ---
>>>  drivers/ata/libata-scsi.c | 17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index 79e8103ef3a9..4bfe47e7d266 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>>>  	unsigned char *desc = sb + 8;
>>>  	u8 sense_key, asc, ascq;
>>>  
>>> -	/*
>>> -	 * Use ata_to_sense_error() to map status register bits
>>> -	 * onto sense key, asc & ascq.
>>> -	 */
>>> -	if (qc->err_mask ||
>>> -	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
>>> +	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
>>> +		/*
>>> +		 * Do not generate sense data from ATA status/error
>>> +		 * registers if valid sense data is already present.
>>> +		 */
>>
>> The empty "if" here is really horrible. Please revert the condition and add it
>> as a "&&" in the below if.
>>
> Adding the condition to the below if will change the code flow and we'll end
> up executing scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D) when
> ATA_QCFLAG_SENSE_VALID is set, which is not what we want.

I did say "reverse the condition" :)
So that if would be done only if ATA_QCFLAG_SENSE_VALID is *not* set.

> 
> I agree about horrible :)
> 
> Perhaps I should have factored out the descriptor population code into
> a separate function to make the code correct and not so horrible. Let me
> do that in v2.
> 
>>> +	} else if (qc->err_mask ||
>>> +		   tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
>>> +		/*
>>> +		 * Use ata_to_sense_error() to map status register bits
>>> +		 * onto sense key, asc & ascq.
>>> +		 */
>>>  		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
>>>  				   &sense_key, &asc, &ascq);
>>>  		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
>>
> Thank you,
> Igor 
> 

-- 
Damien Le Moal
Western Digital Research