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