[PATCH 0/19] clean ups on the drivers

[PATCH 0/19] clean ups on the drivers

[FUJITA Tomonori]

Hi,

This patchset cleans up 9 scsi drivers, ipr, ibmvscsi, fusion,
aic7xxx, qla2xxx, lpfc, aacraid, BusLogic and aic79xxx.

- remove the now unnecessary map_single path.

- convert to new accessors for the scsi data buffer, the sg lists,
bufferlen, resid.

There are two patches for each driver, removing the non-use-sg code
and converting to use the accessors.

You can find the new accessors in the first patch. It includes
scsi_dma_map/unmap (calling dma_map/unmap_sg) that Christoph proposed
long ago and several macros (like scsi_for_each_sg, scsi_bufflen, etc)
to isolate the driver from the sg lists and the parameters.

This is in preparation for chaining sg lists and bidirectional
requests and possibly the mid-layer dma mapping, which has been
discussed. scsi_for_each_sg will be converted to use Jens' chaining sg
lists (for_each_sg) when it's ready. Jens, sorry about overlapping
with the chaining sg lists patches for the SCSI drivers.

I tested the patches lightly, booting machines and performing some
I/Os for few minutes.

This patchset can be applied cleanly to the scsi-misc or Linus' tree.

If this looks ok, I try to clean up the rest of the drivers (though I
can only do compile tests).

Re: [PATCH 0/19] clean ups on the drivers

[Christoph Hellwig]

On Sat, May 12, 2007 at 07:05:42PM +0900, FUJITA Tomonori wrote:
> There are two patches for each driver, removing the non-use-sg code
> and converting to use the accessors.

One patch would probably be fine aswell.

Re: [PATCH 0/19] clean ups on the drivers

[FUJITA Tomonori]

From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 0/19] clean ups on the drivers
Date: Sat, 12 May 2007 16:30:23 +0100

> On Sat, May 12, 2007 at 07:05:42PM +0900, FUJITA Tomonori wrote:
> > There are two patches for each driver, removing the non-use-sg code
> > and converting to use the accessors.
> 
> One patch would probably be fine aswell.

Yeah probably. I just thought that it's easier for the driver
maintainers to review separate two patches.

Re: [PATCH 0/19] clean ups on the drivers

[FUJITA Tomonori]

From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 0/19] clean ups on the drivers
Date: Sat, 12 May 2007 16:30:23 +0100

> On Sat, May 12, 2007 at 07:05:42PM +0900, FUJITA Tomonori wrote:
> > There are two patches for each driver, removing the non-use-sg code
> > and converting to use the accessors.
> 
> One patch would probably be fine aswell.

I merged the two. And I finished cleaning up 35 drivers in total.

git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-bidi.git cleanups

Re: [PATCH 0/19] clean ups on the drivers

[Stefan Richter]

FUJITA Tomonori wrote:
>> > There are two patches for each driver, removing the non-use-sg code
>> > and converting to use the accessors.
...
> I merged the two. And I finished cleaning up 35 drivers in total.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-bidi.git cleanups

According to what I see via gitweb, at least usb-storage, ieee1394/sbp2,
and firewire/fw-sbp2 weren't treated yet.  If somebody does so for the
latter two:  The shost->shost_gendev.parent has to be changed for
scsi_dma_{un}map.  I don't know if this can be done without breaking
anything.
-- 
Stefan Richter
-=====-=-=== -=-= -===-
http://arcgraph.de/sr/

Re: [PATCH 0/19] clean ups on the drivers

[FUJITA Tomonori]

From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: Re: [PATCH 0/19] clean ups on the drivers
Date: Mon, 14 May 2007 17:39:06 +0200

> FUJITA Tomonori wrote:
> >> > There are two patches for each driver, removing the non-use-sg code
> >> > and converting to use the accessors.
> ...
> > I merged the two. And I finished cleaning up 35 drivers in total.
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-bidi.git cleanups
> 
> According to what I see via gitweb, at least usb-storage, ieee1394/sbp2,
> and firewire/fw-sbp2 weren't treated yet.  If somebody does so for the
> latter two:  The shost->shost_gendev.parent has to be changed for
> scsi_dma_{un}map.  I don't know if this can be done without breaking
> anything.

The drivers don't need to use scsi_dma_{un}map. For example, iscsi_tcp
(doesn't need dma mappings), ib_iser and ib_srp (the ib stack provides
something like scsi_dmap_{un}map) don't use them.

Here's a patch for ieee1394/sbp2, which does the minimum requirement,
which kills the non-use-sg case and use scsi_sg_count and scsi_sglist
macros.


>From 07037aac7f5b0a14158a44370e0306f95d9dfcdd Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Mon, 14 May 2007 20:00:04 +0900
Subject: [PATCH] ieee1394/sbp2: convert to use the data buffer accessors

- remove the unnecessary map_single path.

- convert to use the new accessors for the sg lists and the
parameters.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/ieee1394/sbp2.c |   75 ++--------------------------------------------
 1 files changed, 4 insertions(+), 71 deletions(-)

diff --git a/drivers/ieee1394/sbp2.c b/drivers/ieee1394/sbp2.c
index 4cb6fa2..d0db6f8 100644
--- a/drivers/ieee1394/sbp2.c
+++ b/drivers/ieee1394/sbp2.c
@@ -1488,69 +1488,6 @@ static void sbp2_prep_command_orb_sg(struct sbp2_command_orb *orb,
 	}
 }
 
-static void sbp2_prep_command_orb_no_sg(struct sbp2_command_orb *orb,
-					struct sbp2_fwhost_info *hi,
-					struct sbp2_command_info *cmd,
-					struct scatterlist *sgpnt,
-					u32 orb_direction,
-					unsigned int scsi_request_bufflen,
-					void *scsi_request_buffer,
-					enum dma_data_direction dma_dir)
-{
-	cmd->dma_dir = dma_dir;
-	cmd->dma_size = scsi_request_bufflen;
-	cmd->dma_type = CMD_DMA_SINGLE;
-	cmd->cmd_dma = dma_map_single(hi->host->device.parent,
-				      scsi_request_buffer,
-				      cmd->dma_size, cmd->dma_dir);
-	orb->data_descriptor_hi = ORB_SET_NODE_ID(hi->host->node_id);
-	orb->misc |= ORB_SET_DIRECTION(orb_direction);
-
-	/* handle case where we get a command w/o s/g enabled
-	 * (but check for transfers larger than 64K) */
-	if (scsi_request_bufflen <= SBP2_MAX_SG_ELEMENT_LENGTH) {
-
-		orb->data_descriptor_lo = cmd->cmd_dma;
-		orb->misc |= ORB_SET_DATA_SIZE(scsi_request_bufflen);
-
-	} else {
-		/* The buffer is too large. Turn this into page tables. */
-
-		struct sbp2_unrestricted_page_table *sg_element =
-						&cmd->scatter_gather_element[0];
-		u32 sg_count, sg_len;
-		dma_addr_t sg_addr;
-
-		orb->data_descriptor_lo = cmd->sge_dma;
-		orb->misc |= ORB_SET_PAGE_TABLE_PRESENT(0x1);
-
-		/* fill out our SBP-2 page tables; split up the large buffer */
-		sg_count = 0;
-		sg_len = scsi_request_bufflen;
-		sg_addr = cmd->cmd_dma;
-		while (sg_len) {
-			sg_element[sg_count].segment_base_lo = sg_addr;
-			if (sg_len > SBP2_MAX_SG_ELEMENT_LENGTH) {
-				sg_element[sg_count].length_segment_base_hi =
-					PAGE_TABLE_SET_SEGMENT_LENGTH(SBP2_MAX_SG_ELEMENT_LENGTH);
-				sg_addr += SBP2_MAX_SG_ELEMENT_LENGTH;
-				sg_len -= SBP2_MAX_SG_ELEMENT_LENGTH;
-			} else {
-				sg_element[sg_count].length_segment_base_hi =
-					PAGE_TABLE_SET_SEGMENT_LENGTH(sg_len);
-				sg_len = 0;
-			}
-			sg_count++;
-		}
-
-		orb->misc |= ORB_SET_DATA_SIZE(sg_count);
-
-		sbp2util_cpu_to_be32_buffer(sg_element,
-				(sizeof(struct sbp2_unrestricted_page_table)) *
-				sg_count);
-	}
-}
-
 static void sbp2_create_command_orb(struct sbp2_lu *lu,
 				    struct sbp2_command_info *cmd,
 				    unchar *scsi_cmd,
@@ -1594,13 +1531,9 @@ static void sbp2_create_command_orb(struct sbp2_lu *lu,
 		orb->data_descriptor_hi = 0x0;
 		orb->data_descriptor_lo = 0x0;
 		orb->misc |= ORB_SET_DIRECTION(1);
-	} else if (scsi_use_sg)
+	} else
 		sbp2_prep_command_orb_sg(orb, hi, cmd, scsi_use_sg, sgpnt,
 					 orb_direction, dma_dir);
-	else
-		sbp2_prep_command_orb_no_sg(orb, hi, cmd, sgpnt, orb_direction,
-					    scsi_request_bufflen,
-					    scsi_request_buffer, dma_dir);
 
 	sbp2util_cpu_to_be32_buffer(orb, sizeof(*orb));
 
@@ -1689,15 +1622,15 @@ static int sbp2_send_command(struct sbp2_lu *lu, struct scsi_cmnd *SCpnt,
 			     void (*done)(struct scsi_cmnd *))
 {
 	unchar *scsi_cmd = (unchar *)SCpnt->cmnd;
-	unsigned int request_bufflen = SCpnt->request_bufflen;
+	unsigned int request_bufflen = scsi_bufflen(SCpnt);
 	struct sbp2_command_info *cmd;
 
 	cmd = sbp2util_allocate_command_orb(lu, SCpnt, done);
 	if (!cmd)
 		return -EIO;
 
-	sbp2_create_command_orb(lu, cmd, scsi_cmd, SCpnt->use_sg,
-				request_bufflen, SCpnt->request_buffer,
+	sbp2_create_command_orb(lu, cmd, scsi_cmd, scsi_sg_count(SCpnt),
+				request_bufflen, scsi_sglist(SCpnt),
 				SCpnt->sc_data_direction);
 	sbp2_link_orb_command(lu, cmd);
 
-- 
1.4.4.3

Re: [PATCH 0/19] clean ups on the drivers

[Christoph Hellwig]

On Mon, May 14, 2007 at 05:39:06PM +0200, Stefan Richter wrote:
> FUJITA Tomonori wrote:
> >> > There are two patches for each driver, removing the non-use-sg code
> >> > and converting to use the accessors.
> ...
> > I merged the two. And I finished cleaning up 35 drivers in total.
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-bidi.git cleanups
> 
> According to what I see via gitweb, at least usb-storage, ieee1394/sbp2,
> and firewire/fw-sbp2 weren't treated yet.  If somebody does so for the
> latter two:  The shost->shost_gendev.parent has to be changed for
> scsi_dma_{un}map.  I don't know if this can be done without breaking
> anything.

That means they should not be converted to these helpers for now.

Personally I'd still love to have the dma mapping routines to work on
any given struct device but walking up the parent chain until an iommu
is found, but that was vetoed when first proposed.

Re: [PATCH 0/19] clean ups on the drivers

[James Bottomley]

On Tue, 2007-05-15 at 10:01 +0100, Christoph Hellwig wrote:
> On Mon, May 14, 2007 at 05:39:06PM +0200, Stefan Richter wrote:
> > FUJITA Tomonori wrote:
> > >> > There are two patches for each driver, removing the non-use-sg code
> > >> > and converting to use the accessors.
> > ...
> > > I merged the two. And I finished cleaning up 35 drivers in total.
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-bidi.git cleanups
> > 
> > According to what I see via gitweb, at least usb-storage, ieee1394/sbp2,
> > and firewire/fw-sbp2 weren't treated yet.  If somebody does so for the
> > latter two:  The shost->shost_gendev.parent has to be changed for
> > scsi_dma_{un}map.  I don't know if this can be done without breaking
> > anything.
> 
> That means they should not be converted to these helpers for now.
> 
> Personally I'd still love to have the dma mapping routines to work on
> any given struct device but walking up the parent chain until an iommu
> is found, but that was vetoed when first proposed.

Er ... I really hope not ... that's exactly how the parisc iommu
platform code works ... and why I designed the generic dma mapping this
way.  The key thing parisc needed was the ability to walk up different
busses until it found the iommu (for example the pci bus -> dino -> GSC
-> IOMMU) which it does by traversing the dev->parent; However, I didn't
mandate working this way for other architectures.

James

Re: [PATCH 0/19] clean ups on the drivers

[Christoph Hellwig]

On Tue, May 15, 2007 at 06:54:16AM -0500, James Bottomley wrote:
> Er ... I really hope not ... that's exactly how the parisc iommu
> platform code works ... and why I designed the generic dma mapping this
> way.  The key thing parisc needed was the ability to walk up different
> busses until it found the iommu (for example the pci bus -> dino -> GSC
> -> IOMMU) which it does by traversing the dev->parent; However, I didn't
> mandate working this way for other architectures.

Well, the NACK was not for the implementation details but rather the
exported and documented interface.  IIRC the rationale was that Dave wants
to keep the sparc implementation super-optimized and avoid indirection.

If it was up to me I'd have something like the parisc implementation in
generic code.

Re: [PATCH 0/19] clean ups on the drivers

[James Bottomley]

On Tue, 2007-05-15 at 12:57 +0100, Christoph Hellwig wrote:
> On Tue, May 15, 2007 at 06:54:16AM -0500, James Bottomley wrote:
> > Er ... I really hope not ... that's exactly how the parisc iommu
> > platform code works ... and why I designed the generic dma mapping this
> > way.  The key thing parisc needed was the ability to walk up different
> > busses until it found the iommu (for example the pci bus -> dino -> GSC
> > -> IOMMU) which it does by traversing the dev->parent; However, I didn't
> > mandate working this way for other architectures.
> 
> Well, the NACK was not for the implementation details but rather the
> exported and documented interface.  IIRC the rationale was that Dave wants
> to keep the sparc implementation super-optimized and avoid indirection.

Well, actually, I do understand that.  How about the weaker requirement
that you be able to call the dma_ functions on any dev (provided it's
properly parented) and leave the implementation up to the platform?

> If it was up to me I'd have something like the parisc implementation in
> generic code.

It might make sense to put it in lib ... however, I don't think many
architectures have the problems we have ... specifically certain boxes
can have >1 IOMMU, then you really have to know *which* iommu you're
programming.

James

Re: [PATCH 0/19] clean ups on the drivers

[David Miller]

From: James Bottomley <James.Bottomley@SteelEye.com>
Date: Tue, 15 May 2007 12:57:25 -0400

> It might make sense to put it in lib ... however, I don't think many
> architectures have the problems we have ... specifically certain boxes
> can have >1 IOMMU, then you really have to know *which* iommu you're
> programming.

Sparc64 boxes will have this at some point, but what I'm
going to do is simply fill in the dev_archdata properly at
device scan time.

Instead of doing a bus walk up the parent every IOMMU request, why not
cache those results in the dev_archdata?  That seems to make the most
sense.

If there is some probing complexity, catch not-setup dev_archdata at
the IOMMU request, and slow path into a resolver of some sort.

Re: [PATCH 0/19] clean ups on the drivers

[James Bottomley]

On Tue, 2007-05-15 at 13:37 -0700, David Miller wrote:
> From: James Bottomley <James.Bottomley@SteelEye.com>
> Date: Tue, 15 May 2007 12:57:25 -0400
> 
> > It might make sense to put it in lib ... however, I don't think many
> > architectures have the problems we have ... specifically certain boxes
> > can have >1 IOMMU, then you really have to know *which* iommu you're
> > programming.
> 
> Sparc64 boxes will have this at some point, but what I'm
> going to do is simply fill in the dev_archdata properly at
> device scan time.
> 
> Instead of doing a bus walk up the parent every IOMMU request, why not
> cache those results in the dev_archdata?  That seems to make the most
> sense.

That is what we do ... except parisc caches the result in platform_data,
largely because dev_archdata wasn't around when the scheme was
concocted.  However, if we find a NULL platform_data we walk up the tree
until we find a non-NULL value and use that (IOMMUs are on a
disconnected bus topology).

> If there is some probing complexity, catch not-setup dev_archdata at
> the IOMMU request, and slow path into a resolver of some sort.

Yes, that's where we do the walk if the cache is NULL.

James

Re: [PATCH 0/19] clean ups on the drivers

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: Re: [PATCH 0/19] clean ups on the drivers
Date: Mon, 14 May 2007 23:40:03 +0900

> From: Christoph Hellwig <hch@infradead.org>
> Subject: Re: [PATCH 0/19] clean ups on the drivers
> Date: Sat, 12 May 2007 16:30:23 +0100
> 
> > On Sat, May 12, 2007 at 07:05:42PM +0900, FUJITA Tomonori wrote:
> > > There are two patches for each driver, removing the non-use-sg code
> > > and converting to use the accessors.
> > 
> > One patch would probably be fine aswell.
> 
> I merged the two. And I finished cleaning up 35 drivers in total.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-bidi.git cleanups

I rebased the cleanup patchset against the latest scsi-misc
(352e921f0dd42f79652cdb50dd91122d068d7209). It includes the cleanups
for more than 40 drivers.