2019-12-13 08:05:24

by Sascha Hauer

[permalink] [raw]
Subject: [PATCH v2] libata: Fix retrieving of active qcs

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 <[email protected]>
---

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


2019-12-25 18:20:24

by Pali Rohár

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

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/[email protected]/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 <[email protected]>

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 <[email protected]>
> ---
>
> 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
[email protected]


Attachments:
(No filename) (5.57 kB)
signature.asc (201.00 B)
Download all attachments

2019-12-25 18:27:45

by Jens Axboe

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

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/[email protected]/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 <[email protected]>
>
> 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

2019-12-25 18:36:57

by Pali Rohár

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

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/[email protected]/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 <[email protected]>
> >
> > 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
[email protected]


Attachments:
(No filename) (2.33 kB)
signature.asc (201.00 B)
Download all attachments

2019-12-26 00:08:57

by Jens Axboe

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

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/[email protected]/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 <[email protected]>
>>>
>>> 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

2020-01-06 08:17:10

by Sascha Hauer

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

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/[email protected]/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 <[email protected]>
>
> 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 |

2020-01-27 11:20:29

by Pali Rohár

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

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/[email protected]/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 <[email protected]>
> >
> > 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
[email protected]

2020-01-27 11:25:58

by Sascha Hauer

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

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/[email protected]/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 <[email protected]>
> > >
> > > 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 |

2020-05-03 21:48:29

by Pali Rohár

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

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/[email protected]/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 <[email protected]>
> > > >
> > > > 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.