linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] libata: Fix retrieving of active qcs
@ 2019-12-13  8:03 Sascha Hauer
  2019-12-13  8:05 ` Sascha Hauer
  0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2019-12-13  8:03 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, Jens Axboe, kernel, 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>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: Fix retrieving of active qcs
  2019-12-13  8:03 [PATCH] libata: Fix retrieving of active qcs Sascha Hauer
@ 2019-12-13  8:05 ` Sascha Hauer
  0 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2019-12-13  8:05 UTC (permalink / raw)
  To: linux-ide; +Cc: Jens Axboe, linux-kernel, kernel

On Fri, Dec 13, 2019 at 09:03:34AM +0100, 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.
> 
> Fixes: 28361c403683 ("libata: add extra internal command")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---

Ignore this, it lacks the v2 tag :-/

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 |

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: Fix retrieving of active qcs
  2019-12-12 15:12   ` Jens Axboe
@ 2019-12-13  8:05     ` Sascha Hauer
  0 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2019-12-13  8:05 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-ide, linux-kernel, kernel

On Thu, Dec 12, 2019 at 08:12:27AM -0700, Jens Axboe wrote:
> On 12/12/19 7:23 AM, Sascha Hauer wrote:
> > On Thu, Dec 12, 2019 at 03:16:56PM +0100, 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.
> >>
> >> Fixes: 28361c403683 ("libata: add extra internal command")
> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >> ---
> >>  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 28c492be0a57..d73bec933892 100644
> >> --- a/drivers/ata/libata-core.c
> >> +++ b/drivers/ata/libata-core.c
> >> @@ -5325,6 +5325,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 8e9cb198fcd1..ca6c706e9c25 100644
> >> --- a/drivers/ata/sata_fsl.c
> >> +++ b/drivers/ata/sata_fsl.c
> >> @@ -1278,7 +1278,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 ad385a113391..bde695a32097 100644
> >> --- a/drivers/ata/sata_mv.c
> >> +++ b/drivers/ata/sata_mv.c
> >> @@ -2827,7 +2827,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 56946012d113..7510303111fa 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 207e7ee764ce..f4c045b56e6c 100644
> >> --- a/include/linux/libata.h
> >> +++ b/include/linux/libata.h
> >> @@ -1174,6 +1174,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_started(struct ata_port *ap);
> > 
> > s/ata_qc_get_started/ata_qc_get_active/ obviously.
> 
> Last minute edit? Please send a v2.

Just did that

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: Fix retrieving of active qcs
  2019-12-12 14:23 ` Sascha Hauer
@ 2019-12-12 15:12   ` Jens Axboe
  2019-12-13  8:05     ` Sascha Hauer
  0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2019-12-12 15:12 UTC (permalink / raw)
  To: Sascha Hauer, linux-ide; +Cc: linux-kernel, kernel

On 12/12/19 7:23 AM, Sascha Hauer wrote:
> On Thu, Dec 12, 2019 at 03:16:56PM +0100, 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.
>>
>> Fixes: 28361c403683 ("libata: add extra internal command")
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>  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 28c492be0a57..d73bec933892 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5325,6 +5325,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 8e9cb198fcd1..ca6c706e9c25 100644
>> --- a/drivers/ata/sata_fsl.c
>> +++ b/drivers/ata/sata_fsl.c
>> @@ -1278,7 +1278,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 ad385a113391..bde695a32097 100644
>> --- a/drivers/ata/sata_mv.c
>> +++ b/drivers/ata/sata_mv.c
>> @@ -2827,7 +2827,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 56946012d113..7510303111fa 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 207e7ee764ce..f4c045b56e6c 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -1174,6 +1174,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_started(struct ata_port *ap);
> 
> s/ata_qc_get_started/ata_qc_get_active/ obviously.

Last minute edit? Please send a v2.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: Fix retrieving of active qcs
  2019-12-12 14:16 Sascha Hauer
@ 2019-12-12 14:23 ` Sascha Hauer
  2019-12-12 15:12   ` Jens Axboe
  0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2019-12-12 14:23 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, Jens Axboe, kernel

On Thu, Dec 12, 2019 at 03:16:56PM +0100, 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.
> 
> Fixes: 28361c403683 ("libata: add extra internal command")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  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 28c492be0a57..d73bec933892 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5325,6 +5325,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 8e9cb198fcd1..ca6c706e9c25 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1278,7 +1278,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 ad385a113391..bde695a32097 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -2827,7 +2827,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 56946012d113..7510303111fa 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 207e7ee764ce..f4c045b56e6c 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1174,6 +1174,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_started(struct ata_port *ap);

s/ata_qc_get_started/ata_qc_get_active/ obviously.

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 |

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] libata: Fix retrieving of active qcs
@ 2019-12-12 14:16 Sascha Hauer
  2019-12-12 14:23 ` Sascha Hauer
  0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2019-12-12 14:16 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, Jens Axboe, kernel, 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>
---
 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 28c492be0a57..d73bec933892 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5325,6 +5325,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 8e9cb198fcd1..ca6c706e9c25 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1278,7 +1278,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 ad385a113391..bde695a32097 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2827,7 +2827,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 56946012d113..7510303111fa 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 207e7ee764ce..f4c045b56e6c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1174,6 +1174,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_started(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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-13  8:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-13  8:03 [PATCH] libata: Fix retrieving of active qcs Sascha Hauer
2019-12-13  8:05 ` Sascha Hauer
  -- strict thread matches above, loose matches on Subject: below --
2019-12-12 14:16 Sascha Hauer
2019-12-12 14:23 ` Sascha Hauer
2019-12-12 15:12   ` Jens Axboe
2019-12-13  8:05     ` Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).