[PATCH v2] libata: Fix retrieving of active qcs

[PATCH v2] libata: Fix retrieving of active qcs

[Sascha Hauer]

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.

Fixes: 28361c403683 ("libata: add extra internal command")
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Changes since v1:
- Fix wrong function name in include/linux/libata.h

 drivers/ata/libata-core.c | 23 +++++++++++++++++++++++
 drivers/ata/sata_fsl.c    |  2 +-
 drivers/ata/sata_mv.c     |  2 +-
 drivers/ata/sata_nv.c     |  2 +-
 include/linux/libata.h    |  1 +
 5 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e9017c570bc5..a330e1f28ff4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5328,6 +5328,29 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 	}
 }
 
+/**
+ *	ata_qc_get_active - get bitmask of active qcs
+ *	@ap: port in question
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ *
+ *	RETURNS:
+ *	Bitmask of active qcs
+ */
+u64 ata_qc_get_active(struct ata_port *ap)
+{
+	u64 qc_active = ap->qc_active;
+
+	/* ATA_TAG_INTERNAL is sent to hw as tag 0 */
+	if (qc_active & (1ULL << ATA_TAG_INTERNAL)) {
+		qc_active |= (1 << 0);
+		qc_active &= ~(1ULL << ATA_TAG_INTERNAL);
+	}
+
+	return qc_active;
+}
+
 /**
  *	ata_qc_complete_multiple - Complete multiple qcs successfully
  *	@ap: port in question
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 9239615d8a04..d55ee244d693 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1280,7 +1280,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 				     i, ioread32(hcr_base + CC),
 				     ioread32(hcr_base + CA));
 		}
-		ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
+		ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
 		return;
 
 	} else if ((ap->qc_active & (1ULL << ATA_TAG_INTERNAL))) {
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 277f11909fc1..d7228f8e9297 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2829,7 +2829,7 @@ static void mv_process_crpb_entries(struct ata_port *ap, struct mv_port_priv *pp
 	}
 
 	if (work_done) {
-		ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
+		ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
 
 		/* Update the software queue position index in hardware */
 		writelfl((pp->crpb_dma & EDMA_RSP_Q_BASE_LO_MASK) |
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index f3e62f5528bd..eb9dc14e5147 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -984,7 +984,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
 					check_commands = 0;
 				check_commands &= ~(1 << pos);
 			}
-			ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
+			ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
 		}
 	}
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d3bbfddf616a..2dbde119721d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1175,6 +1175,7 @@ extern unsigned int ata_do_dev_read_id(struct ata_device *dev,
 					struct ata_taskfile *tf, u16 *id);
 extern void ata_qc_complete(struct ata_queued_cmd *qc);
 extern int ata_qc_complete_multiple(struct ata_port *ap, u64 qc_active);
+extern u64 ata_qc_get_active(struct ata_port *ap);
 extern void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd);
 extern int ata_std_bios_param(struct scsi_device *sdev,
 			      struct block_device *bdev,
-- 
2.24.0

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 5701 bytes --]

Hello Sascha!

On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
> 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/

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 <pali.rohar@gmail.com>

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?

> Fixes: 28361c403683 ("libata: add extra internal command")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> Changes since v1:
> - Fix wrong function name in include/linux/libata.h
> 
>  drivers/ata/libata-core.c | 23 +++++++++++++++++++++++
>  drivers/ata/sata_fsl.c    |  2 +-
>  drivers/ata/sata_mv.c     |  2 +-
>  drivers/ata/sata_nv.c     |  2 +-
>  include/linux/libata.h    |  1 +
>  5 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e9017c570bc5..a330e1f28ff4 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5328,6 +5328,29 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
>  	}
>  }
>  
> +/**
> + *	ata_qc_get_active - get bitmask of active qcs
> + *	@ap: port in question
> + *
> + *	LOCKING:
> + *	spin_lock_irqsave(host lock)
> + *
> + *	RETURNS:
> + *	Bitmask of active qcs
> + */
> +u64 ata_qc_get_active(struct ata_port *ap)
> +{
> +	u64 qc_active = ap->qc_active;
> +
> +	/* ATA_TAG_INTERNAL is sent to hw as tag 0 */
> +	if (qc_active & (1ULL << ATA_TAG_INTERNAL)) {
> +		qc_active |= (1 << 0);
> +		qc_active &= ~(1ULL << ATA_TAG_INTERNAL);
> +	}
> +
> +	return qc_active;
> +}

Here is missing

  EXPORT_SYMBOL_GPL(ata_qc_get_active);

otherwise sata_nv.ko cannot use this ata_qc_get_active() function and
throw error about undefined symbol when modprobing module.

> +
>  /**
>   *	ata_qc_complete_multiple - Complete multiple qcs successfully
>   *	@ap: port in question
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 9239615d8a04..d55ee244d693 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1280,7 +1280,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>  				     i, ioread32(hcr_base + CC),
>  				     ioread32(hcr_base + CA));
>  		}
> -		ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
> +		ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
>  		return;
>  
>  	} else if ((ap->qc_active & (1ULL << ATA_TAG_INTERNAL))) {
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 277f11909fc1..d7228f8e9297 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -2829,7 +2829,7 @@ static void mv_process_crpb_entries(struct ata_port *ap, struct mv_port_priv *pp
>  	}
>  
>  	if (work_done) {
> -		ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
> +		ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
>  
>  		/* Update the software queue position index in hardware */
>  		writelfl((pp->crpb_dma & EDMA_RSP_Q_BASE_LO_MASK) |
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index f3e62f5528bd..eb9dc14e5147 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -984,7 +984,7 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
>  					check_commands = 0;
>  				check_commands &= ~(1 << pos);
>  			}
> -			ata_qc_complete_multiple(ap, ap->qc_active ^ done_mask);
> +			ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
>  		}
>  	}
>  
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index d3bbfddf616a..2dbde119721d 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1175,6 +1175,7 @@ extern unsigned int ata_do_dev_read_id(struct ata_device *dev,
>  					struct ata_taskfile *tf, u16 *id);
>  extern void ata_qc_complete(struct ata_queued_cmd *qc);
>  extern int ata_qc_complete_multiple(struct ata_port *ap, u64 qc_active);
> +extern u64 ata_qc_get_active(struct ata_port *ap);
>  extern void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd);
>  extern int ata_std_bios_param(struct scsi_device *sdev,
>  			      struct block_device *bdev,

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Jens Axboe]

On 12/25/19 11:18 AM, Pali Rohár wrote:
> Hello Sascha!
> 
> On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
>> 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/
> 
> 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 <pali.rohar@gmail.com>
> 
> 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?

Thanks for testing, I've applied this for 5.5 and marked it for stable.

-- 
Jens Axboe

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 2390 bytes --]

On Wednesday 25 December 2019 11:26:47 Jens Axboe wrote:
> On 12/25/19 11:18 AM, Pali Rohár wrote:
> > Hello Sascha!
> > 
> > On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
> >> 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/
> > 
> > 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 <pali.rohar@gmail.com>
> > 
> > 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?
> 
> Thanks for testing, I've applied this for 5.5 and marked it for stable.

It is this one?
https://git.kernel.dk/cgit/linux-block/commit/?h=libata-5.5&id=d80f359d0ebddb3ab3e9cc3fe96f244827ae7b09

Because there is missing EXPORT_SYMBOL_GPL for ata_qc_get_active()
function as I wrote in previous email.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Jens Axboe]

On 12/25/19 11:36 AM, Pali Rohár wrote:
> On Wednesday 25 December 2019 11:26:47 Jens Axboe wrote:
>> On 12/25/19 11:18 AM, Pali Rohár wrote:
>>> Hello Sascha!
>>>
>>> On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
>>>> 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/
>>>
>>> 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 <pali.rohar@gmail.com>
>>>
>>> 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?
>>
>> Thanks for testing, I've applied this for 5.5 and marked it for stable.
> 
> It is this one?
> https://git.kernel.dk/cgit/linux-block/commit/?h=libata-5.5&id=d80f359d0ebddb3ab3e9cc3fe96f244827ae7b09
> 
> Because there is missing EXPORT_SYMBOL_GPL for ata_qc_get_active()
> function as I wrote in previous email.

I missed that, I'll add it. Thanks!

-- 
Jens Axboe

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Sascha Hauer]

On Wed, Dec 25, 2019 at 07:18:40PM +0100, Pali Rohár wrote:
> Hello Sascha!
> 
> On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
> > 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/
> 
> 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 <pali.rohar@gmail.com>
> 
> 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


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

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Pali Rohár]

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:
> > Hello Sascha!
> > 
> > On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
> > > 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/
> > 
> > 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 <pali.rohar@gmail.com>
> > 
> > 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

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Sascha Hauer]

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:
> > On Wed, Dec 25, 2019 at 07:18:40PM +0100, Pali Rohár wrote:
> > > Hello Sascha!
> > > 
> > > On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
> > > > 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/
> > > 
> > > 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 <pali.rohar@gmail.com>
> > > 
> > > 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 |

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Pali Rohár]

On Monday 27 January 2020 12:24:28 Sascha Hauer wrote:
> 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:
> > > On Wed, Dec 25, 2019 at 07:18:40PM +0100, Pali Rohár wrote:
> > > > Hello Sascha!
> > > > 
> > > > On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
> > > > > 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/
> > > > 
> > > > 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 <pali.rohar@gmail.com>
> > > > 
> > > > 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.

Well, I have hardware and could test changes. But I'm not really sure
that I understand this part of code. So it would be better if somebody
else with better knowledge prepares patches I could test them. But
currently during coronavirus I have only remote ssh access, so boot,
modify/compile/reboot process is quite slower.

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Sascha Hauer]

On Sun, May 03, 2020 at 11:46:27PM +0200, Pali Rohár wrote:
> On Monday 27 January 2020 12:24:28 Sascha Hauer wrote:
> > 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:
> > > > On Wed, Dec 25, 2019 at 07:18:40PM +0100, Pali Rohár wrote:
> > > > > Hello Sascha!
> > > > > 
> > > > > On Friday 13 December 2019 09:04:08 Sascha Hauer wrote:
> > > > > > 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/
> > > > > 
> > > > > 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 <pali.rohar@gmail.com>
> > > > > 
> > > > > 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.
> 
> Well, I have hardware and could test changes. But I'm not really sure
> that I understand this part of code. So it would be better if somebody
> else with better knowledge prepares patches I could test them. But
> currently during coronavirus I have only remote ssh access, so boot,
> modify/compile/reboot process is quite slower.

Ok, here we go. Compile tested only.

Regards,
 Sascha

------------------------------8<-----------------------------------

From fcdcfa9e7a4ee4faf411de1df4f3c4e12c78545c Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Fri, 8 May 2020 07:28:19 +0200
Subject: [PATCH] ata: sata_nv: Fix retrieving of active qcs

ata_qc_complete_multiple() has to be called with the tags physically
active, that is the hw tag is at bit 0. ap->qc_active has the same tag
at bit ATA_TAG_INTERNAL instead, so call ata_qc_get_active() to fix that
up. This is done in the vein of 8385d756e114 ("libata: Fix retrieving of
active qcs").

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/ata/sata_nv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index eb9dc14e5147..20190f66ced9 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -2100,7 +2100,7 @@ static int nv_swncq_sdbfis(struct ata_port *ap)
 	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);
+	ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
 
 	if (!ap->qc_active) {
 		DPRINTK("over\n");
-- 
2.26.2

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

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Pali Rohár]

On Friday 08 May 2020 07:46:44 Sascha Hauer wrote:
> From fcdcfa9e7a4ee4faf411de1df4f3c4e12c78545c Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Fri, 8 May 2020 07:28:19 +0200
> Subject: [PATCH] ata: sata_nv: Fix retrieving of active qcs
> 
> ata_qc_complete_multiple() has to be called with the tags physically
> active, that is the hw tag is at bit 0. ap->qc_active has the same tag
> at bit ATA_TAG_INTERNAL instead, so call ata_qc_get_active() to fix that
> up. This is done in the vein of 8385d756e114 ("libata: Fix retrieving of
> active qcs").
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

I tested this second change on nforce4 box with sata_nv controllers:

  00:07.0 IDE interface [0101]: NVIDIA Corporation CK804 Serial ATA Controller [10de:0054] (rev f3)
  00:08.0 IDE interface [0101]: NVIDIA Corporation CK804 Serial ATA Controller [10de:0055] (rev f3)

Both disks are working fine, I do not see any regression or change, so
you can add my:

Tested-by: Pali Rohár <pali@kernel.org>

Ideally add also Fixes line:

Fixes: 28361c403683 ("libata: add extra internal command")

Jens, do you need something more from me? Some special tests, etc?

> ---
>  drivers/ata/sata_nv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index eb9dc14e5147..20190f66ced9 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -2100,7 +2100,7 @@ static int nv_swncq_sdbfis(struct ata_port *ap)
>  	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);
> +	ata_qc_complete_multiple(ap, ata_qc_get_active(ap) ^ done_mask);
>  
>  	if (!ap->qc_active) {
>  		DPRINTK("over\n");
> -- 
> 2.26.2

Re: [PATCH v2] libata: Fix retrieving of active qcs

[Jens Axboe]

On 10/28/20 7:55 AM, Pali Rohár wrote:
> On Friday 08 May 2020 07:46:44 Sascha Hauer wrote:
>> From fcdcfa9e7a4ee4faf411de1df4f3c4e12c78545c Mon Sep 17 00:00:00 2001
>> From: Sascha Hauer <s.hauer@pengutronix.de>
>> Date: Fri, 8 May 2020 07:28:19 +0200
>> Subject: [PATCH] ata: sata_nv: Fix retrieving of active qcs
>>
>> ata_qc_complete_multiple() has to be called with the tags physically
>> active, that is the hw tag is at bit 0. ap->qc_active has the same tag
>> at bit ATA_TAG_INTERNAL instead, so call ata_qc_get_active() to fix that
>> up. This is done in the vein of 8385d756e114 ("libata: Fix retrieving of
>> active qcs").
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> I tested this second change on nforce4 box with sata_nv controllers:
> 
>   00:07.0 IDE interface [0101]: NVIDIA Corporation CK804 Serial ATA Controller [10de:0054] (rev f3)
>   00:08.0 IDE interface [0101]: NVIDIA Corporation CK804 Serial ATA Controller [10de:0055] (rev f3)
> 
> Both disks are working fine, I do not see any regression or change, so
> you can add my:
> 
> Tested-by: Pali Rohár <pali@kernel.org>
> 
> Ideally add also Fixes line:
> 
> Fixes: 28361c403683 ("libata: add extra internal command")
> 
> Jens, do you need something more from me? Some special tests, etc?

Thanks, I'll queue this up.

-- 
Jens Axboe