[PATCH] libsas, aic94xx: fix dma mapping cockups with ATA

[PATCH] libsas, aic94xx: fix dma mapping cockups with ATA

[James Bottomley]

This one was noticed by Gilbert Wu of Adaptec:

The libata core actually does the DMA mapping for you, so there has to
be an exception in the device drivers that *don't* do dma mapping for
ATA commands.  However, since we've already done this, libsas must now
dma map any ATA commands that it wishes to issue ... and yes, this is a
horrible mess.

Additionally, the test in aic94xx for ATA protocols isn't quite right.

---

James

Index: BUILD-2.6/drivers/scsi/libsas/sas_discover.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/libsas/sas_discover.c	2007-07-16 13:01:19.000000000 -0500
+++ BUILD-2.6/drivers/scsi/libsas/sas_discover.c	2007-07-16 13:39:57.000000000 -0500
@@ -110,6 +110,13 @@ static int sas_execute_task(struct sas_t
 	task->total_xfer_len = size;
 	task->data_dir = pci_dma_dir;
 	task->task_done = sas_disc_task_done;
+	if (pci_dma_dir != PCI_DMA_NONE &&
+	    sas_protocol_ata(task->task_proto)) {
+		task->num_scatter = pci_map_sg(task->dev->port->ha->pcidev,
+					       task->scatter,
+					       task->num_scatter,
+					       task->data_dir);
+	}
 
 	for (retries = 0; retries < 5; retries++) {
 		task->task_state_flags = SAS_TASK_STATE_PENDING;
@@ -192,8 +199,13 @@ static int sas_execute_task(struct sas_t
 		}
 	}
 ex_err:
-	if (pci_dma_dir != PCI_DMA_NONE)
+	if (pci_dma_dir != PCI_DMA_NONE) {
+		if (sas_protocol_ata(task->task_proto))
+			pci_unmap_sg(task->dev->port->ha->pcidev,
+				     task->scatter, task->num_scatter,
+				     task->data_dir);
 		kfree(scatter);
+	}
 out:
 	return res;
 }
Index: BUILD-2.6/drivers/scsi/aic94xx/aic94xx_task.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/aic94xx/aic94xx_task.c	2007-07-16 13:37:59.000000000 -0500
+++ BUILD-2.6/drivers/scsi/aic94xx/aic94xx_task.c	2007-07-16 13:38:46.000000000 -0500
@@ -76,7 +76,7 @@ static inline int asd_map_scatterlist(st
 
 	/* STP tasks come from libata which has already mapped
 	 * the SG list */
-	if (task->task_proto == SAS_PROTOCOL_STP)
+	if (sas_protocol_ata(task->task_proto))
 		num_sg = task->num_scatter;
 	else
 		num_sg = pci_map_sg(asd_ha->pcidev, task->scatter,
@@ -125,7 +125,7 @@ static inline int asd_map_scatterlist(st
 
 	return 0;
 err_unmap:
-	if (task->task_proto != SAS_PROTOCOL_STP)
+	if (sas_protocol_ata(task->task_proto))
 		pci_unmap_sg(asd_ha->pcidev, task->scatter, task->num_scatter,
 			     task->data_dir);
 	return res;
Index: BUILD-2.6/include/scsi/scsi_transport_sas.h
===================================================================
--- BUILD-2.6.orig/include/scsi/scsi_transport_sas.h	2007-07-16 13:34:05.000000000 -0500
+++ BUILD-2.6/include/scsi/scsi_transport_sas.h	2007-07-16 13:35:58.000000000 -0500
@@ -23,6 +23,12 @@ enum sas_protocol {
 	SAS_PROTOCOL_SSP		= 0x08,
 };
 
+static inline int sas_protocol_ata(enum sas_protocol proto)
+{
+	return ((proto & SAS_PROTOCOL_SATA) ||
+		(proto & SAS_PROTOCOL_STP))? 1 : 0;
+}
+
 enum sas_linkrate {
 	/* These Values are defined in the SAS standard */
 	SAS_LINK_RATE_UNKNOWN = 0,

Re: [PATCH] libsas, aic94xx: fix dma mapping cockups with ATA

[Jeff Garzik]

James Bottomley wrote:
> This one was noticed by Gilbert Wu of Adaptec:
> 
> The libata core actually does the DMA mapping for you, so there has to
> be an exception in the device drivers that *don't* do dma mapping for
> ATA commands.  However, since we've already done this, libsas must now
> dma map any ATA commands that it wishes to issue ... and yes, this is a
> horrible mess.

Can you help me understand this logic?

libsas must DMA map an ATA command... because libata also DMA maps an 
ATA command?  That does not make sense to me.

	Jeff

Re: [PATCH] libsas, aic94xx: fix dma mapping cockups with ATA

[James Bottomley]

On Mon, 2007-07-16 at 16:19 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > This one was noticed by Gilbert Wu of Adaptec:
> > 
> > The libata core actually does the DMA mapping for you, so there has to
> > be an exception in the device drivers that *don't* do dma mapping for
> > ATA commands.  However, since we've already done this, libsas must now
> > dma map any ATA commands that it wishes to issue ... and yes, this is a
> > horrible mess.
> 
> Can you help me understand this logic?
> 
> libsas must DMA map an ATA command... because libata also DMA maps an 
> ATA command?  That does not make sense to me.

Why not? ... since the driver must *not* map an ATA command (to prevent
double mapping from libata), it can't tell if the command comes from
libsas or libata ... so libsas has to map any internal ATA commands it
uses.

I think you're perhaps thinking libsas must map an ATA command libata
has already mapped?  If so, no, libsas must only map the commands it is
using internally independent of libata.

James

Re: [PATCH] libsas, aic94xx: fix dma mapping cockups with ATA

[Jeff Garzik]

James Bottomley wrote:
> On Mon, 2007-07-16 at 16:19 -0400, Jeff Garzik wrote:
>> James Bottomley wrote:
>>> This one was noticed by Gilbert Wu of Adaptec:
>>>
>>> The libata core actually does the DMA mapping for you, so there has to
>>> be an exception in the device drivers that *don't* do dma mapping for
>>> ATA commands.  However, since we've already done this, libsas must now
>>> dma map any ATA commands that it wishes to issue ... and yes, this is a
>>> horrible mess.
>> Can you help me understand this logic?
>>
>> libsas must DMA map an ATA command... because libata also DMA maps an 
>> ATA command?  That does not make sense to me.
> 
> Why not? ... since the driver must *not* map an ATA command (to prevent
> double mapping from libata), it can't tell if the command comes from
> libsas or libata ... so libsas has to map any internal ATA commands it
> uses.
> 
> I think you're perhaps thinking libsas must map an ATA command libata
> has already mapped?  If so, no, libsas must only map the commands it is
> using internally independent of libata.

I was merely noting the patch description did not make sense to me, 
nothing more.  With this additional background, it does make sense :)

	Jeff