| Message ID | 1282268444.9628.4.camel@corey |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 10-08-19 09:40 PM, Mark Lord wrote: > Fix DSM/TRIM commands in sata_mv (v2). > These need to be issued using old-school "BM DMA", > rather than via the EDMA host queue. > > Since the chips don't have proper BM DMA status, > we need to be more careful with setting the ATA_DMA_INTR bit, > since DSM/TRIM often has a long delay between "DMA complete" > and "command complete". .. Speaking of which.. it appears that BM DMA support for sata_mv is intentionally broken by 2.6.35 kernels, with the addition of this code in libata-sff.c: unsigned int ata_bmdma_qc_issue(struct ata_queued_cmd *qc) { struct ata_port *ap = qc->ap; /* see ata_dma_blacklisted() */ BUG_ON((ap->flags & ATA_FLAG_PIO_POLLING) && qc->tf.protocol == ATAPI_PROT_DMA); Is that BUG_ON even necessary?? It really should be looking only at tf.flags, not qp->flags there, as it actually does redundantly further on: WARN_ON_ONCE(qc->tf.flags & ATA_TFLAG_POLLING); sata_mv has ATA_FLAG_PIO_POLLING set for all ports, but doesn't set ATA_TFLAG_POLLING when doing BM DMA. ??? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/20/2010 03:40 AM, Mark Lord wrote: > Fix DSM/TRIM commands in sata_mv (v2). > These need to be issued using old-school "BM DMA", > rather than via the EDMA host queue. > > Since the chips don't have proper BM DMA status, > we need to be more careful with setting the ATA_DMA_INTR bit, > since DSM/TRIM often has a long delay between "DMA complete" > and "command complete". > > GEN_I chips don't have BM DMA, so no TRIM for them. > > Signed-off-by: Mark Lord <mlord@pobox.com> > --- > > Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. > (my apologies for broken mailers -- patch also attached in case inline is b0rked). > > --- 2.6.35.2/drivers/ata/sata_mv.c 2010-08-01 18:11:14.000000000 -0400 > +++ linux/drivers/ata/sata_mv.c 2010-08-19 21:34:46.139766319 -0400 > @@ -1898,19 +1898,25 @@ > * LOCKING: > * Inherited from caller. > */ > -static void mv_bmdma_stop(struct ata_queued_cmd *qc) > +static void mv_bmdma_stop_ap(struct ata_port *ap) > { > - struct ata_port *ap = qc->ap; > void __iomem *port_mmio = mv_ap_base(ap); > u32 cmd; > > /* clear start/stop bit */ > cmd = readl(port_mmio + BMDMA_CMD); > - cmd &= ~ATA_DMA_START; > - writelfl(cmd, port_mmio + BMDMA_CMD); > + if (cmd & ATA_DMA_START) { > + cmd &= ~ATA_DMA_START; > + writelfl(cmd, port_mmio + BMDMA_CMD); > > - /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */ > - ata_sff_dma_pause(ap); > + /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */ > + ata_sff_dma_pause(ap); > + } > +} Don't we need this for other BMDMA commands too? If so, maybe it's better to make it two patches? Thanks.
On Mon, 2010-08-23 at 10:22 +0200, Tejun Heo wrote: > On 08/20/2010 03:40 AM, Mark Lord wrote: > > Fix DSM/TRIM commands in sata_mv (v2). > > These need to be issued using old-school "BM DMA", > > rather than via the EDMA host queue. > > > > Since the chips don't have proper BM DMA status, > > we need to be more careful with setting the ATA_DMA_INTR bit, > > since DSM/TRIM often has a long delay between "DMA complete" > > and "command complete". > > > > GEN_I chips don't have BM DMA, so no TRIM for them. > > > > Signed-off-by: Mark Lord <mlord@pobox.com> .. > Don't we need this for other BMDMA commands too? If so, maybe it's > better to make it two patches? TRIM is the first/only command which actually breaks without this. It is possible that other commands may benefit as well on some DVD-RW drives, but I have not encountered those yet. So for now, I've left it as a single patch, with all of the (small) pieces together for TRIM. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/23/2010 03:41 PM, Mark Lord wrote: >> Don't we need this for other BMDMA commands too? If so, maybe it's >> better to make it two patches? > > > TRIM is the first/only command which actually breaks > without this. It is possible that other commands may > benefit as well on some DVD-RW drives, but I have not > encountered those yet. > > So for now, I've left it as a single patch, > with all of the (small) pieces together for TRIM. I see. Looks good to me then. Acked-by: Tejun Heo <tj@kernel.org> Thanks.
>On 08/20/2010 03:40 AM, Mark Lord wrote: >> Fix DSM/TRIM commands in sata_mv (v2). >> These need to be issued using old-school "BM DMA", >> rather than via the EDMA host queue. >> >> Since the chips don't have proper BM DMA status, >> we need to be more careful with setting the ATA_DMA_INTR bit, >> since DSM/TRIM often has a long delay between "DMA complete" >> and "command complete". >> >> GEN_I chips don't have BM DMA, so no TRIM for them. >> >> Signed-off-by: Mark Lord <mlord@pobox.com> >> --- >> >> Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. >> (my apologies for broken mailers -- patch also attached in case inline is b0rked). >> >> --- 2.6.35.2/drivers/ata/sata_mv.c 2010-08-01 18:11:14.000000000 -0400 >> +++ linux/drivers/ata/sata_mv.c 2010-08-19 21:34:46.139766319 -0400 >> @@ -1898,19 +1898,25 @@ >> * LOCKING: >> * Inherited from caller. >> */ >> -static void mv_bmdma_stop(struct ata_queued_cmd *qc) >> +static void mv_bmdma_stop_ap(struct ata_port *ap) >> { >> - struct ata_port *ap = qc->ap; >> void __iomem *port_mmio = mv_ap_base(ap); >> u32 cmd; >> >> /* clear start/stop bit */ >> cmd = readl(port_mmio + BMDMA_CMD); >> - cmd &= ~ATA_DMA_START; >> - writelfl(cmd, port_mmio + BMDMA_CMD); >> + if (cmd & ATA_DMA_START) { >> + cmd &= ~ATA_DMA_START; >> + writelfl(cmd, port_mmio + BMDMA_CMD); >> >> - /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */ >> - ata_sff_dma_pause(ap); >> + /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */ >> + ata_sff_dma_pause(ap); >> + } >> +} > >Don't we need this for other BMDMA commands too? If so, maybe it's >better to make it two patches? >-- >tejun mlord wrote: > TRIM is the first/only command which actually breaks > without this. It is possible that other commands may > benefit as well on some DVD-RW drives, but I have not > encountered those yet. > > So for now, I've left it as a single patch, > with all of the (small) pieces together for TRIM. Oh, waitasec.. you were commenting on just that one scrap of code above. Here, there's no change to normal operation. It still gets called, and still runs. The change above just removes unnecessary register writes if/when the code gets called multiple times, which can now happen from the status polling further down. Tested and working for TRIM and ATAPI (the only two users) on both the PCI (6081) and PCIe (7042) variants, and also confirmed by Nils on his Kirkwood SOC variant. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10-08-23 09:47 AM, Tejun Heo wrote: > On 08/23/2010 03:41 PM, Mark Lord wrote: >>> Don't we need this for other BMDMA commands too? If so, maybe it's >>> better to make it two patches? >> >> >> TRIM is the first/only command which actually breaks >> without this. It is possible that other commands may >> benefit as well on some DVD-RW drives, but I have not >> encountered those yet. >> >> So for now, I've left it as a single patch, >> with all of the (small) pieces together for TRIM. > > I see. Looks good to me then. > > Acked-by: Tejun Heo<tj@kernel.org> .. Peachy. I do imagine that both of these patches might also be prime candidates for -stable, if/when they ever make it upstream. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/23/2010 03:54 PM, Mark Lord wrote: > On 10-08-23 09:47 AM, Tejun Heo wrote: >> On 08/23/2010 03:41 PM, Mark Lord wrote: >>>> Don't we need this for other BMDMA commands too? If so, maybe it's >>>> better to make it two patches? >>> >>> >>> TRIM is the first/only command which actually breaks >>> without this. It is possible that other commands may >>> benefit as well on some DVD-RW drives, but I have not >>> encountered those yet. >>> >>> So for now, I've left it as a single patch, >>> with all of the (small) pieces together for TRIM. >> >> I see. Looks good to me then. >> >> Acked-by: Tejun Heo<tj@kernel.org> > .. > > Peachy. I do imagine that both of these patches > might also be prime candidates for -stable, > if/when they ever make it upstream. Just add Cc: stable@kernel.org below SOB's. Greg will be automatically notified when these patches hit mainline. Thanks.
Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue. Since the chips don't have proper BM DMA status, we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete". GEN_I chips don't have BM DMA, so no TRIM for them. Signed-off-by: Mark Lord <mlord@pobox.com> --- Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. (my apologies for broken mailers -- patch also attached in case inline is b0rked). --- 2.6.35.2/drivers/ata/sata_mv.c 2010-08-01 18:11:14.000000000 -0400 +++ linux/drivers/ata/sata_mv.c 2010-08-19 21:34:46.139766319 -0400 @@ -1898,19 +1898,25 @@ * LOCKING: * Inherited from caller. */ -static void mv_bmdma_stop(struct ata_queued_cmd *qc) +static void mv_bmdma_stop_ap(struct ata_port *ap) { - struct ata_port *ap = qc->ap; void __iomem *port_mmio = mv_ap_base(ap); u32 cmd; /* clear start/stop bit */ cmd = readl(port_mmio + BMDMA_CMD); - cmd &= ~ATA_DMA_START; - writelfl(cmd, port_mmio + BMDMA_CMD); + if (cmd & ATA_DMA_START) { + cmd &= ~ATA_DMA_START; + writelfl(cmd, port_mmio + BMDMA_CMD); - /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */ - ata_sff_dma_pause(ap); + /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */ + ata_sff_dma_pause(ap); + } +} + +static void mv_bmdma_stop(struct ata_queued_cmd *qc) +{ + mv_bmdma_stop_ap(qc->ap); } /** @@ -1934,8 +1940,21 @@ reg = readl(port_mmio + BMDMA_STATUS); if (reg & ATA_DMA_ACTIVE) status = ATA_DMA_ACTIVE; - else + else if (reg & ATA_DMA_ERR) status = (reg & ATA_DMA_ERR) | ATA_DMA_INTR; + else { + /* + * Just because DMA_ACTIVE is 0 (DMA completed), + * this does _not_ mean the device is "done". + * So we should not yet be signalling ATA_DMA_INTR + * in some cases. Eg. DSM/TRIM, and perhaps others. + */ + mv_bmdma_stop_ap(ap); + if (ioread8(ap->ioaddr.altstatus_addr) & ATA_BUSY) + status = 0; + else + status = ATA_DMA_INTR; + } return status; } @@ -1995,6 +2014,9 @@ switch (tf->protocol) { case ATA_PROT_DMA: + if (tf->command == ATA_CMD_DSM) + return; + /* fall-thru */ case ATA_PROT_NCQ: break; /* continue below */ case ATA_PROT_PIO: @@ -2094,6 +2116,8 @@ if ((tf->protocol != ATA_PROT_DMA) && (tf->protocol != ATA_PROT_NCQ)) return; + if (tf->command == ATA_CMD_DSM) + return; /* use bmdma for this */ /* Fill in Gen IIE command request block */ if (!(tf->flags & ATA_TFLAG_WRITE)) @@ -2289,6 +2313,12 @@ switch (qc->tf.protocol) { case ATA_PROT_DMA: + if (qc->tf.command == ATA_CMD_DSM) { + if (!ap->ops->bmdma_setup) /* no bmdma on GEN_I */ + return AC_ERR_OTHER; + break; /* use bmdma for this */ + } + /* fall thru */ case ATA_PROT_NCQ: mv_start_edma(ap, port_mmio, pp, qc->tf.protocol); pp->req_idx = (pp->req_idx + 1) & MV_MAX_Q_DEPTH_MASK;
Fix DSM/TRIM commands in sata_mv (v2). These need to be issued using old-school "BM DMA", rather than via the EDMA host queue. Since the chips don't have proper BM DMA status, we need to be more careful with setting the ATA_DMA_INTR bit, since DSM/TRIM often has a long delay between "DMA complete" and "command complete". GEN_I chips don't have BM DMA, so no TRIM for them. Signed-off-by: Mark Lord <mlord@pobox.com> --- Re-issue of original patch, with fixes for GEN_I and GEN_II chipsets. (my apologies for broken mailers -- patch also attached in case inline is b0rked).