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