Skip to contentSkip to conversation

highmem_io for the sym53c8xx_2 driver??

PATCH In review Ryan Bradetich 3 participants 6 messages Nov 23, 2002 – Nov 26, 2002

lore
Share & tools
lore

Ryan Bradetich

author
#0 lore raw

PATCH 1/1 highmem_io for the sym53c8xx_2 driver??

Hello parisc-linux hackers,

I traced my boot problem with cvs head for the linux-2.4 tree down to the following patch:

Index: drivers/scsi/sym53c8xx_2/sym53c8xx.h
===================================================================
RCS file: /var/cvs/linux/drivers/scsi/sym53c8xx_2/sym53c8xx.h,v retrieving revision 1.4

scsi/sym53c8xx_2/sym53c8xx.h +1 -2
diff -u -p -r1.4 sym53c8xx.h
@@ -119,8 +119,7 @@ int sym53c8xx_release(struct Scsi_Host *
 	this_id:		7,					\
 	sg_tablesize:		0,					\
 	cmd_per_lun:		0,					\
-	use_clustering:		DISABLE_CLUSTERING,			\
-	highmem_io:		1}
+	use_clustering:		DISABLE_CLUSTERING}
 
 #endif /* defined(HOSTS_C) || defined(MODULE) */

Index: drivers/scsi/sym53c8xx_2/sym_glue.c
===================================================================
RCS file: /var/cvs/linux/drivers/scsi/sym53c8xx_2/sym_glue.c,v retrieving revision 1.6

scsi/sym53c8xx_2/sym_glue.c +0 -1
diff -u -p -r1.6 sym_glue.c
@@ -2154,7 +2154,6 @@ sym_attach (Scsi_Host_Template *tpnt, in
 	instance->max_cmd_len	= 16;
 #endif
 	instance->select_queue_depths = sym53c8xx_select_queue_depths;
-	instance->highmem_io	= 1;
 
 	SYM_UNLOCK_HCB(np, flags);

I am not sure what the highmem_io flag does, but when it is enabled it causes my C200 to HPMC.

Thoughts?

Thanks,

- Ryan

#1 lore raw

Ryan Bradetich wrote:
...

Quoted 2 lines

I am not sure what the highmem_io flag does, but when it is enabled it causes my C200 to HPMC.

That's for the SCSI midlayer to know whether or not the driver can do 64-bit DMA or not. Basically it means SCSI will use "page+offset" instead of "address" when building SG lists.

All parisc machines require 32-bit DMA - ie through an IO MMU or direct. Failures with highmem_io enabled suggests we aren't handling "page+offset" correctly in the iommu code.

grant

#2 lore raw
In reply to Ryan Bradetich
Quoted 2 lines · Ryan Bradetich

I am not sure what the highmem_io flag does, but when it is enabled it causes my C200 to HPMC.

yeah - ccio-dma.c isn't using sg_virt_address() to access to the virtual address of an SG list entry. It's defined in include/asm/scatterlist.h. See sba_iommu.c on how to use this correctly.

Attached diff is a first cut, untested. My advice is to rewrite the loop so sg_virt_address() result is saved once instead of referencing the macro 3 (or more) times in the main loop.

grant

Index: ccio-dma.c
===================================================================
RCS file: /var/cvs/linux/arch/parisc/kernel/ccio-dma.c,v retrieving revision 1.56

ccio-dma.c +9 -8
diff -u -p -r1.56 ccio-dma.c
@@ -805,7 +805,7 @@ ccio_fill_pdir(struct ioc *ioc, struct s
 
 		DBG_RUN_SG(" %d : %08lx/%05x %p/%05x\n", nents,
 			   (unsigned long)sg_dma_address(startsg), cnt,
-			   startsg->address, startsg->length
+			   sg_virt_address(startsg), startsg->length
 		);
 
 		/*

@@ -825,7 +825,7 @@ ccio_fill_pdir(struct ioc *ioc, struct s
 		** Look for a VCONTIG chunk
 		*/
 		if (cnt) {
-			unsigned long vaddr = (unsigned long)startsg->address;
+			unsigned long vaddr = (unsigned long)sg_virt_address(startsg);
 			ASSERT(pdirp);
 
 			/* Since multiple Vcontig blocks could make up

@@ -878,8 +878,8 @@ ccio_coalesce_chunks(struct ioc *ioc, st
 		*/
 		dma_sg = vcontig_sg = startsg;
 		dma_len = vcontig_len = vcontig_end = startsg->length;
-		vcontig_end += (unsigned long) startsg->address;
-		dma_offset = (unsigned long) startsg->address & ~IOVP_MASK;
+		vcontig_end += (unsigned long) sg_virt_address(startsg);
+		dma_offset = (unsigned long) sg_virt_address(startsg) & ~IOVP_MASK;
 
 		/* PARANOID: clear entries */
 		sg_dma_address(startsg) = 0;

@@ -893,7 +893,7 @@ ccio_coalesce_chunks(struct ioc *ioc, st
 			unsigned long startsg_end;
 
 			startsg++;
-			startsg_end = (unsigned long)startsg->address + 
+			startsg_end = (unsigned long)sg_virt_address(startsg) + 
 				startsg->length;
 
 			/* PARANOID: clear entries */

@@ -912,7 +912,7 @@ ccio_coalesce_chunks(struct ioc *ioc, st
 			/*
 			** Append the next transaction?
 			*/
-			if(vcontig_end == (unsigned long) startsg->address) {
+			if(vcontig_end == (unsigned long) sg_virt_address(startsg)) {
 				vcontig_len += startsg->length;
 				vcontig_end += startsg->length;
 				dma_len     += startsg->length;

@@ -981,7 +981,8 @@ ccio_map_sg(struct pci_dev *dev, struct 
 
 	/* Fast path single entry scatterlists. */
 	if(nents == 1) {
-		sg_dma_address(sglist)= ccio_map_single(dev, sglist->address,
+		sg_dma_address(sglist)= ccio_map_single(dev,
+							sg_virt_address(sglist),
 							sglist->length, 
 							direction);
 		sg_dma_len(sglist)= sglist->length;

@@ -1043,7 +1044,7 @@ ccio_unmap_sg(struct pci_dev *dev, struc
 	ioc = GET_IOC(dev);
 
 	DBG_RUN_SG("%s() START %d entries,  %p,%x\n",
-		__FUNCTION__, nents, sglist->address, sglist->length);
+		__FUNCTION__, nents, sg_virt_address(sglist), sglist->length);
 
 #ifdef CONFIG_PROC_FS
 	ioc->usg_calls++;
author
#3 lore raw

Thanks Grant!

I will look look at this and try this out later this week after I get back!

Nice catch.

- Ryan

Quoted 89 lines · Grant Grundler
Quoted 2 lines · Ryan Bradetich

I am not sure what the highmem_io flag does, but when it is enabled it causes my C200 to HPMC.

yeah - ccio-dma.c isn't using sg_virt_address() to access to the virtual address of an SG list entry. It's defined in include/asm/scatterlist.h. See sba_iommu.c on how to use this correctly.

Attached diff is a first cut, untested. My advice is to rewrite the loop so sg_virt_address() result is saved once instead of referencing the macro 3 (or more) times in the main loop.

grant

Index: ccio-dma.c
===================================================================
RCS file: /var/cvs/linux/arch/parisc/kernel/ccio-dma.c,v retrieving revision 1.56
diff -u -p -r1.56 ccio-dma.c

ccio-dma.c +9 -8
@@ -805,7 +805,7 @@ ccio_fill_pdir(struct ioc *ioc, struct s
 
 		DBG_RUN_SG(" %d : %08lx/%05x %p/%05x\n", nents,
 			   (unsigned long)sg_dma_address(startsg), cnt,
-			   startsg->address, startsg->length
+			   sg_virt_address(startsg), startsg->length
 		);
 
 		/*

@@ -825,7 +825,7 @@ ccio_fill_pdir(struct ioc *ioc, struct s
 		** Look for a VCONTIG chunk
 		*/
 		if (cnt) {
-			unsigned long vaddr = (unsigned long)startsg->address;
+			unsigned long vaddr = (unsigned long)sg_virt_address(startsg);
 			ASSERT(pdirp);
 
 			/* Since multiple Vcontig blocks could make up

@@ -878,8 +878,8 @@ ccio_coalesce_chunks(struct ioc *ioc, st
 		*/
 		dma_sg = vcontig_sg = startsg;
 		dma_len = vcontig_len = vcontig_end = startsg->length;
-		vcontig_end += (unsigned long) startsg->address;
-		dma_offset = (unsigned long) startsg->address & ~IOVP_MASK;
+		vcontig_end += (unsigned long) sg_virt_address(startsg);
+		dma_offset = (unsigned long) sg_virt_address(startsg) & ~IOVP_MASK;
 
 		/* PARANOID: clear entries */
 		sg_dma_address(startsg) = 0;

@@ -893,7 +893,7 @@ ccio_coalesce_chunks(struct ioc *ioc, st
 			unsigned long startsg_end;
 
 			startsg++;
-			startsg_end = (unsigned long)startsg->address + 
+			startsg_end = (unsigned long)sg_virt_address(startsg) + 
 				startsg->length;
 
 			/* PARANOID: clear entries */

@@ -912,7 +912,7 @@ ccio_coalesce_chunks(struct ioc *ioc, st
 			/*
 			** Append the next transaction?
 			*/
-			if(vcontig_end == (unsigned long) startsg->address) {
+			if(vcontig_end == (unsigned long) sg_virt_address(startsg)) {
 				vcontig_len += startsg->length;
 				vcontig_end += startsg->length;
 				dma_len     += startsg->length;

@@ -981,7 +981,8 @@ ccio_map_sg(struct pci_dev *dev, struct 
 
 	/* Fast path single entry scatterlists. */
 	if(nents == 1) {
-		sg_dma_address(sglist)= ccio_map_single(dev, sglist->address,
+		sg_dma_address(sglist)= ccio_map_single(dev,
+							sg_virt_address(sglist),
 							sglist->length, 
 							direction);
 		sg_dma_len(sglist)= sglist->length;

@@ -1043,7 +1044,7 @@ ccio_unmap_sg(struct pci_dev *dev, struc
 	ioc = GET_IOC(dev);
 
 	DBG_RUN_SG("%s() START %d entries,  %p,%x\n",
-		__FUNCTION__, nents, sglist->address, sglist->length);
+		__FUNCTION__, nents, sg_virt_address(sglist), sglist->length);
 
 #ifdef CONFIG_PROC_FS
 	ioc->usg_calls++;
#4 lore raw
In reply to Grant Grundler
Quoted 9 lines

yeah - ccio-dma.c isn't using sg_virt_address() to access to the virtual address of an SG list entry. It's defined in include/asm/scatterlist.h. See sba_iommu.c on how to use this correctly.

Attached diff is a first cut, untested. My advice is to rewrite the loop so sg_virt_address() result is saved once instead of referencing the macro 3 (or more) times in the main loop.

Hey Grant,

Am I wrong or sg_virt_address() should be called sg_virt_addr()?

Joel

********************************************************************************
Controlez mieux votre consommation Internet...surfez Tiscali Complete...http://tiscali.complete.be