On 09/20/2018 05:22 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)) {
> + qc_active |= (qc_active & 0x01) << ATA_TAG_INTERNAL;
> + qc_active ^= qc_active & 0x01;
Sorry for missing it earlier... but why 0x01, not 0x1 or even 1? I don't think
a middle 0 provides any more clarity. Looks good otherwise. :-)
> + }
> +
> + done_mask = ap_qc_active ^ qc_active;
>
> if (unlikely(done_mask & qc_active)) {
> ata_port_err(ap, "illegal qc_active transition (%08llx->%08llx)\n",
>
MBR, Sergei
