On 9/20/18 10:44 AM, Sergei Shtylyov wrote:
> On 09/20/2018 06:15 PM, Jens Axboe wrote:
>
>>>> When we're comparing the hardware completion mask passed in from the
>>>> driver with the internal tag pending mask, we need to account for the
>>>> fact that the internal tag is different from the hardware tag. If not,
>>>> then we can end up either prematurely completing the internal tag (since
>>>> it's not set in the hw mask), or simply flag an error:
>>>>
>>>> ata2: illegal qc_active transition (100000000->00000001)
>>>>
>>>> If the internal tag is set, then swap that with the hardware tag in this
>>>> case before comparing with what the hardware reports.
>>>>
>>>> Fixes: 28361c403683 ("libata: add extra internal command")
>>>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=201151
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Reported-by: Paul Sbarra <sbarra.paul@xxxxxxxxx>
>>>> Tested-by: Paul Sbarra <sbarra.paul@xxxxxxxxx>
>>>> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>>>>
>>>> ---
>>>>
>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>> index 599e01bcdef2..a9dd4ea7467d 100644
>>>> --- a/drivers/ata/libata-core.c
>>>> +++ b/drivers/ata/libata-core.c
>>>> @@ -5359,10 +5359,20 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
>>>> */
>>>> int ata_qc_complete_multiple(struct ata_port *ap, u64 qc_active)
>>>> {
>>>> + u64 done_mask, ap_qc_active = ap->qc_active;
>>>> int nr_done = 0;
>>>> - u64 done_mask;
>>>>
>>>> - done_mask = ap->qc_active ^ qc_active;
>>>> + /*
>>>> + * If the internal tag is set on ap->qc_active, then we care about
>>>> + * bit0 on the passed in qc_active mask. Move that bit up to match
>>>> + * the internal tag.
>>>> + */
>>>> + if (ap_qc_active & (1ULL << ATA_TAG_INTERNAL)) {
>>>
>>> Maybe BIT_ULL(ATA_TAG_INTERNAL)?
>>
>> Honestly, I had defines like that, since they hide what's going on, whereas
>
> s/had/hate/? :-)
Doh yes, hate of course :-)
On the 0x1 vs 0x01, can really go both ways on that. I don't feel strongly
about that at all.
--
Jens Axboe
