[PATCHSET 0/3] pluto/fc - some fixes and cleanups

[PATCHSET 0/3] pluto/fc - some fixes and cleanups

[Boaz Harrosh]

I'm sending a small lift-up to the drivers/scsi/pluto.c and
drivers/fc4/fc.c pair, that where a bit stepped on lately.

Matthew this includes your patch, I just fixed up the patch
comment, since You had a good comment on the first patch
but sent a better second patch with no comment. (And my set
depends on yours so just to make sure.)

This is only compile tested, and from patch 3/3 below the
driver is in grate need of testers.

[patch 1/3] pluto/fc - Remove uses of the scsi_cmnd ->done
  This is Matthew's patch with right commit-log inline?

[patch 2/3] pluto/fc - Enable compilation for all ARCHs
  I'm not sure if the final verdict was yes on this one,
  but it's what I needed in order to compile this.

[patch 3/3] pluto/fc - fix INQUIRY still using !use_sg commands
  So this is broken and, can some body explain to me how this was
  checked, because it looks to me that it should surely crash
  terribly. pluto.c would send use_sg == 0 command on detection
  but fc.c was already converted.
  What this patch does is effectively reverts some of Tomo's
  !use_sg cleanup, but only for the INQUIRY command that is
  issued internally by pluto.c. (Sorry)

Happy testing
Boaz

[PATCH 1/3] pluto/fc - Remove uses of the scsi_cmnd->done

[Boaz Harrosh]

From: Matthew Wilcox <matthew@wil.cx>

Remove uses of the scsi_cmnd ->done method from the fc4 driver.  It was
being abused to flag commands that had already been through queuecommand;
use the fcmd->proto value for that instead.  The fcmd->done pointer now
becomes irrelevant.  Reuse the fcp_scsi_done name for an entirely different
function which provides a handy single place to call ->scsi_done (removing
some broken places that used to leak scsi_cmnds on error by calling ->done).

I had to add an include of <linux/pci.h> to get this to compile on non-sparc,
and pluto_detect_done() was unused, so remove it too.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/fc4/fc.c       |   64 ++++++++++++++++-------------------------------
 drivers/fc4/fcp_impl.h |    1 -
 drivers/scsi/pluto.c   |   10 +------
 3 files changed, 23 insertions(+), 52 deletions(-)

diff --git a/drivers/fc4/fc.c b/drivers/fc4/fc.c
index 82de9e1..48c3b62 100644
--- a/drivers/fc4/fc.c
+++ b/drivers/fc4/fc.c
@@ -33,6 +33,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/init.h>
+#include <linux/pci.h>
 
 #include <asm/pgtable.h>
 #include <asm/irq.h>
@@ -378,7 +379,12 @@ void fcp_register(fc_channel *fc, u8 type, int unregister)
 		printk ("FC: %segistering unknown type %02x\n", unregister ? "Unr" : "R", type);
 }
 
-static void fcp_scsi_done(struct scsi_cmnd *SCpnt);
+static void fcp_scsi_done(struct scsi_cmnd *SCpnt, unsigned int result)
+{
+	SCpnt->result = result;
+	FCD(("Calling scsi_done with %08x\n", SCpnt->result))
+	SCpnt->scsi_done(SCpnt);
+}
 
 static inline void fcp_scsi_receive(fc_channel *fc, int token, int status, fc_hdr *fch)
 {
@@ -394,9 +400,6 @@ static inline void fcp_scsi_receive(fc_channel *fc, int token, int status, fc_hd
 	rsp = (fcp_rsp *) (fc->scsi_rsp_pool + fc->rsp_size * token);
 	SCpnt = SC_FCMND(fcmd);
 
-	if (SCpnt->done != fcp_scsi_done)
-		return;
-
 	rsp_status = rsp->fcp_status;
 	FCD(("rsp_status %08x status %08x\n", rsp_status, status))
 	switch (status) {
@@ -442,17 +445,9 @@ static inline void fcp_scsi_receive(fc_channel *fc, int token, int status, fc_hd
 		printk ("%s: (%d,%d) Received rsp_status 0x%x\n", fc->name, SCpnt->device->channel, SCpnt->device->id, rsp_status);
 	}	
 	
-	SCpnt->result = (host_status << 16) | (rsp_status & 0xff);
-#ifdef FCDEBUG	
-	if (host_status || SCpnt->result || rsp_status) printk("FC: host_status %d, packet status %d\n",
-			host_status, SCpnt->result);
-#endif
-	SCpnt->done = fcmd->done;
-	fcmd->done=NULL;
 	clear_bit(token, fc->scsi_bitmap);
 	fc->scsi_free++;
-	FCD(("Calling scsi_done with %08x\n", SCpnt->result))
-	SCpnt->scsi_done(SCpnt);
+	fcp_scsi_done(SCpnt, (host_status << 16) | (rsp_status & 0xff));
 }
 
 void fcp_receive_solicited(fc_channel *fc, int proto, int token, int status, fc_hdr *fch)
@@ -751,13 +746,6 @@ void fcp_release(fc_channel *fcchain, int count)  /* count must > 0 */
 	 */
 }
 
-
-static void fcp_scsi_done(struct scsi_cmnd *SCpnt)
-{
-	if (FCP_CMND(SCpnt)->done)
-		FCP_CMND(SCpnt)->done(SCpnt);
-}
-
 static int fcp_scsi_queue_it(fc_channel *fc, struct scsi_cmnd *SCpnt,
 			     fcp_cmnd *fcmd, int prepare)
 {
@@ -773,8 +761,7 @@ static int fcp_scsi_queue_it(fc_channel *fc, struct scsi_cmnd *SCpnt,
 		if (fc->encode_addr (SCpnt, cmd->fcp_addr, fc, fcmd)) {
 			/* Invalid channel/id/lun and couldn't map it into fcp_addr */
 			clear_bit (i, fc->scsi_bitmap);
-			SCpnt->result = (DID_BAD_TARGET << 16);
-			SCpnt->scsi_done(SCpnt);
+			fcp_scsi_done(SCpnt, (DID_BAD_TARGET << 16));
 			return 0;
 		}
 		fc->scsi_free--;
@@ -834,21 +821,20 @@ int fcp_scsi_queuecommand(struct scsi_cmnd *SCpnt,
 	fc_channel *fc = FC_SCMND(SCpnt);
 	
 	FCD(("Entering SCSI queuecommand %p\n", fcmd))
-	if (SCpnt->done != fcp_scsi_done) {
-		fcmd->done = SCpnt->done;
-		SCpnt->done = fcp_scsi_done;
-		SCpnt->scsi_done = done;
-		fcmd->proto = TYPE_SCSI_FCP;
-		if (!fc->scsi_free) {
-			FCD(("FC: !scsi_free, putting cmd on ML queue\n"))
+	SCpnt->scsi_done = done;
+	if (!fc->scsi_free) {
+		FCD(("FC: !scsi_free, putting cmd on ML queue\n"))
 #if (FCP_SCSI_USE_NEW_EH_CODE == 0)
-			printk("fcp_scsi_queue_command: queue full, losing cmd, bad\n");
+		printk("fcp_scsi_queue_command: queue full, losing cmd, bad\n");
 #endif
-			return 1;
-		}
+		return 1;
+	}
+	if (fcmd->proto == TYPE_SCSI_FCP) {
+		return fcp_scsi_queue_it(fc, SCpnt, fcmd, 0);
+	} else {
+		fcmd->proto = TYPE_SCSI_FCP;
 		return fcp_scsi_queue_it(fc, SCpnt, fcmd, 1);
 	}
-	return fcp_scsi_queue_it(fc, SCpnt, fcmd, 0);
 }
 
 void fcp_queue_empty(fc_channel *fc)
@@ -867,7 +853,6 @@ void fcp_queue_empty(fc_channel *fc)
 int fcp_scsi_abort(struct scsi_cmnd *SCpnt)
 {
 	/* Internal bookkeeping only. Lose 1 cmd_slots slot. */
-	fcp_cmnd *fcmd = FCP_CMND(SCpnt);
 	fc_channel *fc = FC_SCMND(SCpnt);
 	
 	/*
@@ -890,8 +875,7 @@ int fcp_scsi_abort(struct scsi_cmnd *SCpnt)
 	 */
 
 	if (++fc->abort_count < (fc->can_queue >> 1)) {
-		SCpnt->result = DID_ABORT;
-		fcmd->done(SCpnt);
+		fcp_scsi_done(SCpnt, DID_ABORT);
 		printk("FC: soft abort\n");
 		return SUCCESS;
 	} else {
@@ -949,8 +933,6 @@ int fcp_scsi_dev_reset(struct scsi_cmnd *SCpnt)
 		if (fc->rst_pkt->eh_state == SCSI_STATE_QUEUED)
 			return FAILED; /* or SUCCESS. Only these */
 	}
-	fc->rst_pkt->done = NULL;
-
 
         fc->rst_pkt->eh_state = SCSI_STATE_QUEUED;
 	init_timer(&fc->rst_pkt->eh_timeout);
@@ -966,7 +948,7 @@ int fcp_scsi_dev_reset(struct scsi_cmnd *SCpnt)
 
 	fc->rst_pkt->device->host->eh_action = &sem;
 
-	fc->rst_pkt->done = fcp_scsi_reset_done;
+	fc->rst_pkt->scsi_done = fcp_scsi_reset_done;
 
 	spin_lock_irqsave(SCpnt->device->host->host_lock, flags);
 	fcp_scsi_queue_it(fc, fc->rst_pkt, fcmd, 0);
@@ -993,15 +975,13 @@ int fcp_scsi_dev_reset(struct scsi_cmnd *SCpnt)
 static int __fcp_scsi_host_reset(struct scsi_cmnd *SCpnt)
 {
 	fc_channel *fc = FC_SCMND(SCpnt);
-	fcp_cmnd *fcmd = FCP_CMND(SCpnt);
 	int i;
 
 	printk ("FC: host reset\n");
 
 	for (i=0; i < fc->can_queue; i++) {
 		if (fc->cmd_slots[i] && SCpnt->result != DID_ABORT) {
-			SCpnt->result = DID_RESET;
-			fcmd->done(SCpnt);
+			fcp_scsi_done(SCpnt, DID_RESET);
 			fc->cmd_slots[i] = NULL;
 		}
 	}
diff --git a/drivers/fc4/fcp_impl.h b/drivers/fc4/fcp_impl.h
index 1ac6133..41fa149 100644
--- a/drivers/fc4/fcp_impl.h
+++ b/drivers/fc4/fcp_impl.h
@@ -39,7 +39,6 @@ struct _fc_channel;
 typedef struct fcp_cmnd {
 	struct fcp_cmnd		*next;
 	struct fcp_cmnd		*prev;
-	void			(*done)(struct scsi_cmnd *);
 	unsigned short		proto;
 	unsigned short		token;
 	unsigned int		did;
diff --git a/drivers/scsi/pluto.c b/drivers/scsi/pluto.c
index 0363c1c..e598a90 100644
--- a/drivers/scsi/pluto.c
+++ b/drivers/scsi/pluto.c
@@ -55,14 +55,9 @@ static DECLARE_COMPLETION(fc_detect_complete);
 
 static int pluto_encode_addr(Scsi_Cmnd *SCpnt, u16 *addr, fc_channel *fc, fcp_cmnd *fcmd);
 
-static void __init pluto_detect_done(Scsi_Cmnd *SCpnt)
-{
-	/* Do nothing */
-}
-
 static void __init pluto_detect_scsi_done(Scsi_Cmnd *SCpnt)
 {
-	PLND(("Detect done %08lx\n", (long)SCpnt))
+	PLND(("Detect done %p\n", SCpnt))
 	if (atomic_dec_and_test (&fcss))
 		complete(&fc_detect_complete);
 }
@@ -193,9 +188,6 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 	
 		SCpnt = &(fcs[i].cmd);
 		
-		/* Let FC mid-level free allocated resources */
-		pluto_detect_scsi_done(SCpnt);
-		
 		if (!SCpnt->result) {
 			struct pluto_inquiry *inq;
 			struct pluto *pluto;
-- 
1.5.3.1

Re: [PATCH 1/3] pluto/fc - Remove uses of the scsi_cmnd->done

[Matthew Wilcox]

On Mon, Oct 15, 2007 at 07:25:14PM +0200, Boaz Harrosh wrote:
> From: Matthew Wilcox <matthew@wil.cx>
> 
> Remove uses of the scsi_cmnd ->done method from the fc4 driver.  It was
> being abused to flag commands that had already been through queuecommand;
> use the fcmd->proto value for that instead.  The fcmd->done pointer now
> becomes irrelevant.  Reuse the fcp_scsi_done name for an entirely different
> function which provides a handy single place to call ->scsi_done (removing
> some broken places that used to leak scsi_cmnds on error by calling ->done).

Looks right to me.  Thanks for picking this up.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH 2/3] pluto/fc - Enable compilation for all ARCHs

[Boaz Harrosh]

  - It was suggested on the linux-scsi-ml that:
    "Well if fc4.c compiles OK on non-sparc64 then perhaps we should
     enable compilation on non-sparc64.  It will increase maintainability
     and code quality and stuff."

  - WATCH OUT Distro maintainers:
	"otoh people might end up shipping useless drivers in x86 distros."

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 arch/sparc64/Kconfig |    2 --
 drivers/Kconfig      |    2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/sparc64/Kconfig b/arch/sparc64/Kconfig
index 33dabf5..897afd8 100644
--- a/arch/sparc64/Kconfig
+++ b/arch/sparc64/Kconfig
@@ -456,8 +456,6 @@ source "drivers/Kconfig"
 
 source "drivers/sbus/char/Kconfig"
 
-source "drivers/fc4/Kconfig"
-
 source "fs/Kconfig"
 
 menu "Instrumentation Support"
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 3e1c442..b57232d 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -22,6 +22,8 @@ source "drivers/misc/Kconfig"
 
 source "drivers/ide/Kconfig"
 
+source "drivers/fc4/Kconfig"
+
 source "drivers/scsi/Kconfig"
 
 source "drivers/ata/Kconfig"
-- 
1.5.3.1

Re: [PATCH 2/3] pluto/fc - Enable compilation for all ARCHs

[Matthew Wilcox]

On Mon, Oct 15, 2007 at 07:26:51PM +0200, Boaz Harrosh wrote:
> @@ -22,6 +22,8 @@ source "drivers/misc/Kconfig"
>  
>  source "drivers/ide/Kconfig"
>  
> +source "drivers/fc4/Kconfig"
> +
>  source "drivers/scsi/Kconfig"
>  
>  source "drivers/ata/Kconfig"

I think I see two problems ... one is that fc4 plainly depends on SCSI,
yet it's not mentioned in the fc4 Kconfig file.  The other is that,
given this dependency, fc4 should come after scsi.  And maybe even be
included from drivers/scsi/Kconfig rather than drivers/Kconfig.  I don't
have a strong feeling on that last point.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2/3] pluto/fc - Enable compilation for all ARCHs

[Boaz Harrosh]

Matthew Wilcox wrote:
> On Mon, Oct 15, 2007 at 07:26:51PM +0200, Boaz Harrosh wrote:
>> @@ -22,6 +22,8 @@ source "drivers/misc/Kconfig"
>>  
>>  source "drivers/ide/Kconfig"
>>  
>> +source "drivers/fc4/Kconfig"
>> +
>>  source "drivers/scsi/Kconfig"
>>  
>>  source "drivers/ata/Kconfig"
> 
> I think I see two problems ... one is that fc4 plainly depends on SCSI,
> yet it's not mentioned in the fc4 Kconfig file.  The other is that,
> given this dependency, fc4 should come after scsi.  And maybe even be
> included from drivers/scsi/Kconfig rather than drivers/Kconfig.  I don't
> have a strong feeling on that last point.
> 

This is all new territory for me. But CONFIG_SCSI_PLUTO is dependent on
SCSI and fc.c is not the real driver just the needed bits from the sparc
side. So the code mess calls for a Kconfig mess, I guess.

Please, anyone, try something better, it is the best I could think of.

Thanks Boaz

Re: [PATCH 2/3] pluto/fc - Enable compilation for all ARCHs

[Matthew Wilcox]

On Mon, Oct 15, 2007 at 08:18:24PM +0200, Boaz Harrosh wrote:
> This is all new territory for me. But CONFIG_SCSI_PLUTO is dependent on
> SCSI and fc.c is not the real driver just the needed bits from the sparc
> side. So the code mess calls for a Kconfig mess, I guess.

I've spent a lot of time looking at pluto/fc4 recently.  And I'm very
much still in a 'wtf' mode.  Pluto creates a new scsi *host* for every
Sun Storage Array proessor *device* on the SCSI bus.  I have no clue
why it does this.

I'm sure there's a sane sensible driver or three trying to get out from
inside the pluto/soc/socal/fc4 maze, but I'm not quite sure what form
it should take.

Dave, can you explain what's going on?  I really want to help this
driver fit the current scsi model better, but I don't understand
what it's trying to do.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH 1/2] fc4/pluto - Enable compilation for all ARCHs

[Randy Dunlap]

> > I think I see two problems ... one is that fc4 plainly depends on SCSI,

Does it?  It builds with SCSI=n.

> > yet it's not mentioned in the fc4 Kconfig file.  The other is that,
> > given this dependency, fc4 should come after scsi.  And maybe even be
> > included from drivers/scsi/Kconfig rather than drivers/Kconfig.  I don't
> > have a strong feeling on that last point.

Me either.  I moved it to after scsi/Kconfig.

> This is all new territory for me. But CONFIG_SCSI_PLUTO is dependent on
> SCSI and fc.c is not the real driver just the needed bits from the sparc
> side. So the code mess calls for a Kconfig mess, I guess.
> 
> Please, anyone, try something better, it is the best I could think of.

Here's my attempt.
patch 1/2 for fc4.
patch 2/2 for one more pluto fix.

---

From: Randy Dunlap <randy.dunlap@oracle.com>

Move drivers/fc4/Kconfig to the main drivers/Kconfig file & menu.
This allows any $arch to build fc4.

fc.c (on non-sparc) uses pci interfaces, so include pci.h and
make FC4 depend on PCI || SPARC.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 arch/sparc64/Kconfig |    2 --
 drivers/Kconfig      |    2 ++
 drivers/fc4/Kconfig  |    1 +
 drivers/fc4/fc.c     |    1 +
 4 files changed, 4 insertions(+), 2 deletions(-)

--- linux-2.6.23-git7.orig/arch/sparc64/Kconfig
+++ linux-2.6.23-git7/arch/sparc64/Kconfig
@@ -456,8 +456,6 @@ source "drivers/Kconfig"
 
 source "drivers/sbus/char/Kconfig"
 
-source "drivers/fc4/Kconfig"
-
 source "fs/Kconfig"
 
 menu "Instrumentation Support"
--- linux-2.6.23-git7.orig/drivers/Kconfig
+++ linux-2.6.23-git7/drivers/Kconfig
@@ -24,6 +24,8 @@ source "drivers/ide/Kconfig"
 
 source "drivers/scsi/Kconfig"
 
+source "drivers/fc4/Kconfig"
+
 source "drivers/ata/Kconfig"
 
 source "drivers/md/Kconfig"
--- linux-2.6.23-git7.orig/drivers/fc4/fc.c
+++ linux-2.6.23-git7/drivers/fc4/fc.c
@@ -30,6 +30,7 @@
 #include <linux/ptrace.h>
 #include <linux/ioport.h>
 #include <linux/in.h>
+#include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/init.h>
--- linux-2.6.23-git7.orig/drivers/fc4/Kconfig
+++ linux-2.6.23-git7/drivers/fc4/Kconfig
@@ -6,6 +6,7 @@ menu "Fibre Channel support"
 
 config FC4
 	tristate "Fibre Channel and FC4 SCSI support"
+	depends on SPARC || PCI
 	---help---
 	  Fibre Channel is a high speed serial protocol mainly used to
 	  connect large storage devices to the computer; it is compatible with

Re: [PATCH 1/2] fc4/pluto - Enable compilation for all ARCHs

[Boaz Harrosh]

On Mon, Oct 15 2007 at 22:15 +0200, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> --- linux-2.6.23-git7.orig/drivers/fc4/fc.c
> +++ linux-2.6.23-git7/drivers/fc4/fc.c
> @@ -30,6 +30,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/ioport.h>
>  #include <linux/in.h>
> +#include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/init.h>

This bit is not needed it's already done with Matthew's
patch.

The rest looks Good, thanks

Boaz

[PATCH 2/2] pluto fix disable/enable irq

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Pluto drivers uses disable/enable_irq(), so add prototypes for them.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/scsi/pluto.c |    1 +
 1 file changed, 1 insertion(+)

--- linux-2.6.23-git7.orig/drivers/scsi/pluto.c
+++ linux-2.6.23-git7/drivers/scsi/pluto.c
@@ -14,6 +14,7 @@
 #include <linux/proc_fs.h>
 #include <linux/stat.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #ifdef CONFIG_KMOD
 #include <linux/kmod.h>
 #endif

[PATCH 3/3] pluto/fc - fix INQUIRY still using !use_sg commands

[Boaz Harrosh]

  - pluto.c was still issuing use_sg == 0 commands down to
    fc.c, which was already converted. Fix that by adding
    a member to hold the inquiry_buffer in struct fcp_cmnd
    and using it when mapping/unmapping of command payload,
    if needed.

  - Also fix a compilation warning in pluto_info() now that
    driver can be compiled not only on sparc.

  - Stop using struct scsi_cmnd members that will be removed
    soon.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/fc4/fc.c       |   21 +++++++++++++++------
 drivers/fc4/fcp_impl.h |    2 ++
 drivers/scsi/pluto.c   |   14 +++++++-------
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/fc4/fc.c b/drivers/fc4/fc.c
index 48c3b62..3962ba8 100644
--- a/drivers/fc4/fc.c
+++ b/drivers/fc4/fc.c
@@ -429,11 +429,16 @@ static inline void fcp_scsi_receive(fc_channel *fc, int token, int status, fc_hd
 			if (sense_len > sizeof(SCpnt->sense_buffer)) sense_len = sizeof(SCpnt->sense_buffer);
 			memcpy(SCpnt->sense_buffer, ((char *)(rsp+1)), sense_len);
 		}
-		
-		if (fcmd->data)
-			dma_unmap_sg(fc->dev, scsi_sglist(SCpnt),
-				     scsi_sg_count(SCpnt),
-				     SCpnt->sc_data_direction);
+
+		if (fcmd->data) {
+			if (fcmd->inquiry_buff)
+				dma_unmap_single(fc->dev, fcmd->data,
+					      FC_INQUIRY_SIZE, DMA_FROM_DEVICE);
+			else
+				dma_unmap_sg(fc->dev, scsi_sglist(SCpnt),
+					     scsi_sg_count(SCpnt),
+					     SCpnt->sc_data_direction);
+		}
 		break;
 	default:
 		host_status=DID_ERROR; /* FIXME */
@@ -776,7 +781,11 @@ static int fcp_scsi_queue_it(fc_channel *fc, struct scsi_cmnd *SCpnt,
 		} else
 			fcp_cntl = FCP_CNTL_QTYPE_UNTAGGED;
 
-		if (!scsi_bufflen(SCpnt)) {
+		if (fcmd->inquiry_buff) {
+			cmd->fcp_cntl = (FCP_CNTL_READ | fcp_cntl);
+			fcmd->data = dma_map_single(fc->dev, fcmd->inquiry_buff,
+			                      FC_INQUIRY_SIZE, DMA_FROM_DEVICE);
+		} else if (!scsi_bufflen(SCpnt)) {
 			cmd->fcp_cntl = fcp_cntl;
 			fcmd->data = (dma_addr_t)NULL;
 		} else {
diff --git a/drivers/fc4/fcp_impl.h b/drivers/fc4/fcp_impl.h
index 41fa149..968a20e 100644
--- a/drivers/fc4/fcp_impl.h
+++ b/drivers/fc4/fcp_impl.h
@@ -39,6 +39,7 @@ struct _fc_channel;
 typedef struct fcp_cmnd {
 	struct fcp_cmnd		*next;
 	struct fcp_cmnd		*prev;
+	char*			inquiry_buff;
 	unsigned short		proto;
 	unsigned short		token;
 	unsigned int		did;
@@ -138,6 +139,7 @@ extern fc_channel *fc_channels;
 #define FC_STATUS_NO_SEQ_INIT		0x29
 #define FC_STATUS_TIMED_OUT		-1
 #define FC_STATUS_BAD_RSP		-2
+#define FC_INQUIRY_SIZE			256
 
 void fcp_queue_empty(fc_channel *);
 int fcp_init(fc_channel *);
diff --git a/drivers/scsi/pluto.c b/drivers/scsi/pluto.c
index e598a90..1cb8a39 100644
--- a/drivers/scsi/pluto.c
+++ b/drivers/scsi/pluto.c
@@ -46,7 +46,7 @@ static struct ctrl_inquiry {
 	struct Scsi_Host host;
 	struct pluto pluto;
 	Scsi_Cmnd cmd;
-	char inquiry[256];
+	char inquiry[FC_INQUIRY_SIZE];
 	fc_channel *fc;
 } *fcs __initdata;
 static int fcscount __initdata = 0;
@@ -120,6 +120,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		Scsi_Cmnd *SCpnt;
 		struct Scsi_Host *host;
 		struct pluto *pluto;
+		fcp_cmnd *fcmd;
 
 		if (i == fcscount) break;
 		if (fc->posmap) continue;
@@ -141,6 +142,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		SCpnt = &(fcs[i].cmd);
 		host = &(fcs[i].host);
 		pluto = (struct pluto *)host->hostdata;
+		fcmd = ((fcp_cmnd *)&(SCpnt->SCp));
 		
 		pluto->fc = fc;
 	
@@ -155,8 +157,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 	
 		SCpnt->request->cmd_flags &= ~REQ_STARTED;
 		
-		SCpnt->request_bufflen = 256;
-		SCpnt->request_buffer = fcs[i].inquiry;
+		fcmd->inquiry_buff = fcs[i].inquiry;
 		PLD(("set up %d %08lx\n", i, (long)SCpnt))
 		i++;
 	}
@@ -271,16 +272,15 @@ int pluto_release(struct Scsi_Host *host)
 
 const char *pluto_info(struct Scsi_Host *host)
 {
-	static char buf[128], *p;
+	static char buf[128];
 	struct pluto *pluto = (struct pluto *) host->hostdata;
 
 	sprintf(buf, "SUN SparcSTORAGE Array %s fw %s serial %s %dx%d on %s",
 		pluto->rev_str, pluto->fw_rev_str, pluto->serial_str,
 		host->max_channel, host->max_id, pluto->fc->name);
 #ifdef __sparc__
-	p = strchr(buf, 0);
-	sprintf(p, " PROM node %x", pluto->fc->dev->prom_node);
-#endif	
+	sprintf(buf + strlen(buf), " PROM node %x", pluto->fc->dev->prom_node);
+#endif
 	return buf;
 }
 
-- 
1.5.3.1

[PATCH 3/3 ver2] pluto/fc - fix INQUIRY still using !use_sg commands

[Boaz Harrosh]

Some people, me included, might like this approach better

----

  - pluto.c was still issuing use_sg == 0 commands down to
    fc.c, which was already converted. Fix that by adding
    a member to hold the inquiry_sg in struct fcp_cmnd
    and using it when mapping/unmapping of command payload,
    if needed.

  - Also fix a compilation warning in pluto_info() now that
    driver can be compiled not only on sparc.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/fc4/fc.c       |   21 +++++++++++++--------
 drivers/fc4/fcp_impl.h |    2 ++
 drivers/scsi/pluto.c   |   21 +++++++++++++--------
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/fc4/fc.c b/drivers/fc4/fc.c
index 48c3b62..5a0eba5 100644
--- a/drivers/fc4/fc.c
+++ b/drivers/fc4/fc.c
@@ -429,11 +429,15 @@ static inline void fcp_scsi_receive(fc_channel *fc, int token, int status, fc_hd
 			if (sense_len > sizeof(SCpnt->sense_buffer)) sense_len = sizeof(SCpnt->sense_buffer);
 			memcpy(SCpnt->sense_buffer, ((char *)(rsp+1)), sense_len);
 		}
-		
-		if (fcmd->data)
-			dma_unmap_sg(fc->dev, scsi_sglist(SCpnt),
+
+		if (fcmd->data) {
+			struct scatterlist *sg = fcmd->inquiry_sg ?
+				fcmd->inquiry_sg : scsi_sglist(SCpnt);
+
+			dma_unmap_sg(fc->dev, sg,
 				     scsi_sg_count(SCpnt),
 				     SCpnt->sc_data_direction);
+		}
 		break;
 	default:
 		host_status=DID_ERROR; /* FIXME */
@@ -776,12 +780,11 @@ static int fcp_scsi_queue_it(fc_channel *fc, struct scsi_cmnd *SCpnt,
 		} else
 			fcp_cntl = FCP_CNTL_QTYPE_UNTAGGED;
 
-		if (!scsi_bufflen(SCpnt)) {
+		if (!scsi_bufflen(SCpnt) && !fcmd->inquiry_sg) {
 			cmd->fcp_cntl = fcp_cntl;
 			fcmd->data = (dma_addr_t)NULL;
 		} else {
 			struct scatterlist *sg;
-			int nents;
 
 			switch (SCpnt->cmnd[0]) {
 			case WRITE_6:
@@ -792,9 +795,11 @@ static int fcp_scsi_queue_it(fc_channel *fc, struct scsi_cmnd *SCpnt,
 				cmd->fcp_cntl = (FCP_CNTL_READ | fcp_cntl); break;
 			}
 
-			sg = scsi_sglist(SCpnt);
-			nents = dma_map_sg(fc->dev, sg, scsi_sg_count(SCpnt),
-					   SCpnt->sc_data_direction);
+			sg = fcmd->inquiry_sg ? fcmd->inquiry_sg :
+				scsi_sglist(SCpnt);
+
+			BUG_ON(scsi_sg_count(SCpnt) > 1);
+			dma_map_sg(fc->dev, sg, 1, SCpnt->sc_data_direction);
 			fcmd->data = sg_dma_address(sg);
 			cmd->fcp_data_len = sg_dma_len(sg);
 		}
diff --git a/drivers/fc4/fcp_impl.h b/drivers/fc4/fcp_impl.h
index 41fa149..728f36d 100644
--- a/drivers/fc4/fcp_impl.h
+++ b/drivers/fc4/fcp_impl.h
@@ -39,6 +39,7 @@ struct _fc_channel;
 typedef struct fcp_cmnd {
 	struct fcp_cmnd		*next;
 	struct fcp_cmnd		*prev;
+	struct scatterlist	*inquiry_sg;
 	unsigned short		proto;
 	unsigned short		token;
 	unsigned int		did;
@@ -138,6 +139,7 @@ extern fc_channel *fc_channels;
 #define FC_STATUS_NO_SEQ_INIT		0x29
 #define FC_STATUS_TIMED_OUT		-1
 #define FC_STATUS_BAD_RSP		-2
+#define FC_INQUIRY_SIZE			256
 
 void fcp_queue_empty(fc_channel *);
 int fcp_init(fc_channel *);
diff --git a/drivers/scsi/pluto.c b/drivers/scsi/pluto.c
index e598a90..df231d3 100644
--- a/drivers/scsi/pluto.c
+++ b/drivers/scsi/pluto.c
@@ -14,6 +14,8 @@
 #include <linux/proc_fs.h>
 #include <linux/stat.h>
 #include <linux/init.h>
+#include <linux/scatterlist.h>
+
 #ifdef CONFIG_KMOD
 #include <linux/kmod.h>
 #endif
@@ -46,7 +48,8 @@ static struct ctrl_inquiry {
 	struct Scsi_Host host;
 	struct pluto pluto;
 	Scsi_Cmnd cmd;
-	char inquiry[256];
+	char inquiry[FC_INQUIRY_SIZE];
+	struct scatterlist inquiry_sg;
 	fc_channel *fc;
 } *fcs __initdata;
 static int fcscount __initdata = 0;
@@ -120,6 +123,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		Scsi_Cmnd *SCpnt;
 		struct Scsi_Host *host;
 		struct pluto *pluto;
+		fcp_cmnd *fcmd;
 
 		if (i == fcscount) break;
 		if (fc->posmap) continue;
@@ -141,6 +145,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		SCpnt = &(fcs[i].cmd);
 		host = &(fcs[i].host);
 		pluto = (struct pluto *)host->hostdata;
+		fcmd = ((fcp_cmnd *)&(SCpnt->SCp));
 		
 		pluto->fc = fc;
 	
@@ -154,9 +159,10 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		SCpnt->cmd_len = COMMAND_SIZE(INQUIRY);
 	
 		SCpnt->request->cmd_flags &= ~REQ_STARTED;
-		
-		SCpnt->request_bufflen = 256;
-		SCpnt->request_buffer = fcs[i].inquiry;
+		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
+
+		sg_init_one(&fcs[i].inquiry_sg, &fcs[i].inquiry, 256);
+		fcmd->inquiry_sg = &fcs[i].inquiry_sg;
 		PLD(("set up %d %08lx\n", i, (long)SCpnt))
 		i++;
 	}
@@ -271,16 +277,15 @@ int pluto_release(struct Scsi_Host *host)
 
 const char *pluto_info(struct Scsi_Host *host)
 {
-	static char buf[128], *p;
+	static char buf[128];
 	struct pluto *pluto = (struct pluto *) host->hostdata;
 
 	sprintf(buf, "SUN SparcSTORAGE Array %s fw %s serial %s %dx%d on %s",
 		pluto->rev_str, pluto->fw_rev_str, pluto->serial_str,
 		host->max_channel, host->max_id, pluto->fc->name);
 #ifdef __sparc__
-	p = strchr(buf, 0);
-	sprintf(p, " PROM node %x", pluto->fc->dev->prom_node);
-#endif	
+	sprintf(buf + strlen(buf), " PROM node %x", pluto->fc->dev->prom_node);
+#endif
 	return buf;
 }
 
-- 
1.5.3.1

Re: [PATCH 3/3 ver2] pluto/fc - fix INQUIRY still using !use_sg commands

[Matthew Wilcox]

On Mon, Oct 15, 2007 at 08:00:22PM +0200, Boaz Harrosh wrote:
> Some people, me included, might like this approach better

I think I prefer this approach too.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH 3/3 ver3] pluto/fc - fix INQUIRY still using !use_sg commands

[Boaz Harrosh]

oofff that was to fast, sorry. Wrong sg_count in unmapping.

---

  - pluto.c was still issuing use_sg == 0 commands down to
    fc.c, which was already converted. Fix that by adding
    a member to hold the inquiry_sg in struct fcp_cmnd
    and using it when mapping/unmapping of command payload,
    if needed.

  - Also fix a compilation warning in pluto_info() now that
    driver can be compiled not only on sparc.

  - Stop using struct scsi_cmnd members that will be removed
    soon.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/fc4/fc.c       |   23 +++++++++++++----------
 drivers/fc4/fcp_impl.h |    2 ++
 drivers/scsi/pluto.c   |   21 +++++++++++++--------
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/fc4/fc.c b/drivers/fc4/fc.c
index 48c3b62..631e4ad 100644
--- a/drivers/fc4/fc.c
+++ b/drivers/fc4/fc.c
@@ -429,11 +429,13 @@ static inline void fcp_scsi_receive(fc_channel *fc, int token, int status, fc_hd
 			if (sense_len > sizeof(SCpnt->sense_buffer)) sense_len = sizeof(SCpnt->sense_buffer);
 			memcpy(SCpnt->sense_buffer, ((char *)(rsp+1)), sense_len);
 		}
-		
-		if (fcmd->data)
-			dma_unmap_sg(fc->dev, scsi_sglist(SCpnt),
-				     scsi_sg_count(SCpnt),
-				     SCpnt->sc_data_direction);
+
+		if (fcmd->data) {
+			struct scatterlist *sg = fcmd->inquiry_sg ?
+				fcmd->inquiry_sg : scsi_sglist(SCpnt);
+
+			dma_unmap_sg(fc->dev, sg, 1, SCpnt->sc_data_direction);
+		}
 		break;
 	default:
 		host_status=DID_ERROR; /* FIXME */
@@ -776,12 +778,11 @@ static int fcp_scsi_queue_it(fc_channel *fc, struct scsi_cmnd *SCpnt,
 		} else
 			fcp_cntl = FCP_CNTL_QTYPE_UNTAGGED;
 
-		if (!scsi_bufflen(SCpnt)) {
+		if (!scsi_bufflen(SCpnt) && !fcmd->inquiry_sg) {
 			cmd->fcp_cntl = fcp_cntl;
 			fcmd->data = (dma_addr_t)NULL;
 		} else {
 			struct scatterlist *sg;
-			int nents;
 
 			switch (SCpnt->cmnd[0]) {
 			case WRITE_6:
@@ -792,9 +793,11 @@ static int fcp_scsi_queue_it(fc_channel *fc, struct scsi_cmnd *SCpnt,
 				cmd->fcp_cntl = (FCP_CNTL_READ | fcp_cntl); break;
 			}
 
-			sg = scsi_sglist(SCpnt);
-			nents = dma_map_sg(fc->dev, sg, scsi_sg_count(SCpnt),
-					   SCpnt->sc_data_direction);
+			sg = fcmd->inquiry_sg ? fcmd->inquiry_sg :
+				scsi_sglist(SCpnt);
+
+			BUG_ON(scsi_sg_count(SCpnt) > 1);
+			dma_map_sg(fc->dev, sg, 1, SCpnt->sc_data_direction);
 			fcmd->data = sg_dma_address(sg);
 			cmd->fcp_data_len = sg_dma_len(sg);
 		}
diff --git a/drivers/fc4/fcp_impl.h b/drivers/fc4/fcp_impl.h
index 41fa149..728f36d 100644
--- a/drivers/fc4/fcp_impl.h
+++ b/drivers/fc4/fcp_impl.h
@@ -39,6 +39,7 @@ struct _fc_channel;
 typedef struct fcp_cmnd {
 	struct fcp_cmnd		*next;
 	struct fcp_cmnd		*prev;
+	struct scatterlist	*inquiry_sg;
 	unsigned short		proto;
 	unsigned short		token;
 	unsigned int		did;
@@ -138,6 +139,7 @@ extern fc_channel *fc_channels;
 #define FC_STATUS_NO_SEQ_INIT		0x29
 #define FC_STATUS_TIMED_OUT		-1
 #define FC_STATUS_BAD_RSP		-2
+#define FC_INQUIRY_SIZE			256
 
 void fcp_queue_empty(fc_channel *);
 int fcp_init(fc_channel *);
diff --git a/drivers/scsi/pluto.c b/drivers/scsi/pluto.c
index e598a90..df231d3 100644
--- a/drivers/scsi/pluto.c
+++ b/drivers/scsi/pluto.c
@@ -14,6 +14,8 @@
 #include <linux/proc_fs.h>
 #include <linux/stat.h>
 #include <linux/init.h>
+#include <linux/scatterlist.h>
+
 #ifdef CONFIG_KMOD
 #include <linux/kmod.h>
 #endif
@@ -46,7 +48,8 @@ static struct ctrl_inquiry {
 	struct Scsi_Host host;
 	struct pluto pluto;
 	Scsi_Cmnd cmd;
-	char inquiry[256];
+	char inquiry[FC_INQUIRY_SIZE];
+	struct scatterlist inquiry_sg;
 	fc_channel *fc;
 } *fcs __initdata;
 static int fcscount __initdata = 0;
@@ -120,6 +123,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		Scsi_Cmnd *SCpnt;
 		struct Scsi_Host *host;
 		struct pluto *pluto;
+		fcp_cmnd *fcmd;
 
 		if (i == fcscount) break;
 		if (fc->posmap) continue;
@@ -141,6 +145,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		SCpnt = &(fcs[i].cmd);
 		host = &(fcs[i].host);
 		pluto = (struct pluto *)host->hostdata;
+		fcmd = ((fcp_cmnd *)&(SCpnt->SCp));
 		
 		pluto->fc = fc;
 	
@@ -154,9 +159,10 @@ int __init pluto_detect(struct scsi_host_template *tpnt)
 		SCpnt->cmd_len = COMMAND_SIZE(INQUIRY);
 	
 		SCpnt->request->cmd_flags &= ~REQ_STARTED;
-		
-		SCpnt->request_bufflen = 256;
-		SCpnt->request_buffer = fcs[i].inquiry;
+		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
+
+		sg_init_one(&fcs[i].inquiry_sg, &fcs[i].inquiry, 256);
+		fcmd->inquiry_sg = &fcs[i].inquiry_sg;
 		PLD(("set up %d %08lx\n", i, (long)SCpnt))
 		i++;
 	}
@@ -271,16 +277,15 @@ int pluto_release(struct Scsi_Host *host)
 
 const char *pluto_info(struct Scsi_Host *host)
 {
-	static char buf[128], *p;
+	static char buf[128];
 	struct pluto *pluto = (struct pluto *) host->hostdata;
 
 	sprintf(buf, "SUN SparcSTORAGE Array %s fw %s serial %s %dx%d on %s",
 		pluto->rev_str, pluto->fw_rev_str, pluto->serial_str,
 		host->max_channel, host->max_id, pluto->fc->name);
 #ifdef __sparc__
-	p = strchr(buf, 0);
-	sprintf(p, " PROM node %x", pluto->fc->dev->prom_node);
-#endif	
+	sprintf(buf + strlen(buf), " PROM node %x", pluto->fc->dev->prom_node);
+#endif
 	return buf;
 }
 
-- 
1.5.3.1