[RFC][PATCH] libata ATAPI alignment

[RFC][PATCH] libata ATAPI alignment

[Jeff Garzik]

So, one thing that's terribly ugly about SATA ATAPI is that we need to
pad DMA transfers to the next 32-bit boundary, if the length is not
evenly divisible by 4.

Messing with the scatterlist to accomplish this is terribly ugly
no matter how you slice it.  One way would be to create my own
scatterlist, via memcpy and then manual labor.  Another way would be
to special case a pad buffer, appending it onto the end of various
scatterlist code.

Complicating matters, we currently must support two methods of data
buffer submission:  a single kernel virtual address, or a struct
scatterlist.

Review is requested by any and all parties, as well as suggestions for
a prettier approach.

This is one of the last steps needed to get ATAPI going.



diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -44,7 +44,7 @@
 
 enum {
 	AHCI_PCI_BAR		= 5,
-	AHCI_MAX_SG		= 168, /* hardware max is 64K */
+	AHCI_MAX_SG		= 300, /* hardware max is 64K */
 	AHCI_DMA_BOUNDARY	= 0xffffffff,
 	AHCI_USE_CLUSTERING	= 0,
 	AHCI_CMD_SLOT_SZ	= 32 * 32,
@@ -197,7 +197,7 @@ static Scsi_Host_Template ahci_sht = {
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
-	.sg_tablesize		= AHCI_MAX_SG,
+	.sg_tablesize		= AHCI_MAX_SG - 1,
 	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
@@ -313,8 +313,15 @@ static int ahci_port_start(struct ata_po
 		return -ENOMEM;
 	memset(pp, 0, sizeof(*pp));
 
+	ap->pad = dma_alloc_coherent(dev, ATA_DMA_PAD_BUF_SZ, &ap->pad_dma, GFP_KERNEL);
+	if (!ap->pad) {
+		kfree(pp);
+		return -ENOMEM;
+	}
+
 	mem = dma_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma, GFP_KERNEL);
 	if (!mem) {
+		dma_free_coherent(dev, ATA_DMA_PAD_BUF_SZ, ap->pad, ap->pad_dma);
 		kfree(pp);
 		return -ENOMEM;
 	}
@@ -390,6 +397,7 @@ static void ahci_port_stop(struct ata_po
 	ap->private_data = NULL;
 	dma_free_coherent(dev, AHCI_PORT_PRIV_DMA_SZ,
 			  pp->cmd_slot, pp->cmd_slot_dma);
+	dma_free_coherent(dev, ATA_DMA_PAD_BUF_SZ, ap->pad, ap->pad_dma);
 	kfree(pp);
 }
 
@@ -474,7 +482,8 @@ static void ahci_tf_read(struct ata_port
 
 static void ahci_fill_sg(struct ata_queued_cmd *qc)
 {
-	struct ahci_port_priv *pp = qc->ap->private_data;
+	struct ata_port *ap = qc->ap;
+	struct ahci_port_priv *pp = ap->private_data;
 	unsigned int i;
 
 	VPRINTK("ENTER\n");
@@ -493,6 +502,24 @@ static void ahci_fill_sg(struct ata_queu
 		pp->cmd_tbl_sg[i].addr_hi = cpu_to_le32((addr >> 16) >> 16);
 		pp->cmd_tbl_sg[i].flags_size = cpu_to_le32(sg_len - 1);
 	}
+
+	/* if we added a small buffer, to pad xfer to next 32-bit bound,
+	 * add it to the s/g list here
+	 */
+	if (qc->flags & ATA_QCFLAG_PADDED) {
+		dma_addr_t pad_addr = ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ);
+		u32 len;
+
+		/* fixup last s/g entry */
+		len = le32_to_cpu(pp->cmd_tbl_sg[i - 1].flags_size);
+		pp->cmd_tbl_sg[i - 1].flags_size =
+			cpu_to_le32(len - qc->pad_len);
+
+		/* append pad buffer to s/g list */
+		pp->cmd_tbl_sg[i].addr = cpu_to_le32(pad_addr & 0xffffffff);
+		pp->cmd_tbl_sg[i].addr_hi = cpu_to_le32((pad_addr >> 16) >> 16);
+		pp->cmd_tbl_sg[i].flags_size = cpu_to_le32(ATA_DMA_PAD_SZ - 1);
+	}
 }
 
 static void ahci_qc_prep(struct ata_queued_cmd *qc)
@@ -501,13 +528,16 @@ static void ahci_qc_prep(struct ata_queu
 	struct ahci_port_priv *pp = ap->private_data;
 	u32 opts;
 	const u32 cmd_fis_len = 5; /* five dwords */
+	unsigned int n_elem = qc->n_elem;
 
 	/*
 	 * Fill in command slot information (currently only one slot,
 	 * slot 0, is currently since we don't do queueing)
 	 */
 
-	opts = (qc->n_elem << 16) | cmd_fis_len;
+	if (qc->flags & ATA_QCFLAG_PADDED)
+		n_elem++;
+	opts = (n_elem << 16) | cmd_fis_len;
 	if (qc->tf.flags & ATA_TFLAG_WRITE)
 		opts |= AHCI_CMD_WRITE;
 	if (is_atapi_taskfile(&qc->tf))
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2144,6 +2144,8 @@ static void ata_sg_clean(struct ata_queu
 	struct ata_port *ap = qc->ap;
 	struct scatterlist *sg = qc->sg;
 	int dir = qc->dma_dir;
+	unsigned int copy_pad = 0;
+	void *pad_buf = NULL;
 
 	assert(qc->flags & ATA_QCFLAG_DMAMAP);
 	assert(sg != NULL);
@@ -2153,11 +2155,33 @@ static void ata_sg_clean(struct ata_queu
 
 	DPRINTK("unmapping %u sg elements\n", qc->n_elem);
 
-	if (qc->flags & ATA_QCFLAG_SG)
+	/* if we padded the buffer out to 32-bit bound, and data
+	 * xfer direction is from-device, we must copy from the
+	 * pad buffer back into the supplied buffer
+	 */
+	if ((qc->flags & ATA_QCFLAG_PADDED) &&
+	    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
+		copy_pad = 1;
+		pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
+	}
+
+	if (qc->flags & ATA_QCFLAG_SG) {
 		dma_unmap_sg(ap->host_set->dev, sg, qc->n_elem, dir);
-	else
+		if (copy_pad) {
+			void *addr = kmap_atomic(sg[qc->n_elem - 1].page, KM_IRQ0);
+			memcpy(addr + sg[qc->n_elem - 1].offset +
+			         sg[qc->n_elem - 1].length - qc->pad_len,
+			       pad_buf, qc->pad_len);
+
+			kunmap_atomic(sg[qc->n_elem - 1].page, KM_IRQ0);
+		}
+	} else {
 		dma_unmap_single(ap->host_set->dev, sg_dma_address(&sg[0]),
 				 sg_dma_len(&sg[0]), dir);
+		if (copy_pad)
+			memcpy(qc->buf_virt + sg->length - qc->pad_len,
+			       pad_buf, qc->pad_len);
+	}
 
 	qc->flags &= ~ATA_QCFLAG_DMAMAP;
 	qc->sg = NULL;
@@ -2211,6 +2235,23 @@ static void ata_fill_sg(struct ata_queue
 		}
 	}
 
+	/* if we added a small buffer, to pad xfer to next 32-bit bound,
+	 * add it to the s/g list here
+	 */
+	if (qc->flags & ATA_QCFLAG_PADDED) {
+		u32 pad_addr = (u32) (ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ));
+		u32 len;
+
+		/* fix up length of last s/g entry */
+		len = le32_to_cpu(ap->prd[idx - 1].flags_len);
+		ap->prd[idx - 1].flags_len = cpu_to_le32(len - qc->pad_len);
+
+		/* append pad entry to s/g list */
+		ap->prd[idx].addr = cpu_to_le32(pad_addr);
+		ap->prd[idx].flags_len = cpu_to_le32(ATA_DMA_PAD_SZ);
+		idx++;
+	}
+
 	if (idx)
 		ap->prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
 }
@@ -2351,6 +2392,21 @@ static int ata_sg_setup_one(struct ata_q
 	int dir = qc->dma_dir;
 	struct scatterlist *sg = qc->sg;
 	dma_addr_t dma_address;
+	unsigned int pad_len;
+
+	/* we must lengthen transfers to end on a 32-bit boundary */
+	pad_len = sg->length & 3;
+	if (pad_len) {
+		void *pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
+		memset(pad_buf, 0, ATA_DMA_PAD_SZ);
+
+		if (qc->tf.flags & ATA_TFLAG_WRITE)
+			memcpy(pad_buf, qc->buf_virt + sg->length - pad_len,
+			       pad_len);
+
+		qc->pad_len = pad_len;
+		qc->flags |= ATA_QCFLAG_PADDED;
+	}
 
 	dma_address = dma_map_single(ap->host_set->dev, qc->buf_virt,
 				     sg->length, dir);
@@ -2385,10 +2441,31 @@ static int ata_sg_setup(struct ata_queue
 	struct ata_port *ap = qc->ap;
 	struct scatterlist *sg = qc->sg;
 	int n_elem, dir;
+	unsigned int pad_len;
 
 	VPRINTK("ENTER, ata%u\n", ap->id);
 	assert(qc->flags & ATA_QCFLAG_SG);
 
+	/* we must lengthen transfers to end on a 32-bit boundary */
+	pad_len = sg[qc->n_elem - 1].length & 3;
+	if (pad_len) {
+		void *pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
+		memset(pad_buf, 0, ATA_DMA_PAD_SZ);
+
+		if (qc->tf.flags & ATA_TFLAG_WRITE) {
+			void *addr = kmap_atomic(sg[qc->n_elem - 1].page, KM_IRQ0);
+			memcpy(pad_buf,
+			       addr + sg[qc->n_elem - 1].offset +
+			         sg[qc->n_elem - 1].length - pad_len,
+			       pad_len);
+
+			kunmap_atomic(sg[qc->n_elem - 1].page, KM_IRQ0);
+		}
+
+		qc->pad_len = pad_len;
+		qc->flags |= ATA_QCFLAG_PADDED;
+	}
+
 	dir = qc->dma_dir;
 	n_elem = dma_map_sg(ap->host_set->dev, sg, qc->n_elem, dir);
 	if (n_elem < 1)
@@ -3672,6 +3749,12 @@ int ata_port_start (struct ata_port *ap)
 	if (!ap->prd)
 		return -ENOMEM;
 
+	ap->pad = dma_alloc_coherent(dev, ATA_DMA_PAD_BUF_SZ, &ap->pad_dma, GFP_KERNEL);
+	if (!ap->pad) {
+		dma_free_coherent(dev, ATA_PRD_TBL_SZ, ap->prd, ap->prd_dma);
+		return -ENOMEM;
+	}
+
 	DPRINTK("prd alloc, virt %p, dma %llx\n", ap->prd, (unsigned long long) ap->prd_dma);
 
 	return 0;
@@ -3694,6 +3777,7 @@ void ata_port_stop (struct ata_port *ap)
 	struct device *dev = ap->host_set->dev;
 
 	dma_free_coherent(dev, ATA_PRD_TBL_SZ, ap->prd, ap->prd_dma);
+	dma_free_coherent(dev, ATA_DMA_PAD_BUF_SZ, ap->pad, ap->pad_dma);
 }
 
 void ata_host_stop (struct ata_host_set *host_set)
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -130,7 +130,7 @@ static Scsi_Host_Template qs_ata_sht = {
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
-	.sg_tablesize		= QS_MAX_PRD,
+	.sg_tablesize		= QS_MAX_PRD - 1,
 	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
@@ -270,10 +270,14 @@ static void qs_fill_sg(struct ata_queued
 	struct qs_port_priv *pp = ap->private_data;
 	unsigned int nelem;
 	u8 *prd = pp->pkt + QS_CPB_BYTES;
+	unsigned int pad_len = 0;
 
 	assert(sg != NULL);
 	assert(qc->n_elem > 0);
 
+	if (qc->flags & ATA_QCFLAG_PADDED)
+		pad_len = qc->pad_len;
+
 	for (nelem = 0; nelem < qc->n_elem; nelem++,sg++) {
 		u64 addr;
 		u32 len;
@@ -283,12 +287,33 @@ static void qs_fill_sg(struct ata_queued
 		prd += sizeof(u64);
 
 		len = sg_dma_len(sg);
+
+		/* fixup last s/g entry, if using pad buffer */
+		if (nelem == (qc->n_elem - 1))
+			len -= pad_len;
+
 		*(__le32 *)prd = cpu_to_le32(len);
 		prd += sizeof(u64);
 
 		VPRINTK("PRD[%u] = (0x%llX, 0x%X)\n", nelem,
 					(unsigned long long)addr, len);
 	}
+
+	/* if we added a small buffer, to pad xfer to next 32-bit bound,
+	 * add it to the s/g list here
+	 */
+	if (qc->flags & ATA_QCFLAG_PADDED) {
+		dma_addr_t pad_addr = ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ);
+		u64 addr = pad_addr;
+
+		*(__le64 *)prd = cpu_to_le64(addr);
+		prd += sizeof(u64);
+
+		*(__le32 *)prd = cpu_to_le32(ATA_DMA_PAD_SZ);
+
+		VPRINTK("PRD[%u] = (0x%llX, 0x%X) [pad PRD]\n", nelem,
+					(unsigned long long)addr, ATA_DMA_PAD_SZ);
+	}
 }
 
 static void qs_qc_prep(struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -452,6 +452,7 @@ static void pdc20621_dma_prep(struct ata
 	unsigned int portno = ap->port_no;
 	unsigned int i, last, idx, total_len = 0, sgt_len;
 	u32 *buf = (u32 *) &pp->dimm_buf[PDC_DIMM_HEADER_SZ];
+	unsigned int pad_len = 0;
 
 	assert(qc->flags & ATA_QCFLAG_DMAMAP);
 
@@ -460,16 +461,37 @@ static void pdc20621_dma_prep(struct ata
 	/* hard-code chip #0 */
 	mmio += PDC_CHIP0_OFS;
 
+	if (qc->flags & ATA_QCFLAG_PADDED)
+		pad_len = qc->pad_len;
+
 	/*
 	 * Build S/G table
 	 */
 	last = qc->n_elem;
 	idx = 0;
 	for (i = 0; i < last; i++) {
+		u32 len;
+
 		buf[idx++] = cpu_to_le32(sg_dma_address(&sg[i]));
-		buf[idx++] = cpu_to_le32(sg_dma_len(&sg[i]));
-		total_len += sg[i].length;
+
+		len = sg_dma_len(&sg[i]);
+		if (i == (last - 1))
+			len -= pad_len;
+
+		buf[idx++] = cpu_to_le32(len);
+		total_len += len;
 	}
+
+	/* if we added a small buffer, to pad xfer to next 32-bit bound,
+	 * add it to the s/g list here
+	 */
+	if (qc->flags & ATA_QCFLAG_PADDED) {
+		u32 pad_addr = (u32) (ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ));
+		buf[idx++] = cpu_to_le32(pad_addr);
+		buf[idx++] = cpu_to_le32(ATA_DMA_PAD_SZ);
+		total_len += ATA_DMA_PAD_SZ;
+	}
+
 	buf[idx - 1] |= cpu_to_le32(ATA_PRD_EOT);
 	sgt_len = idx * 4;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -115,6 +115,7 @@ enum {
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
+	ATA_QCFLAG_PADDED	= (1 << 2), /* xfer padded to 32-bit bound */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
 	ATA_QCFLAG_SINGLE	= (1 << 4), /* no s/g, just a single buffer */
 	ATA_QCFLAG_DMAMAP	= ATA_QCFLAG_SG | ATA_QCFLAG_SINGLE,
@@ -150,6 +151,10 @@ enum {
 	ATA_SHIFT_UDMA		= 0,
 	ATA_SHIFT_MWDMA		= 8,
 	ATA_SHIFT_PIO		= 11,
+
+	/* size of buffer to pad xfers ending on unaligned boundaries */
+	ATA_DMA_PAD_SZ		= 4,
+	ATA_DMA_PAD_BUF_SZ	= ATA_DMA_PAD_SZ * ATA_MAX_QUEUE,
 };
 
 enum pio_task_states {
@@ -236,6 +241,8 @@ struct ata_queued_cmd {
 
 	int			dma_dir;
 
+	unsigned int		pad_len;
+
 	unsigned int		nsect;
 	unsigned int		cursect;
 
@@ -291,6 +298,9 @@ struct ata_port {
 	struct ata_prd		*prd;	 /* our SG list */
 	dma_addr_t		prd_dma; /* and its DMA mapping */
 
+	void			*pad;	/* array of DMA pad buffers */
+	dma_addr_t		pad_dma;
+
 	struct ata_ioports	ioaddr;	/* ATA cmd/ctl/dma register blocks */
 
 	u8			ctl;	/* cache of ATA control register */

Re: [RFC][PATCH] libata ATAPI alignment

[Alan Cox]

On Gwe, 2005-07-29 at 01:06 -0400, Jeff Garzik wrote:
> So, one thing that's terribly ugly about SATA ATAPI is that we need to
> pad DMA transfers to the next 32-bit boundary, if the length is not
> evenly divisible by 4.

Looks good and avoids the special case leaking into the core code.

> Complicating matters, we currently must support two methods of data
> buffer submission:  a single kernel virtual address, or a struct
> scatterlist.

For the moment - also you turn the single buffer into a one entry sglist
so its not to bad.

> Review is requested by any and all parties, as well as suggestions for
> a prettier approach.

I'd pull the code into seperate functions but thats my only real
comment.

Re: [RFC][PATCH] libata ATAPI alignment

[Jens Axboe]

On Fri, Jul 29 2005, Jeff Garzik wrote:
> 
> So, one thing that's terribly ugly about SATA ATAPI is that we need to
> pad DMA transfers to the next 32-bit boundary, if the length is not
> evenly divisible by 4.
> 
> Messing with the scatterlist to accomplish this is terribly ugly
> no matter how you slice it.  One way would be to create my own
> scatterlist, via memcpy and then manual labor.  Another way would be
> to special case a pad buffer, appending it onto the end of various
> scatterlist code.

It's not pretty, but I think it's the only solution currently.

> Complicating matters, we currently must support two methods of data
> buffer submission:  a single kernel virtual address, or a struct
> scatterlist.

Fairly soon the !use_sg case will be gone, at least coming from SCSI. I
hope we can completely get away from the virtual address + length for
any remaining cases, just making it a single entry sg list.

> 
> Review is requested by any and all parties, as well as suggestions for
> a prettier approach.
> 
> This is one of the last steps needed to get ATAPI going.
> 
> 
> 
> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
> --- a/drivers/scsi/ahci.c
> +++ b/drivers/scsi/ahci.c
> @@ -44,7 +44,7 @@
>  
>  enum {
>  	AHCI_PCI_BAR		= 5,
> -	AHCI_MAX_SG		= 168, /* hardware max is 64K */
> +	AHCI_MAX_SG		= 300, /* hardware max is 64K */
>  	AHCI_DMA_BOUNDARY	= 0xffffffff,
>  	AHCI_USE_CLUSTERING	= 0,
>  	AHCI_CMD_SLOT_SZ	= 32 * 32,

Reasoning? I agree, just wondering... How big is the allocated area now?

-- 
Jens Axboe

Re: [RFC][PATCH] libata ATAPI alignment

[Jeff Garzik]

Jens Axboe wrote:
> On Fri, Jul 29 2005, Jeff Garzik wrote:
>>diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
>>--- a/drivers/scsi/ahci.c
>>+++ b/drivers/scsi/ahci.c
>>@@ -44,7 +44,7 @@
>> 
>> enum {
>> 	AHCI_PCI_BAR		= 5,
>>-	AHCI_MAX_SG		= 168, /* hardware max is 64K */
>>+	AHCI_MAX_SG		= 300, /* hardware max is 64K */
>> 	AHCI_DMA_BOUNDARY	= 0xffffffff,
>> 	AHCI_USE_CLUSTERING	= 0,
>> 	AHCI_CMD_SLOT_SZ	= 32 * 32,
> 
> 
> Reasoning? I agree, just wondering... How big is the allocated area now?

168 kept the entire allocated DMA area to 4K.

300 increases that to <I don't care>K.  ;-)

	Jeff

Re: [RFC][PATCH] libata ATAPI alignment

[Tejun Heo]

On Fri, Jul 29, 2005 at 01:06:54AM -0400, Jeff Garzik wrote:
> 
> So, one thing that's terribly ugly about SATA ATAPI is that we need to
> pad DMA transfers to the next 32-bit boundary, if the length is not
> evenly divisible by 4.
> 
> Messing with the scatterlist to accomplish this is terribly ugly
> no matter how you slice it.  One way would be to create my own
> scatterlist, via memcpy and then manual labor.  Another way would be
> to special case a pad buffer, appending it onto the end of various
> scatterlist code.
> 
> Complicating matters, we currently must support two methods of data
> buffer submission:  a single kernel virtual address, or a struct
> scatterlist.
> 
> Review is requested by any and all parties, as well as suggestions for
> a prettier approach.
> 
> This is one of the last steps needed to get ATAPI going.
> 

 Hello, Jeff, Jens & Alan.

 I've rewritten the patch to fix some bugs and make it a bit (IMHO)
simpler to use.

 Bug fixes...

 * When copying a sg, original implementation just kmap'ed sg->page
   which can cause trouble as a sg can span more than a page.

 * In ata_sg_clean(), the original implementation accesses last sg by
   indexing w/ (qc->n_elem - 1).  This is incorrect as qc->n_elem
   could have been shrunk by dma_map_sg() in ata_sg_setup().

 * In the original code (before Jeff's patch), sata_sx4 used
   sg[i].legnth to calculate total_len.  This is wrong for the same
   reason as above and Jeff's patch fixes it.  I separated out this
   fix just to clarify.

 Changes...

 * Instead of adding pad sg handling to each fill_sg function,
   ata_for_each_sg() macro is added.  Normal sg entries and
   qc->pad_sgent are setup by sg_setup routines and fill_sg functions
   can just iterate w/ ata_for_each_sg() without caring about padding.

 * hw_max_segments is automatically decremented in slave_config if
   attached device is ATAPI.

 Questions/suggestions...

 * I didn't include AHCI_MAX_SG change as it looked a bit more out of
   place w/ other changes to ahci gone.  Also, I'm curious about how
   meaningful increasing AHCI_MAX_SG is.  We're currently setting
   max_phys_segments to LIBATA_MAX_PRD, which is 128, and AFAIK max hw
   segments higher than phys segments is meaningless (phys segs are
   merged into hw segs and one phys segment cannot be splitted into
   two hw segs).  Am I missing something here?

 * About core routines/callbacks.  Currently, libata-core file
   contains actual libata core routines and all helper functions for
   taskfile controllers.  ata_ prefix is also shared by both function
   categories.  This is a bit confusing, IMO.

   I think ata_port_start should allocate stuff necessary for libata
   core and call ->port_start callback and similary for ata_port_stop,
   and current ata_port_start/stop need to be renamed to something
   like ata_tf_port_start/stop, such that we don't have to allocate
   data structures needed by libata core in specific drivers (ahci in
   this case).

 I've tested both sg and non-sg paths with sg_test_rwbuf.  When
testing sg path, I've commented out sg.c:L1951 (sg_build_indirect:L10)
to prevent it padding transfer size to 512byte boundary.  Read/write
padding in both paths work.

 Two patches will follow this mail.  The first one fixes sata_sx4 as
mentioned above and the second implements atapi align.

 Thanks.

--
tejun

Re: [PATCH 1/2] sata: fix sata_sx4 dma_prep to not use sg->length

[Tejun Heo]

 sata_sx4 directly references sg->length to calculate total_len in
pdc20621_dma_prep().  This is incorrect as dma_map_sg() could have
merged multiple sg's into one and, in such case, sg->length doesn't
reflect true size of the entry.  This patch makes it use
sg_dma_len(sg).


Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/sata_sx4.c
===================================================================
--- work.orig/drivers/scsi/sata_sx4.c	2005-08-07 14:07:17.000000000 +0900
+++ work/drivers/scsi/sata_sx4.c	2005-08-07 14:08:25.000000000 +0900
@@ -468,7 +468,7 @@ static void pdc20621_dma_prep(struct ata
 	for (i = 0; i < last; i++) {
 		buf[idx++] = cpu_to_le32(sg_dma_address(&sg[i]));
 		buf[idx++] = cpu_to_le32(sg_dma_len(&sg[i]));
-		total_len += sg[i].length;
+		total_len += sg_dma_len(&sg[i]);
 	}
 	buf[idx - 1] |= cpu_to_le32(ATA_PRD_EOT);
 	sgt_len = idx * 4;

Rd: [PATCH 2/2] sata: implement ATAPI alignment adjustment

[Tejun Heo]

 SATA ATAPI is nasty in that it requires that all transfers should be
sized in multiples of 4 bytes.  This patch implements ATAPI 4-byte
alignment by mangling sg table and using an extra pad_sgent.  Setups
and cleanups are all done inside libata core layer and the only
requirement on specific drivers is using ata_for_each_sg() to iterate
over sg table.  This patch also renames qc->sg to qc->__sg to signify
that it cannot be iterated directly.

 This patch is a rewrite of Jeff's implementation.


Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/ahci.c
===================================================================
--- work.orig/drivers/scsi/ahci.c	2005-08-07 14:07:17.000000000 +0900
+++ work/drivers/scsi/ahci.c	2005-08-07 14:09:21.000000000 +0900
@@ -313,8 +313,15 @@ static int ahci_port_start(struct ata_po
 		return -ENOMEM;
 	memset(pp, 0, sizeof(*pp));
 
+	ap->pad = dma_alloc_coherent(dev, ATA_DMA_PAD_BUF_SZ, &ap->pad_dma, GFP_KERNEL);
+	if (!ap->pad) {
+		kfree(pp);
+		return -ENOMEM;
+	}
+
 	mem = dma_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma, GFP_KERNEL);
 	if (!mem) {
+		dma_free_coherent(dev, ATA_DMA_PAD_BUF_SZ, ap->pad, ap->pad_dma);
 		kfree(pp);
 		return -ENOMEM;
 	}
@@ -390,6 +397,7 @@ static void ahci_port_stop(struct ata_po
 	ap->private_data = NULL;
 	dma_free_coherent(dev, AHCI_PORT_PRIV_DMA_SZ,
 			  pp->cmd_slot, pp->cmd_slot_dma);
+	dma_free_coherent(dev, ATA_DMA_PAD_BUF_SZ, ap->pad, ap->pad_dma);
 	kfree(pp);
 }
 
@@ -475,23 +483,23 @@ static void ahci_tf_read(struct ata_port
 static void ahci_fill_sg(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;
-	unsigned int i;
+	struct scatterlist *sg;
+	struct ahci_sg *ahci_sg;
 
 	VPRINTK("ENTER\n");
 
 	/*
 	 * Next, the S/G list.
 	 */
-	for (i = 0; i < qc->n_elem; i++) {
-		u32 sg_len;
-		dma_addr_t addr;
-
-		addr = sg_dma_address(&qc->sg[i]);
-		sg_len = sg_dma_len(&qc->sg[i]);
-
-		pp->cmd_tbl_sg[i].addr = cpu_to_le32(addr & 0xffffffff);
-		pp->cmd_tbl_sg[i].addr_hi = cpu_to_le32((addr >> 16) >> 16);
-		pp->cmd_tbl_sg[i].flags_size = cpu_to_le32(sg_len - 1);
+	ahci_sg = pp->cmd_tbl_sg;
+	ata_for_each_sg(sg, qc) {
+		dma_addr_t addr = sg_dma_address(sg);
+		u32 sg_len = sg_dma_len(sg);
+
+		ahci_sg->addr = cpu_to_le32(addr & 0xffffffff);
+		ahci_sg->addr_hi = cpu_to_le32((addr >> 16) >> 16);
+		ahci_sg->flags_size = cpu_to_le32(sg_len - 1);
+		ahci_sg++;
 	}
 }
 
Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-08-07 14:07:17.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-08-07 14:09:21.000000000 +0900
@@ -2142,8 +2142,9 @@ static void ata_dev_set_xfermode(struct 
 static void ata_sg_clean(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	struct scatterlist *sg = qc->sg;
+	struct scatterlist *sg = qc->__sg;
 	int dir = qc->dma_dir;
+	void *pad_buf = NULL;
 
 	assert(qc->flags & ATA_QCFLAG_DMAMAP);
 	assert(sg != NULL);
@@ -2153,14 +2154,35 @@ static void ata_sg_clean(struct ata_queu
 
 	DPRINTK("unmapping %u sg elements\n", qc->n_elem);
 
-	if (qc->flags & ATA_QCFLAG_SG)
+	/* if we padded the buffer out to 32-bit bound, and data
+	 * xfer direction is from-device, we must copy from the
+	 * pad buffer back into the supplied buffer
+	 */
+	if (qc->pad_len && !(qc->tf.flags & ATA_TFLAG_WRITE))
+		pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
+
+	if (qc->flags & ATA_QCFLAG_SG) {
 		dma_unmap_sg(ap->host_set->dev, sg, qc->n_elem, dir);
-	else
+		/* restore last sg */
+		sg[qc->orig_n_elem - 1].length += qc->pad_len;
+		if (pad_buf) {
+			struct scatterlist *psg = &qc->pad_sgent;
+			void *addr = kmap_atomic(psg->page, KM_IRQ0);
+			memcpy(addr + psg->offset, pad_buf, qc->pad_len);
+			kunmap_atomic(psg->page, KM_IRQ0);
+		}
+	} else {
 		dma_unmap_single(ap->host_set->dev, sg_dma_address(&sg[0]),
 				 sg_dma_len(&sg[0]), dir);
+		/* restore sg */
+		sg->length += qc->pad_len;
+		if (pad_buf)
+			memcpy(qc->buf_virt + sg->length - qc->pad_len,
+			       pad_buf, qc->pad_len);
+	}
 
 	qc->flags &= ~ATA_QCFLAG_DMAMAP;
-	qc->sg = NULL;
+	qc->__sg = NULL;
 }
 
 /**
@@ -2176,15 +2198,15 @@ static void ata_sg_clean(struct ata_queu
  */
 static void ata_fill_sg(struct ata_queued_cmd *qc)
 {
-	struct scatterlist *sg = qc->sg;
 	struct ata_port *ap = qc->ap;
-	unsigned int idx, nelem;
+	struct scatterlist *sg;
+	unsigned int idx;
 
-	assert(sg != NULL);
+	assert(qc->__sg != NULL);
 	assert(qc->n_elem > 0);
 
 	idx = 0;
-	for (nelem = qc->n_elem; nelem; nelem--,sg++) {
+	ata_for_each_sg(sg, qc) {
 		u32 addr, offset;
 		u32 sg_len, len;
 
@@ -2288,11 +2310,12 @@ void ata_sg_init_one(struct ata_queued_c
 	qc->flags |= ATA_QCFLAG_SINGLE;
 
 	memset(&qc->sgent, 0, sizeof(qc->sgent));
-	qc->sg = &qc->sgent;
+	qc->__sg = &qc->sgent;
 	qc->n_elem = 1;
+	qc->orig_n_elem = 1;
 	qc->buf_virt = buf;
 
-	sg = qc->sg;
+	sg = qc->__sg;
 	sg->page = virt_to_page(buf);
 	sg->offset = (unsigned long) buf & ~PAGE_MASK;
 	sg->length = buflen;
@@ -2328,8 +2351,9 @@ void ata_sg_init(struct ata_queued_cmd *
 		 unsigned int n_elem)
 {
 	qc->flags |= ATA_QCFLAG_SG;
-	qc->sg = sg;
+	qc->__sg = sg;
 	qc->n_elem = n_elem;
+	qc->orig_n_elem = n_elem;
 }
 
 /**
@@ -2349,9 +2373,32 @@ static int ata_sg_setup_one(struct ata_q
 {
 	struct ata_port *ap = qc->ap;
 	int dir = qc->dma_dir;
-	struct scatterlist *sg = qc->sg;
+	struct scatterlist *sg = qc->__sg;
 	dma_addr_t dma_address;
 
+	/* we must lengthen transfers to end on a 32-bit boundary */
+	qc->pad_len = sg->length & 3;
+	if (qc->pad_len) {
+		void *pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
+		struct scatterlist *psg = &qc->pad_sgent;
+
+		assert(qc->dev->class == ATA_DEV_ATAPI);
+
+		memset(pad_buf, 0, ATA_DMA_PAD_SZ);
+
+		if (qc->tf.flags & ATA_TFLAG_WRITE)
+			memcpy(pad_buf, qc->buf_virt + sg->length - qc->pad_len,
+			       qc->pad_len);
+
+		sg_dma_address(psg) = ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ);
+		sg_dma_len(psg) = ATA_DMA_PAD_SZ;
+		/* trim sg */
+		sg->length -= qc->pad_len;
+
+		DPRINTK("padding done, sg->length=%u pad_len=%u\n",
+			sg->length, qc->pad_len);
+	}
+
 	dma_address = dma_map_single(ap->host_set->dev, qc->buf_virt,
 				     sg->length, dir);
 	if (dma_mapping_error(dma_address))
@@ -2383,12 +2430,47 @@ static int ata_sg_setup_one(struct ata_q
 static int ata_sg_setup(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	struct scatterlist *sg = qc->sg;
+	struct scatterlist *sg = qc->__sg;
+	struct scatterlist *lsg = &sg[qc->n_elem - 1];
 	int n_elem, dir;
 
 	VPRINTK("ENTER, ata%u\n", ap->id);
 	assert(qc->flags & ATA_QCFLAG_SG);
 
+	/* we must lengthen transfers to end on a 32-bit boundary */
+	qc->pad_len = lsg->length & 3;
+	if (qc->pad_len) {
+		void *pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
+		struct scatterlist *psg = &qc->pad_sgent;
+		unsigned int offset;
+
+		assert(qc->dev->class == ATA_DEV_ATAPI);
+
+		memset(pad_buf, 0, ATA_DMA_PAD_SZ);
+
+		/*
+		 * psg->page/offset are used to copy to-be-written
+		 * data in this function or read data in ata_sg_clean.
+		 */
+		offset = lsg->offset + lsg->length - qc->pad_len;
+		psg->page = nth_page(lsg->page, offset >> PAGE_SHIFT);
+		psg->offset = offset_in_page(offset);
+
+		if (qc->tf.flags & ATA_TFLAG_WRITE) {
+			void *addr = kmap_atomic(psg->page, KM_IRQ0);
+			memcpy(pad_buf, addr + psg->offset, qc->pad_len);
+			kunmap_atomic(psg->page, KM_IRQ0);
+		}
+
+		sg_dma_address(psg) = ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ);
+		sg_dma_len(psg) = ATA_DMA_PAD_SZ;
+		/* trim last sg */
+		lsg->length -= qc->pad_len;
+
+		DPRINTK("padding done, sg[%d].length=%u pad_len=%u\n",
+			qc->n_elem - 1, lsg->length, qc->pad_len);
+	}
+
 	dir = qc->dma_dir;
 	n_elem = dma_map_sg(ap->host_set->dev, sg, qc->n_elem, dir);
 	if (n_elem < 1)
@@ -2559,7 +2641,7 @@ static void ata_data_xfer(struct ata_por
 static void ata_pio_sector(struct ata_queued_cmd *qc)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
-	struct scatterlist *sg = qc->sg;
+	struct scatterlist *sg = qc->__sg;
 	struct ata_port *ap = qc->ap;
 	struct page *page;
 	unsigned int offset;
@@ -2597,7 +2679,7 @@ static void ata_pio_sector(struct ata_qu
 static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
-	struct scatterlist *sg = qc->sg;
+	struct scatterlist *sg = qc->__sg;
 	struct ata_port *ap = qc->ap;
 	struct page *page;
 	unsigned char *buf;
@@ -2607,7 +2689,7 @@ static void __atapi_pio_bytes(struct ata
 		ap->pio_task_state = PIO_ST_LAST;
 
 next_sg:
-	sg = &qc->sg[qc->cursg];
+	sg = &qc->__sg[qc->cursg];
 
 	page = sg->page;
 	offset = sg->offset + qc->cursg_ofs;
@@ -2997,7 +3079,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 
 	qc = ata_qc_new(ap);
 	if (qc) {
-		qc->sg = NULL;
+		qc->__sg = NULL;
 		qc->flags = 0;
 		qc->scsicmd = NULL;
 		qc->ap = ap;
@@ -3672,6 +3754,12 @@ int ata_port_start (struct ata_port *ap)
 	if (!ap->prd)
 		return -ENOMEM;
 
+	ap->pad = dma_alloc_coherent(dev, ATA_DMA_PAD_BUF_SZ, &ap->pad_dma, GFP_KERNEL);
+	if (!ap->pad) {
+		dma_free_coherent(dev, ATA_PRD_TBL_SZ, ap->prd, ap->prd_dma);
+		return -ENOMEM;
+	}
+
 	DPRINTK("prd alloc, virt %p, dma %llx\n", ap->prd, (unsigned long long) ap->prd_dma);
 
 	return 0;
@@ -3694,6 +3782,7 @@ void ata_port_stop (struct ata_port *ap)
 	struct device *dev = ap->host_set->dev;
 
 	dma_free_coherent(dev, ATA_PRD_TBL_SZ, ap->prd, ap->prd_dma);
+	dma_free_coherent(dev, ATA_DMA_PAD_BUF_SZ, ap->pad, ap->pad_dma);
 }
 
 void ata_host_stop (struct ata_host_set *host_set)
Index: work/drivers/scsi/libata-scsi.c
===================================================================
--- work.orig/drivers/scsi/libata-scsi.c	2005-08-07 14:07:17.000000000 +0900
+++ work/drivers/scsi/libata-scsi.c	2005-08-07 14:09:21.000000000 +0900
@@ -139,10 +139,10 @@ struct ata_queued_cmd *ata_scsi_qc_new(s
 		qc->scsidone = done;
 
 		if (cmd->use_sg) {
-			qc->sg = (struct scatterlist *) cmd->request_buffer;
+			qc->__sg = (struct scatterlist *) cmd->request_buffer;
 			qc->n_elem = cmd->use_sg;
 		} else {
-			qc->sg = &qc->sgent;
+			qc->__sg = &qc->sgent;
 			qc->n_elem = 1;
 		}
 	} else {
@@ -353,6 +353,16 @@ int ata_scsi_slave_config(struct scsi_de
 			 */
 			blk_queue_max_sectors(sdev->request_queue, 2048);
 		}
+
+		/*
+		 * SATA DMA transfers must be multiples of 4 byte, so
+		 * we need to pad ATAPI transfers using an extra sg.
+		 * Decrement max hw segments accordingly.
+		 */
+		if (dev->class == ATA_DEV_ATAPI) {
+			request_queue_t *q = sdev->request_queue;
+			blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
+		}
 	}
 
 	return 0;	/* scsi layer doesn't check return value, sigh */
Index: work/drivers/scsi/sata_qstor.c
===================================================================
--- work.orig/drivers/scsi/sata_qstor.c	2005-08-07 14:07:17.000000000 +0900
+++ work/drivers/scsi/sata_qstor.c	2005-08-07 14:09:21.000000000 +0900
@@ -265,16 +265,17 @@ static void qs_scr_write (struct ata_por
 
 static void qs_fill_sg(struct ata_queued_cmd *qc)
 {
-	struct scatterlist *sg = qc->sg;
+	struct scatterlist *sg;
 	struct ata_port *ap = qc->ap;
 	struct qs_port_priv *pp = ap->private_data;
 	unsigned int nelem;
 	u8 *prd = pp->pkt + QS_CPB_BYTES;
 
-	assert(sg != NULL);
+	assert(qc->__sg != NULL);
 	assert(qc->n_elem > 0);
 
-	for (nelem = 0; nelem < qc->n_elem; nelem++,sg++) {
+	nelem = 0;
+	ata_for_each_sg(sg, qc) {
 		u64 addr;
 		u32 len;
 
@@ -288,6 +289,7 @@ static void qs_fill_sg(struct ata_queued
 
 		VPRINTK("PRD[%u] = (0x%llX, 0x%X)\n", nelem,
 					(unsigned long long)addr, len);
+		nelem++;
 	}
 }
 
Index: work/drivers/scsi/sata_sx4.c
===================================================================
--- work.orig/drivers/scsi/sata_sx4.c	2005-08-07 14:08:25.000000000 +0900
+++ work/drivers/scsi/sata_sx4.c	2005-08-07 14:10:01.000000000 +0900
@@ -443,14 +443,14 @@ static inline void pdc20621_host_pkt(str
 
 static void pdc20621_dma_prep(struct ata_queued_cmd *qc)
 {
-	struct scatterlist *sg = qc->sg;
+	struct scatterlist *sg;
 	struct ata_port *ap = qc->ap;
 	struct pdc_port_priv *pp = ap->private_data;
 	void *mmio = ap->host_set->mmio_base;
 	struct pdc_host_priv *hpriv = ap->host_set->private_data;
 	void *dimm_mmio = hpriv->dimm_mmio;
 	unsigned int portno = ap->port_no;
-	unsigned int i, last, idx, total_len = 0, sgt_len;
+	unsigned int i, idx, total_len = 0, sgt_len;
 	u32 *buf = (u32 *) &pp->dimm_buf[PDC_DIMM_HEADER_SZ];
 
 	assert(qc->flags & ATA_QCFLAG_DMAMAP);
@@ -463,12 +463,11 @@ static void pdc20621_dma_prep(struct ata
 	/*
 	 * Build S/G table
 	 */
-	last = qc->n_elem;
 	idx = 0;
-	for (i = 0; i < last; i++) {
-		buf[idx++] = cpu_to_le32(sg_dma_address(&sg[i]));
-		buf[idx++] = cpu_to_le32(sg_dma_len(&sg[i]));
-		total_len += sg_dma_len(&sg[i]);
+	ata_for_each_sg(sg, qc) {
+		buf[idx++] = cpu_to_le32(sg_dma_address(sg));
+		buf[idx++] = cpu_to_le32(sg_dma_len(sg));
+		total_len += sg_dma_len(sg);
 	}
 	buf[idx - 1] |= cpu_to_le32(ATA_PRD_EOT);
 	sgt_len = idx * 4;
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-08-07 14:07:17.000000000 +0900
+++ work/include/linux/libata.h	2005-08-07 14:09:21.000000000 +0900
@@ -150,6 +150,10 @@ enum {
 	ATA_SHIFT_UDMA		= 0,
 	ATA_SHIFT_MWDMA		= 8,
 	ATA_SHIFT_PIO		= 11,
+
+	/* size of buffer to pad xfers ending on unaligned boundaries */
+	ATA_DMA_PAD_SZ		= 4,
+	ATA_DMA_PAD_BUF_SZ	= ATA_DMA_PAD_SZ * ATA_MAX_QUEUE,
 };
 
 enum pio_task_states {
@@ -233,9 +237,12 @@ struct ata_queued_cmd {
 	unsigned long		flags;		/* ATA_QCFLAG_xxx */
 	unsigned int		tag;
 	unsigned int		n_elem;
+	unsigned int		orig_n_elem;
 
 	int			dma_dir;
 
+	unsigned int		pad_len;
+
 	unsigned int		nsect;
 	unsigned int		cursect;
 
@@ -246,9 +253,11 @@ struct ata_queued_cmd {
 	unsigned int		cursg_ofs;
 
 	struct scatterlist	sgent;
+	struct scatterlist	pad_sgent;
 	void			*buf_virt;
 
-	struct scatterlist	*sg;
+	/* DO NOT iterate over __sg manually, use ata_for_each_sg() */
+	struct scatterlist	*__sg;
 
 	ata_qc_cb_t		complete_fn;
 
@@ -291,6 +300,9 @@ struct ata_port {
 	struct ata_prd		*prd;	 /* our SG list */
 	dma_addr_t		prd_dma; /* and its DMA mapping */
 
+	void			*pad;	/* array of DMA pad buffers */
+	dma_addr_t		pad_dma;
+
 	struct ata_ioports	ioaddr;	/* ATA cmd/ctl/dma register blocks */
 
 	u8			ctl;	/* cache of ATA control register */
@@ -452,6 +464,19 @@ extern int pci_test_config_bits(struct p
 #endif /* CONFIG_PCI */
 
 
+static inline struct scatterlist *
+ata_qc_next_sg(struct scatterlist *sg, struct ata_queued_cmd *qc)
+{
+	if (sg == &qc->pad_sgent)
+		return NULL;
+	if (++sg - qc->__sg < qc->n_elem)
+		return sg;
+	return qc->pad_len ? &qc->pad_sgent : NULL;
+}
+
+#define ata_for_each_sg(sg, qc) \
+	for (sg = qc->__sg; sg; sg = ata_qc_next_sg(sg, qc))
+
 static inline unsigned int ata_tag_valid(unsigned int tag)
 {
 	return (tag < ATA_MAX_QUEUE) ? 1 : 0;

Re: [PATCH 3] sata: restore sg on setup failure

[Tejun Heo]

 I forgot to restore sg->length on setup failure.  This patch adds it.


Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-08-07 15:13:20.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-08-07 15:15:27.000000000 +0900
@@ -2401,8 +2401,11 @@ static int ata_sg_setup_one(struct ata_q
 
 	dma_address = dma_map_single(ap->host_set->dev, qc->buf_virt,
 				     sg->length, dir);
-	if (dma_mapping_error(dma_address))
+	if (dma_mapping_error(dma_address)) {
+		/* restore sg */
+		sg->length += qc->pad_len;
 		return -1;
+	}
 
 	sg_dma_address(sg) = dma_address;
 	sg_dma_len(sg) = sg->length;
@@ -2473,8 +2476,11 @@ static int ata_sg_setup(struct ata_queue
 
 	dir = qc->dma_dir;
 	n_elem = dma_map_sg(ap->host_set->dev, sg, qc->n_elem, dir);
-	if (n_elem < 1)
+	if (n_elem < 1) {
+		/* restore last sg */
+		lsg->length += qc->pad_len;
 		return -1;
+	}
 
 	DPRINTK("%d sg elements mapped\n", n_elem);
 

Re: [PATCH 1/2] sata: fix sata_sx4 dma_prep to not use sg->length

[Jeff Garzik]

Tejun Heo wrote:
>  sata_sx4 directly references sg->length to calculate total_len in
> pdc20621_dma_prep().  This is incorrect as dma_map_sg() could have
> merged multiple sg's into one and, in such case, sg->length doesn't
> reflect true size of the entry.  This patch makes it use
> sg_dma_len(sg).
> 
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied to 2.6.13

libata error handling

[Jeff Garzik]

Tejun,

In an email I cannot find anymore, you asked why I was interested in 
converting libata to use the fine-grained EH hooks in the SCSI layer, 
rather than continued with the current ->eh_strategy_handler() method.

Several reasons:

1) The fine-grained hooks of the SCSI layer are somewhat standard for 
block devices.  The events they signify -- timeout, abort cmd, dev 
reset, bus reset, and host reset -- map precisely to the events that we 
must deal with at the ATA level.

But be warned of false sharing, as I talk about in #2...

2) When libata SAT translation layer becomes optional, and libata drives 
a "true" block device, use of ->eh_strategy_handler() will actually be 
an obstacle due to false sharing of code paths.  ->eh_strategy_handler() 
is indeed a single "do it all" EH entrypoint, but within that entrypoint 
you must perform several SCSI-specific tasks.

3) ->eh_strategy_handler() has continually proven to be a method of 
error handling poorly supported by the SCSI layer.  There are many 
assumption coded into the SCSI layer that this is -not- the path taken 
by LLD EH code, and libata must constantly work around these assumptions.

4) libata is the -only- user of ->eh_strategy_handler(), and oddballs 
must be stomped out.  It creates a maintenance burden on the SCSI layer 
that should be eliminated.

Re: libata error handling

[Tejun Heo]

  Hi, Jeff.

Jeff Garzik wrote:
> 
> Tejun,
> 
> In an email I cannot find anymore, you asked why I was interested in 
> converting libata to use the fine-grained EH hooks in the SCSI layer, 
> rather than continued with the current ->eh_strategy_handler() method.
> 
> Several reasons:
> 
> 1) The fine-grained hooks of the SCSI layer are somewhat standard for 
> block devices.  The events they signify -- timeout, abort cmd, dev 
> reset, bus reset, and host reset -- map precisely to the events that we 
> must deal with at the ATA level.

  I genearally agree that the events are somewhat standard for block 
devices but IMHO SCSI EH also has fair amount SCSI-specific assumptions 
and ATA is a bit too different from SCSI to fit cleanly into it.  For 
example, when handling NCQ errors, the whole task set is aborted and the 
status is retrieved with read log page.  This can be worked around in 
one of the hooks and emulate SCSI behavior, but it just doesn't really 
fit well.  And I think that recovering via translation layer is a bit 
too much translation.

  So, my thought is that SCSI EH assumptions are a bit too specific to 
be used as standard for block devices.

> But be warned of false sharing, as I talk about in #2...
> 
> 2) When libata SAT translation layer becomes optional, and libata drives 
> a "true" block device, use of ->eh_strategy_handler() will actually be 
> an obstacle due to false sharing of code paths.  ->eh_strategy_handler() 
> is indeed a single "do it all" EH entrypoint, but within that entrypoint 
> you must perform several SCSI-specific tasks.

  It's true that we must do SCSI specific tasks inside libata if we use 
eh_strategy_handler but I don't think switching to fine-grained EH will 
reduce the amount of SCSI-specific things inside libata.  I think as 
long as we can insulate LLDD's from SCSI layer, either way should be 
okay later.

> 
> 3) ->eh_strategy_handler() has continually proven to be a method of 
> error handling poorly supported by the SCSI layer.  There are many 
> assumption coded into the SCSI layer that this is -not- the path taken 
> by LLD EH code, and libata must constantly work around these assumptions.
> 
> 4) libata is the -only- user of ->eh_strategy_handler(), and oddballs 
> must be stomped out.  It creates a maintenance burden on the SCSI layer 
> that should be eliminated.

  I agree that being the only user does incur difficulties, but my very 
subjective feeling is that the original libata EH implementation was 
just a bit too fragile to start with.  eg. not grabbing host lock on EH 
entrance causing command completion vs. EH handling race and handling 
errors in several different ways.

  Heh... Maybe I'm just reluctant to let go of my patches.  Anyways, 
I'll now stand down and see how things go and try to help.

  Thanks, always.

-- 
tejun

Re: libata error handling

[Jeff Garzik]

Tejun Heo wrote:
>  Heh... Maybe I'm just reluctant to let go of my patches.  Anyways, I'll 
> now stand down and see how things go and try to help.


Note that my email simply describes a long term target.  For the short 
term, and perhaps medium term, libata will continue to use 
->eh_strategy_handler().

Given Mark's messages, my own knowledge, and other reports, there 
continues to be room for improvement in the current EH code.

In general, we need to distinguish between PCI bus errors, SATA bus 
errors, and ATA device errors, and handle each error class 
appropriately.  In the SCSI layer, ->eh_strategy_handler() or no, this 
will likely consist of taking the SCSI device offline and dealing with 
the error(s).

	Jeff

Re: libata error handling

[Luben Tuikov]

On 08/18/05 23:49, Jeff Garzik wrote:
> 1) The fine-grained hooks of the SCSI layer are somewhat standard for 
> block devices.  The events they signify -- timeout, abort cmd, dev 
> reset, bus reset, and host reset -- map precisely to the events that we 
> must deal with at the ATA level.

"dev reset, bus reset" -- non existant, as I'm sure you're aware of,
depending on what _transport_ you use. ;-)

> 2) When libata SAT translation layer becomes optional, and libata drives 
> a "true" block device,

Yes, this will be very cool! (when (S)ATA(PI) devices become true block
devices.

> use of ->eh_strategy_handler() will actually be 
> an obstacle due to false sharing of code paths.  ->eh_strategy_handler() 

I fully agree.

> is indeed a single "do it all" EH entrypoint, but within that entrypoint 
> you must perform several SCSI-specific tasks.
> 
> 3) ->eh_strategy_handler() has continually proven to be a method of 
> error handling poorly supported by the SCSI layer.  There are many 
> assumption coded into the SCSI layer that this is -not- the path taken 
> by LLD EH code, and libata must constantly work around these assumptions.

I agree.

> 
> 4) libata is the -only- user of ->eh_strategy_handler(), and oddballs 

Not any more ;-)

Using the command time out hook and the strategy routine, gives _complete_
control over host recovery, and I really do mean _complete_.

	Luben


> must be stomped out.  It creates a maintenance burden on the SCSI layer 
> that should be eliminated.
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: libata error handling

[Luben Tuikov]

On 08/19/05 01:40, Tejun Heo wrote:
>   I genearally agree that the events are somewhat standard for block 
> devices but IMHO SCSI EH also has fair amount SCSI-specific assumptions 
> and ATA is a bit too different from SCSI to fit cleanly into it.  For 
> example, when handling NCQ errors, the whole task set is aborted and the 
> status is retrieved with read log page.  This can be worked around in 
> one of the hooks and emulate SCSI behavior, but it just doesn't really 
> fit well.  And I think that recovering via translation layer is a bit 
> too much translation.
> 
>   So, my thought is that SCSI EH assumptions are a bit too specific to 
> be used as standard for block devices.

Ok, so everyone seems to agree on this.
 
>   It's true that we must do SCSI specific tasks inside libata if we use 
> eh_strategy_handler but I don't think switching to fine-grained EH will 
> reduce the amount of SCSI-specific things inside libata.  I think as 
> long as we can insulate LLDD's from SCSI layer, either way should be 
> okay later.

True, this is the goal.  Separation between device management
and how that device got to you, is the future and should be the
a goal.

>   I agree that being the only user does incur difficulties, but my very 
> subjective feeling is that the original libata EH implementation was 
> just a bit too fragile to start with.  eg. not grabbing host lock on EH 
> entrance causing command completion vs. EH handling race and handling 
> errors in several different ways.
> 
>   Heh... Maybe I'm just reluctant to let go of my patches.  Anyways, 
> I'll now stand down and see how things go and try to help.

Please don't do that.  One thing everyone in the Linux community knows
is that Linux-SCSI needs fresh minds and fresh ideas.  Especially from
knowlegable folks in the storage protocols and standards.

	Luben

Re: libata error handling

[Patrick Mansfield]

On Fri, Aug 19, 2005 at 02:46:35PM -0400, Luben Tuikov wrote:

> 
> Using the command time out hook and the strategy routine, gives _complete_
> control over host recovery, and I really do mean _complete_.
> 

I assume you mean hostt->eh_timed_out.

Is anyone implmenting (or has implemented) a ->eh_timed_out function? I see
none in mainline kernel.

I was looking at using it in an LLDD, but hit two problems, and have
started to work on an alternate approach of cancelling (aborting or wtf you
want to call it) a list of commands in the eh thread.

The two problems I see with the hook are:

It calls the driver in interrupt context, so the called function can't
sleep.

There is no queueing or list mechanism, so LLDD's that can only cancel one
command at a time will have problem.

-- Patrick Mansfield

Re: libata error handling

[Luben Tuikov]

On 08/19/05 15:38, Patrick Mansfield wrote:
> On Fri, Aug 19, 2005 at 02:46:35PM -0400, Luben Tuikov wrote:
> 
> 
>>Using the command time out hook and the strategy routine, gives _complete_
>>control over host recovery, and I really do mean _complete_.
>>
> 
> 
> I assume you mean hostt->eh_timed_out.

Hi Patrick, how are you?

Yes, this is what I meant, sorry for not being clear.

> Is anyone implmenting (or has implemented) a ->eh_timed_out function? I see
> none in mainline kernel.

Yes, I have.

> I was looking at using it in an LLDD, but hit two problems, and have
> started to work on an alternate approach of cancelling (aborting or wtf you
> want to call it) a list of commands in the eh thread.

The eh_timed_out + eh_strategy_handler is actually pretty perfect,
and _complete_, for any application and purpose in recovering a
LU/device/host (in that order ;-) ).

> The two problems I see with the hook are:
> 
> It calls the driver in interrupt context, so the called function can't
> sleep.

Consider this: When SCSI Core told you that the command timed out,
	A) it has already finished,
	B) it hasn't already finished.

In case A, you can return EH_HANDLED.  In case B, you return
EH_NOT_HANDLED, and deal with it in the eh_strategy_handler.
(Hint: you can still "finish" it from there.)

EH_RESET_TIMER is not really needed provided that
	- your interface infrastructure is in place,
	- you set the timeout value properly in slave_configure.

> There is no queueing or list mechanism, so LLDD's that can only cancel one
> command at a time will have problem.

See above.

	Luben

Re: libata error handling

[Patrick Mansfield]

On Fri, Aug 19, 2005 at 04:03:15PM -0400, Luben Tuikov wrote:
> On 08/19/05 15:38, Patrick Mansfield wrote:
> > On Fri, Aug 19, 2005 at 02:46:35PM -0400, Luben Tuikov wrote:
> > 
> > 
> >>Using the command time out hook and the strategy routine, gives _complete_
> >>control over host recovery, and I really do mean _complete_.
> >>
> > 
> > 
> > I assume you mean hostt->eh_timed_out.
> 
> Hi Patrick, how are you?

Good thanks :)

> > I was looking at using it in an LLDD, but hit two problems, and have
> > started to work on an alternate approach of cancelling (aborting or wtf you
> > want to call it) a list of commands in the eh thread.
> 
> The eh_timed_out + eh_strategy_handler is actually pretty perfect,
> and _complete_, for any application and purpose in recovering a

One other point: Another problems is that we quiesce all shost IO before
waking up the eh. 

I was changing it to wakeup the eh even while other IO is outstanding, so
the eh can wakeup and cancel individual commands while other IO is still
using the HBA.

> > The two problems I see with the hook are:
> > 
> > It calls the driver in interrupt context, so the called function can't
> > sleep.
> 
> Consider this: When SCSI Core told you that the command timed out,
> 	A) it has already finished,
> 	B) it hasn't already finished.
> 
> In case A, you can return EH_HANDLED.  In case B, you return
> EH_NOT_HANDLED, and deal with it in the eh_strategy_handler.

So, for EH_NOT_HANDLED, do you add the scmd to a LLDD list in your
eh_timed_out, then wait for the eh to run?

Or maybe your host can_queue is 1 :)

> (Hint: you can still "finish" it from there.)
> 
> EH_RESET_TIMER is not really needed provided that
> 	- your interface infrastructure is in place,
> 	- you set the timeout value properly in slave_configure.
> 
> > There is no queueing or list mechanism, so LLDD's that can only cancel one
> > command at a time will have problem.
> 
> See above.

I don't see it ... hence my question above.

-- Patrick Mansfield

Re: libata error handling

[Mike Anderson]

Luben Tuikov <luben_tuikov@adaptec.com> wrote:
> On 08/19/05 15:38, Patrick Mansfield wrote:
> The eh_timed_out + eh_strategy_handler is actually pretty perfect,
> and _complete_, for any application and purpose in recovering a
> LU/device/host (in that order ;-) ).
> 
> > The two problems I see with the hook are:
> > 
> > It calls the driver in interrupt context, so the called function can't
> > sleep.
> 
> Consider this: When SCSI Core told you that the command timed out,
> 	A) it has already finished,
> 	B) it hasn't already finished.
> 
> In case A, you can return EH_HANDLED.  In case B, you return
> EH_NOT_HANDLED, and deal with it in the eh_strategy_handler.
> (Hint: you can still "finish" it from there.)
> 

But dealing with it in the eh_strategy_handler means that you may be
stopping all IO on the host instance as the first lun returns
EH_NOT_HANDLED for LUN based canceling.

I still think we can do better here for an LLDD that cannot execute a
cancel in interrupt context.

Having a error handler that works is a plus, I would hope that
some factoring would happen over time from the eh_strategy_handler to
some transport (or other factor point) error handler. I would think from a
testing, support, and block level multipath predictability sharing code
would be a good goal.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: libata error handling

[Luben Tuikov]

On 08/19/05 16:11, Patrick Mansfield wrote:
> On Fri, Aug 19, 2005 at 04:03:15PM -0400, Luben Tuikov wrote:
>>The eh_timed_out + eh_strategy_handler is actually pretty perfect,
>>and _complete_, for any application and purpose in recovering a
> 
> 
> One other point: Another problems is that we quiesce all shost IO before
> waking up the eh. 

Yes, this is true.
 
> I was changing it to wakeup the eh even while other IO is outstanding, so
> the eh can wakeup and cancel individual commands while other IO is still
> using the HBA.

Hmm, if you want to do this, then SCSI Core needs to know about:
	- Domain,
	- Domain device and
	- LU.

The reason, is that you do not know why a task timed out.
Is it the LU, is it the device, is it the domain?

(Those are concepts talked about in SAM.)

Since currently, SCSI Core has no clue about those concepts,
the current infrastructure, stalling IO to the host on eh,
satisfies.

> So, for EH_NOT_HANDLED, do you add the scmd to a LLDD list in your
> eh_timed_out, then wait for the eh to run?

No, no Patrick, I don't.  The SCSI Core does this for me, and then
calls my eh_strategy routine and all the commands are on the list.

> Or maybe your host can_queue is 1 :)

No, it is actually pretty huge for a controller, and have to 
more than halve it and give that to SCSI Core.
 
> I don't see it ... hence my question above.

Hmm, let me know if I'm missing something out.

	Luben

Re: libata error handling

[Luben Tuikov]

On 08/19/05 16:29, Mike Anderson wrote:
> Luben Tuikov <luben_tuikov@adaptec.com> wrote:
>>Consider this: When SCSI Core told you that the command timed out,
>>	A) it has already finished,
>>	B) it hasn't already finished.
>>
>>In case A, you can return EH_HANDLED.  In case B, you return
>>EH_NOT_HANDLED, and deal with it in the eh_strategy_handler.
>>(Hint: you can still "finish" it from there.)
>>
> 
> 
> But dealing with it in the eh_strategy_handler means that you may be
> stopping all IO on the host instance as the first lun returns
> EH_NOT_HANDLED for LUN based canceling.

Hi Mike, how are you?

Yes, this is true.  See my email to Patrick.
 
> I still think we can do better here for an LLDD that cannot execute a
> cancel in interrupt context.

This is the key!

Think about this:
	You do not need to cancel a command to cancel a command. ;-)
 
> Having a error handler that works is a plus, I would hope that
> some factoring would happen over time from the eh_strategy_handler to
> some transport (or other factor point) error handler. I would think from a
> testing, support, and block level multipath predictability sharing code
> would be a good goal.

Yes, definitely.  Hopefully I'll be posting code soon.

	Luben

Re: libata error handling

[Patrick Mansfield]

Luben -

On Fri, Aug 19, 2005 at 04:43:41PM -0400, Luben Tuikov wrote:
> On 08/19/05 16:11, Patrick Mansfield wrote:

> > I was changing it to wakeup the eh even while other IO is outstanding, so
> > the eh can wakeup and cancel individual commands while other IO is still
> > using the HBA.
> 
> Hmm, if you want to do this, then SCSI Core needs to know about:
> 	- Domain,
> 	- Domain device and
> 	- LU.

Not really, scsi core is just asking the LLDD to cancel or release the scmd.
That is really all we do in the eh today, and then if the LLDD can't
cancel the scmd, we take other sometimes less than useful steps.

The LLDD could start any error handling scheme it wants, independent of
scsi core action.

We don't initiate error handling in scsi core for other error cases, why
should a timeout be any different?

> The reason, is that you do not know why a task timed out.
> Is it the LU, is it the device, is it the domain?

Right, so in scsi core allow a simple method that can cancel commands
while the HBA is still in use.

> (Those are concepts talked about in SAM.)
> 
> Since currently, SCSI Core has no clue about those concepts,
> the current infrastructure, stalling IO to the host on eh,
> satisfies.
> 
> > So, for EH_NOT_HANDLED, do you add the scmd to a LLDD list in your
> > eh_timed_out, then wait for the eh to run?
> 
> No, no Patrick, I don't.  The SCSI Core does this for me, and then
> calls my eh_strategy routine and all the commands are on the list.

Oh right ... I was not thinking straight.

But I don't see how that gains much, if you sometimes still wait for scsi
core to quiesce IO and wakeup the eh.

-- Patrick Mansfield

Re: libata error handling

[Luben Tuikov]

On 08/19/05 17:10, Patrick Mansfield wrote:
> Luben -
> 
> On Fri, Aug 19, 2005 at 04:43:41PM -0400, Luben Tuikov wrote:
> 
>>On 08/19/05 16:11, Patrick Mansfield wrote:
> 
> 
>>>I was changing it to wakeup the eh even while other IO is outstanding, so
>>>the eh can wakeup and cancel individual commands while other IO is still
>>>using the HBA.
>>
>>Hmm, if you want to do this, then SCSI Core needs to know about:
>>	- Domain,
>>	- Domain device and
>>	- LU.
> 
> 
> Not really, scsi core is just asking the LLDD to cancel or release the scmd.

Hi Patrick,

If you want to call any kind of eh, *without* stalling IO to the host, you have
to know the context: LU or Domain Device or Domain,  in order to stall IO
to the context object only.

> That is really all we do in the eh today, and then if the LLDD can't
> cancel the scmd, we take other sometimes less than useful steps.

But remember, eh_timed_out and eh_strategy_handler point to
functions in the driver, so they can do more or less, depending.

BTW, you may be able to implement an _instantaneous_ canceling of
a command in the LLDD from any context, but this would bloat the
LLDD so much, that is is not a viable solution, plus it _cannot_
always be done (physically).

This is why eh_timed_out + eh_strategy_handler is a best current bet
for transport LLDD. (Given the current SCSI Core infrastructure.)

As far as the upper layers are concerned:
	- the command is either finished with status,
	- or the LU or the Device or the Domain had problems.
The above list is exhaustive, because if the task was never
able to be sent via the service delivery subsystem you'd
get a response, but the task will not "sit" in the LLDD.

Most importantly to remember is that a timed out command _cannot_
be returned instantaneously to SCSI Core, unless it is "complete"
in the SAM sense of the meaning.  If the task is out on the domain,
the respective eh needs to be invoked.

> The LLDD could start any error handling scheme it wants, independent of
> scsi core action.

True, but this will bloat LLDD very much, and all you want of a
transport LLDD is access to the delivery subsystem as described in SAM.

> We don't initiate error handling in scsi core for other error cases, why
> should a timeout be any different?

Think of eh_timed_out as a replacement of the transport timeout
plus some delta.  Even if you had implemented it internally,
which you don't need to, you'd still get a timer to fire off
and you'd be in interrupt context.

As I mentioned earlier, if you've properly set the timeout value at
slave_configure, you shouldn't be getting SCSI Core calling your
eh_timed_out _unless_ the transport time out has kicked in, which
*you* set at slave configure time.

If, OTOH, SCSI Core _did_ call your eh_timed_out routine, this means
that the timeout which *you* set has kicked in and action needs
to be taken.  At that time, as per protocol, your task is either
complete or you need to go and find it and take the appropriate
action.

>>The reason, is that you do not know why a task timed out.
>>Is it the LU, is it the device, is it the domain?
> 
> Right, so in scsi core allow a simple method that can cancel commands
> while the HBA is still in use.

Yes, you can do this.  But in order to do this you'll need to know
_why_ the command failed.  Is it the LU or is it the Device or
is it the Domain.

The only one who can tell you this is the LLDD who talks to the
transport.

*Unless*, the LLDD exports SAM TMFs and SCSI Core knows what to call
and how, which is currently not the case.

>>>So, for EH_NOT_HANDLED, do you add the scmd to a LLDD list in your
>>>eh_timed_out, then wait for the eh to run?
>>
>>No, no Patrick, I don't.  The SCSI Core does this for me, and then
>>calls my eh_strategy routine and all the commands are on the list.
> 
> 
> Oh right ... I was not thinking straight.
> 
> But I don't see how that gains much, if you sometimes still wait for scsi
> core to quiesce IO and wakeup the eh.

Yes, you're completely right, the SCSI Core eh infrastructure is
incomplete. (I never mentioned I was gaining anything, I was merely
working around the current infrastructure. ;-) )

With the current infrastructure you cannot fine grain stalling
IO to the different storage objects, simply because SCSI Core
has _no representation for them_.

	Luben

Re: libata error handling

[Bartlomiej Zolnierkiewicz]

On 1/7/07, Kasper Sandberg <lkml@metanurb.dk> wrote:
> On Sat, 2007-01-06 at 20:28 +0100, Bartlomiej Zolnierkiewicz wrote:
> > On 1/6/07, Kasper Sandberg <lkml@metanurb.dk> wrote:
> > > On Sat, 2007-01-06 at 13:01 -0600, Robert Hancock wrote:
> > > > Kasper Sandberg wrote:
> > > > > On Sat, 2007-01-06 at 12:21 -0600, Robert Hancock wrote:
> > > > >> Kasper Sandberg wrote:
> > > > >>> i have heard that libata has much better error handling (this is what
> > > > >>> made me try it), and from initial observations, that appears to be very
> > > > >>> true, however, im wondering, is there something i can do to get
> > > > >>> extremely verbose information from libata? for example if it corrects
> > > > >>> errors? cause i'd really like to know if it still happens, and if i
> > > > >>> perhaps get corruption as before, even though not severe.
> > > > >> Any errors, timeouts or retries would be showing up in dmesg..
> > > > > how sure can i be of this? is it 100% sure that i have not encountered
> > > > > this error then?
> > > >
> > > > Pretty sure, I'm quite certain libata never does any silent error recovery..
> >
> > AFAIR this is true
> > (at least it was last time that I've looked at libata eh code)
> >
> > > okay, i suppose i face two possibilities then:
> > > 1: libata drivers are simply better, and the error does not occur
> > > because of driver bugs in the old ide drivers
> >
> > very likely however pdc202xx_new bugs should be fixed in 2.6.20-rc3
> > (as it contains a lot of bugfixes for this driver from Sergei Shtylyov)
> these fixes are also in the libata driver?

some were backported directly from libata driver and few were
pdc202xx_new specific so probably pata_pdc2027x is also fine

> > > 2: it hasnt happened to me on libata yet (though this is also abit
> > > weird, as it has now ran far longer than were previously required to hit
> > > the errors)

Re: libata error handling

[Kasper Sandberg]

On Sat, 2007-01-06 at 20:28 +0100, Bartlomiej Zolnierkiewicz wrote:
> On 1/6/07, Kasper Sandberg <lkml@metanurb.dk> wrote:
> > On Sat, 2007-01-06 at 13:01 -0600, Robert Hancock wrote:
> > > Kasper Sandberg wrote:
> > > > On Sat, 2007-01-06 at 12:21 -0600, Robert Hancock wrote:
> > > >> Kasper Sandberg wrote:
> > > >>> i have heard that libata has much better error handling (this is what
> > > >>> made me try it), and from initial observations, that appears to be very
> > > >>> true, however, im wondering, is there something i can do to get
> > > >>> extremely verbose information from libata? for example if it corrects
> > > >>> errors? cause i'd really like to know if it still happens, and if i
> > > >>> perhaps get corruption as before, even though not severe.
> > > >> Any errors, timeouts or retries would be showing up in dmesg..
> > > > how sure can i be of this? is it 100% sure that i have not encountered
> > > > this error then?
> > >
> > > Pretty sure, I'm quite certain libata never does any silent error recovery..
> 
> AFAIR this is true
> (at least it was last time that I've looked at libata eh code)
> 
> > okay, i suppose i face two possibilities then:
> > 1: libata drivers are simply better, and the error does not occur
> > because of driver bugs in the old ide drivers
> 
> very likely however pdc202xx_new bugs should be fixed in 2.6.20-rc3
> (as it contains a lot of bugfixes for this driver from Sergei Shtylyov)
these fixes are also in the libata driver?
> 
> > 2: it hasnt happened to me on libata yet (though this is also abit
> > weird, as it has now ran far longer than were previously required to hit
> > the errors)
> 

Re: libata error handling

[Bartlomiej Zolnierkiewicz]

On 1/6/07, Kasper Sandberg <lkml@metanurb.dk> wrote:
> On Sat, 2007-01-06 at 13:01 -0600, Robert Hancock wrote:
> > Kasper Sandberg wrote:
> > > On Sat, 2007-01-06 at 12:21 -0600, Robert Hancock wrote:
> > >> Kasper Sandberg wrote:
> > >>> i have heard that libata has much better error handling (this is what
> > >>> made me try it), and from initial observations, that appears to be very
> > >>> true, however, im wondering, is there something i can do to get
> > >>> extremely verbose information from libata? for example if it corrects
> > >>> errors? cause i'd really like to know if it still happens, and if i
> > >>> perhaps get corruption as before, even though not severe.
> > >> Any errors, timeouts or retries would be showing up in dmesg..
> > > how sure can i be of this? is it 100% sure that i have not encountered
> > > this error then?
> >
> > Pretty sure, I'm quite certain libata never does any silent error recovery..

AFAIR this is true
(at least it was last time that I've looked at libata eh code)

> okay, i suppose i face two possibilities then:
> 1: libata drivers are simply better, and the error does not occur
> because of driver bugs in the old ide drivers

very likely however pdc202xx_new bugs should be fixed in 2.6.20-rc3
(as it contains a lot of bugfixes for this driver from Sergei Shtylyov)

> 2: it hasnt happened to me on libata yet (though this is also abit
> weird, as it has now ran far longer than were previously required to hit
> the errors)

Re: libata error handling

[Kasper Sandberg]

On Sat, 2007-01-06 at 13:01 -0600, Robert Hancock wrote:
> Kasper Sandberg wrote:
> > On Sat, 2007-01-06 at 12:21 -0600, Robert Hancock wrote:
> >> Kasper Sandberg wrote:
> >>> i have heard that libata has much better error handling (this is what
> >>> made me try it), and from initial observations, that appears to be very
> >>> true, however, im wondering, is there something i can do to get
> >>> extremely verbose information from libata? for example if it corrects
> >>> errors? cause i'd really like to know if it still happens, and if i
> >>> perhaps get corruption as before, even though not severe.
> >> Any errors, timeouts or retries would be showing up in dmesg..
> > how sure can i be of this? is it 100% sure that i have not encountered
> > this error then?
> 
> Pretty sure, I'm quite certain libata never does any silent error recovery..
okay, i suppose i face two possibilities then:
1: libata drivers are simply better, and the error does not occur
because of driver bugs in the old ide drivers
2: it hasnt happened to me on libata yet (though this is also abit
weird, as it has now ran far longer than were previously required to hit
the errors)
> 

Re: libata error handling

[Robert Hancock]

Kasper Sandberg wrote:
> On Sat, 2007-01-06 at 12:21 -0600, Robert Hancock wrote:
>> Kasper Sandberg wrote:
>>> i have heard that libata has much better error handling (this is what
>>> made me try it), and from initial observations, that appears to be very
>>> true, however, im wondering, is there something i can do to get
>>> extremely verbose information from libata? for example if it corrects
>>> errors? cause i'd really like to know if it still happens, and if i
>>> perhaps get corruption as before, even though not severe.
>> Any errors, timeouts or retries would be showing up in dmesg..
> how sure can i be of this? is it 100% sure that i have not encountered
> this error then?

Pretty sure, I'm quite certain libata never does any silent error recovery..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: libata error handling

[Kasper Sandberg]

On Sat, 2007-01-06 at 12:21 -0600, Robert Hancock wrote:
> Kasper Sandberg wrote:
> > i have heard that libata has much better error handling (this is what
> > made me try it), and from initial observations, that appears to be very
> > true, however, im wondering, is there something i can do to get
> > extremely verbose information from libata? for example if it corrects
> > errors? cause i'd really like to know if it still happens, and if i
> > perhaps get corruption as before, even though not severe.
> 
> Any errors, timeouts or retries would be showing up in dmesg..
how sure can i be of this? is it 100% sure that i have not encountered
this error then?
> 

Re: libata error handling

[Robert Hancock]

Kasper Sandberg wrote:
> i have heard that libata has much better error handling (this is what
> made me try it), and from initial observations, that appears to be very
> true, however, im wondering, is there something i can do to get
> extremely verbose information from libata? for example if it corrects
> errors? cause i'd really like to know if it still happens, and if i
> perhaps get corruption as before, even though not severe.

Any errors, timeouts or retries would be showing up in dmesg..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

libata error handling

[Kasper Sandberg]

Hello.

i have a question in regards to libata's error handling, specifically
with pata drivers.

ill start by explaining something that happens to me using the normal
ide drivers (via ide and pdc202 new)

this is what i get when it has been used for a while:
hde: dma_intr: bad DMA status (dma_stat=75)
hde: dma_intr: status=0x50 { DriveReady SeekComplete }
ide: failed opcode was: unknown
hde: dma_timer_expiry: dma status == 0x60
hde: DMA timeout retry
PDC202XX: Primary channel reset.
hde: timeout waiting for DMA

its ALWAYS hde, and its on the promise controller, i attempted to
replace the promise controller by other controllers, but i got the same
error. i have tried replacing cables too, and swapping around
harddrives, its ALWAYS the last harddrive that gets me this. after this,
my raid (6x300gb drives in raid5) would go nuts, as if the data was
there, but skewed, so i got it all from an offset.

this has been going on since always on this box, from .15 to .17, but
now i updated to .20-rc3-git4, and went over to the pata-on-libata
drivers, where i think this has stopped, or atleast, its not causing
WEIRD errors anymore, i have observed some stalls, but im not sure this
is due to it doing this, or simply syncing. i get no messages like this
from the kernel anymore.

i have heard that libata has much better error handling (this is what
made me try it), and from initial observations, that appears to be very
true, however, im wondering, is there something i can do to get
extremely verbose information from libata? for example if it corrects
errors? cause i'd really like to know if it still happens, and if i
perhaps get corruption as before, even though not severe.


Regards,
Kasper Sandberg