On Mon, Jan 27, 2020 at 12:16:30PM +0100, Pali Rohár wrote:

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

quoted

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?

As said, this one looks slightly different than the others and I would
prefer if somebody could fix it who actually has a hardware and can test
it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |