Subject: Re: lack of pciide transfer alignment checking causes crash
To: Manuel Bouyer <bouyer@antioche.eu.org>
From: Jason Thorpe <thorpej@shagadelic.org>
List: tech-kern
Date: 06/25/2005 12:58:39
--Apple-Mail-4--311601422
Content-Transfer-Encoding: 7bit
Content-Type: text/plain;
charset=US-ASCII;
delsp=yes;
format=flowed
On Jun 25, 2005, at 12:42 PM, Manuel Bouyer wrote:
> Because I suspect it would be less code, and probably faster when
> we don't
> have a properly-aligned buffer.
The work-around isn't that much code, either. Erik -- I don't know
if I picked the right magic number for the alignment constraint...
please double-check.
-- thorpej
--Apple-Mail-4--311601422
Content-Transfer-Encoding: 7bit
Content-Type: text/plain;
x-mac-type=54455854;
x-unix-mode=0755;
x-mac-creator=74747874;
name="geodeide-patch.txt"
Content-Disposition: attachment;
filename=geodeide-patch.txt
Index: ata/ata_wdc.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ata/ata_wdc.c,v
retrieving revision 1.81
diff -u -p -r1.81 ata_wdc.c
--- ata/ata_wdc.c 7 Jun 2005 13:45:11 -0000 1.81
+++ ata/ata_wdc.c 25 Jun 2005 19:56:12 -0000
@@ -323,7 +323,7 @@ _wdc_ata_bio_start(struct ata_channel *c
int wait_flags = (xfer->c_flags & C_POLL) ? AT_POLL : 0;
u_int16_t cyl;
u_int8_t head, sect, cmd = 0;
- int nblks;
+ int nblks, error;
int dma_flags = 0;
ATADEBUG_PRINT(("_wdc_ata_bio_start %s:%d:%d\n",
@@ -397,10 +397,21 @@ again:
cmd = (ata_bio->flags & ATA_READ) ?
WDCC_READDMA : WDCC_WRITEDMA;
/* Init the DMA channel. */
- if ((*wdc->dma_init)(wdc->dma_arg,
+ error = (*wdc->dma_init)(wdc->dma_arg,
chp->ch_channel, xfer->c_drive,
(char *)xfer->c_databuf + xfer->c_skip,
- ata_bio->nbytes, dma_flags) != 0) {
+ ata_bio->nbytes, dma_flags);
+ if (error) {
+ if (error == EINVAL) {
+ /*
+ * We can't do DMA on this transfer
+ * for some reason. Fall back to
+ * PIO.
+ */
+ xfer->c_flags &= ~C_DMA;
+ error = 0;
+ goto do_pio;
+ }
ata_bio->error = ERR_DMA;
ata_bio->r_error = 0;
wdc_ata_bio_done(chp, xfer);
@@ -437,6 +448,7 @@ again:
/* wait for irq */
goto intr;
} /* else not DMA */
+ do_pio:
ata_bio->nblks = min(nblks, ata_bio->multi);
ata_bio->nbytes = ata_bio->nblks * ata_bio->lp->d_secsize;
KASSERT(nblks == 1 || (ata_bio->flags & ATA_SINGLE) == 0);
Index: pci/geodeide.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/geodeide.c,v
retrieving revision 1.9
diff -u -p -r1.9 geodeide.c
--- pci/geodeide.c 25 Jun 2005 05:04:01 -0000 1.9
+++ pci/geodeide.c 25 Jun 2005 19:56:15 -0000
@@ -54,6 +54,7 @@ __KERNEL_RCSID(0, "$NetBSD: geodeide.c,v
static void geodeide_chip_map(struct pciide_softc *,
struct pci_attach_args *);
static void geodeide_setup_channel(struct ata_channel *);
+static void geodeide_dma_init(void *, int, int, void *, size_t, int);
static int geodeide_match(struct device *, struct cfdata *, void *);
static void geodeide_attach(struct device *, struct device *, void *);
@@ -120,6 +121,11 @@ geodeide_chip_map(struct pciide_softc *s
if (sc->sc_dma_ok) {
sc->sc_wdcdev.sc_atac.atac_cap = ATAC_CAP_DMA | ATAC_CAP_UDMA;
sc->sc_wdcdev.irqack = pciide_irqack;
+ /*
+ * XXXJRT What chip revisions actually need the DMA
+ * alignment work-around?
+ */
+ sc->sc_wdcdev.dma_init = geodeide_dma_init;
}
sc->sc_wdcdev.sc_atac.atac_pio_cap = 4;
sc->sc_wdcdev.sc_atac.atac_dma_cap = 2;
@@ -260,3 +266,18 @@ geodeide_setup_channel(struct ata_channe
idedma_ctl);
}
}
+
+static int
+geodeide_dma_init(void *v, int channel, int drive, void *databuf,
+ size_t datalen, int flags)
+{
+
+ /*
+ * If the buffer is not properly aligned, we can't allow DMA
+ * and need to fall back to PIO.
+ */
+ if (((uintptr_t)databuf) & 0xf)
+ return (EINVAL);
+
+ return (pciide_dma_init(v, channel, drive, databuf, datalen, flags));
+}
Index: scsipi/atapi_wdc.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/atapi_wdc.c,v
retrieving revision 1.94
diff -u -p -r1.94 atapi_wdc.c
--- scsipi/atapi_wdc.c 7 Jun 2005 13:45:11 -0000 1.94
+++ scsipi/atapi_wdc.c 25 Jun 2005 19:56:19 -0000
@@ -611,7 +611,7 @@ wdc_atapi_intr(struct ata_channel *chp,
struct scsipi_xfer *sc_xfer = xfer->c_cmd;
struct ata_drive_datas *drvp = &chp->ch_drive[xfer->c_drive];
int len, phase, i, retries=0;
- int ire;
+ int ire, error;
int dma_flags = 0;
void *cmd;
@@ -692,11 +692,22 @@ again:
ATADEBUG_PRINT(("PHASE_CMDOUT\n"), DEBUG_INTR);
/* Init the DMA channel if necessary */
if (xfer->c_flags & C_DMA) {
- if ((*wdc->dma_init)(wdc->dma_arg,
+ error = (*wdc->dma_init)(wdc->dma_arg,
chp->ch_channel, xfer->c_drive,
- xfer->c_databuf, xfer->c_bcount, dma_flags) != 0) {
- sc_xfer->error = XS_DRIVER_STUFFUP;
- break;
+ xfer->c_databuf, xfer->c_bcount, dma_flags);
+ if (error) {
+ if (error == EINVAL) {
+ /*
+ * We can't do DMA on this transfer
+ * for some reason. Fall back to
+ * PIO.
+ */
+ xfer->c_flags &= ~C_DMA;
+ error = 0;
+ } else {
+ sc_xfer->error = XS_DRIVER_STUFFUP;
+ break;
+ }
}
}
--Apple-Mail-4--311601422--