Message ID | 20191212141656.11439-1-s.hauer@pengutronix.de |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Series | libata: Fix retrieving of active qcs | expand |
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
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.
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
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,
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(-)