On Monday 06 January 2020 09:16:05 Sascha Hauer wrote:

On Wed, Dec 25, 2019 at 07:18:40PM +0100, Pali Rohár wrote:

quoted

Hello Sascha!

On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:

quoted

ata_qc_complete_multiple() is called with a mask of the still active
tags.

mv_sata doesn't have this information directly and instead calculates
the still active tags from the started tags (ap->qc_active) and the
finished tags as (ap->qc_active ^ done_mask)

Since 28361c40368 the hw_tag and tag are no longer the same and the
equation is no longer valid. In ata_exec_internal_sg() ap->qc_active is
initialized as 1ULL << ATA_TAG_INTERNAL, but in hardware tag 0 is
started and this will be in done_mask on completion. ap->qc_active ^
done_mask becomes 0x100000000 ^ 0x1 = 0x100000001 and thus tag 0 used as
the internal tag will never be reported as completed.

This is fixed by introducing ata_qc_get_active() which returns the
active hardware tags and calling it where appropriate.

This is tested on mv_sata, but sata_fsl and sata_nv suffer from the same
problem. There is another case in sata_nv that most likely needs fixing
as well, but this looks a little different, so I wasn't confident enough
to change that.

I can confirm that sata_nv.ko does not work in 4.18 (and new) kernel
version correctly. More details are in email:

https://lore.kernel.org/linux-ide/20191225180824.bql2o5whougii4ch@pali/T/ (local)

I tried this patch and it fixed above problems with sata_nv.ko. It just
needs small modification (see below).

So you can add my:

Tested-by: Pali Rohár <redacted>

And I hope that patch would be backported to 4.18 and 4.19 stable
branches soon as distributions kernels are broken for machines with
these nvidia sata controllers.

Anyway, what is that another case in sata_nv which needs to be fixed
too?

It's in nv_swncq_sdbfis(). Here we have:

	sactive = readl(pp->sactive_block);
	done_mask = pp->qc_active ^ sactive;

	pp->qc_active &= ~done_mask;
	pp->dhfis_bits &= ~done_mask;
	pp->dmafis_bits &= ~done_mask;
	pp->sdbfis_bits |= done_mask;
	ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);

Sascha

Ok. Are you going to fix also this case?

-- 
Pali Rohár
pali.rohar@gmail.com