Skip to contentSkip to conversation

pluto/fc - some fixes and cleanups

PATCH In review Boaz Harrosh 3 participants 14 messages Oct 15, 2007 – Oct 16, 2007 SCSI

lore
Share & tools
lore

Boaz Harrosh

author
#0 lore raw

cover pluto/fc - some fixes and cleanups

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

author
#1 lore raw

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

From: Matthew Wilcox <[email protected]>

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 <[email protected]>
Signed-off-by Boaz Harrosh <[email protected]>
---
 drivers/fc4/fc.c       |   64 ++++++++++++++++-------------------------------
 drivers/fc4/fcp_impl.h |    1 -
 drivers/scsi/pluto.c   |   10 +------
 3 files changed, 23 insertions(+), 52 deletions(-)
drivers/fc4/fc.c +22 -42
index 82de9e1..48c3b62 100644
@@ -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;
 		}
 	}
drivers/fc4/fcp_impl.h +0 -1
index 1ac6133..41fa149 100644
@@ -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;
drivers/scsi/pluto.c +1 -9
index 0363c1c..e598a90 100644
@@ -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;
Signature · 1 line
1.5.3.1
#2 lore raw

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

Quoted 8 lines · Boaz Harrosh

From: Matthew Wilcox <[email protected]>

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.

Signature · 4 lines
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."
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 arch/sparc64/Kconfig |    2 --
 drivers/Kconfig      |    2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/sparc64/Kconfig +0 -2
index 33dabf5..897afd8 100644
@@ -456,8 +456,6 @@ source "drivers/Kconfig"
 
 source "drivers/sbus/char/Kconfig"
 
-source "drivers/fc4/Kconfig"
-
 source "fs/Kconfig"
 
 menu "Instrumentation Support"
drivers/Kconfig +2 -0
index 3e1c442..b57232d 100644
@@ -22,6 +22,8 @@ source "drivers/misc/Kconfig"
 
 source "drivers/ide/Kconfig"
 
+source "drivers/fc4/Kconfig"
+
 source "drivers/scsi/Kconfig"
 
 source "drivers/ata/Kconfig"
Signature · 1 line
1.5.3.1
#4 lore raw

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

drivers/Kconfig source "drivers/misc/Kconfig" · line 29

Quoted 9 lines · Boaz Harrosh
drivers/Kconfig +2 -0
@@ -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.

Signature · 4 lines
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."
author
#5 lore raw

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

Quoted 9 lines · Matthew Wilcox
Quoted 8 lines · Boaz Harrosh
drivers/Kconfig +2 -0
@@ -22,6 +22,8 @@ source "drivers/misc/Kconfig"
 
 source "drivers/ide/Kconfig"
 
+source "drivers/fc4/Kconfig"
+
 source "drivers/scsi/Kconfig"
Quoted 8 lines

drivers/Kconfig source "drivers/misc/Kconfig" · line 29

Quoted 1 line
drivers/Kconfig +0 -0
 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

#6 lore raw

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

Quoted 3 lines · Boaz Harrosh

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.

Signature · 4 lines
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."
#7 lore raw

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

In reply to Boaz Harrosh
Quoted 1 line
Quoted 1 line

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

Does it? It builds with SCSI=n.

Quoted 4 lines
Quoted 4 lines

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.

Quoted 5 lines

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 <[email protected]>

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 <[email protected]>
---
 arch/sparc64/Kconfig |    2 --
 drivers/Kconfig      |    2 ++
 drivers/fc4/Kconfig  |    1 +
 drivers/fc4/fc.c     |    1 +
 4 files changed, 4 insertions(+), 2 deletions(-)
arch/sparc64/Kconfig +0 -2
@@ -456,8 +456,6 @@ source "drivers/Kconfig"
 
 source "drivers/sbus/char/Kconfig"
 
-source "drivers/fc4/Kconfig"
-
 source "fs/Kconfig"
 
 menu "Instrumentation Support"
drivers/Kconfig +2 -0
@@ -24,6 +24,8 @@ source "drivers/ide/Kconfig"
 
 source "drivers/scsi/Kconfig"
 
+source "drivers/fc4/Kconfig"
+
 source "drivers/ata/Kconfig"
 
 source "drivers/md/Kconfig"
drivers/fc4/fc.c +1 -0
@@ -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>
drivers/fc4/Kconfig +1 -0
@@ -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
author
#8 lore raw

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

drivers/fc4/fc.c line 36

Quoted 10 lines · Randy Dunlap
drivers/fc4/fc.c +1 -0
@@ -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

#9 lore raw

PATCH 2/2 pluto fix disable/enable irq

In reply to Boaz Harrosh

From: Randy Dunlap <[email protected]>

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

Signed-off-by Randy Dunlap <[email protected]>
---
 drivers/scsi/pluto.c |    1 +
 1 file changed, 1 insertion(+)
drivers/scsi/pluto.c +1 -0
@@ -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
author
#10 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/fc4/fc.c       |   21 +++++++++++++++------
 drivers/fc4/fcp_impl.h |    2 ++
 drivers/scsi/pluto.c   |   14 +++++++-------
 3 files changed, 24 insertions(+), 13 deletions(-)
drivers/fc4/fc.c +15 -6
index 48c3b62..3962ba8 100644
@@ -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 {
drivers/fc4/fcp_impl.h +2 -0
index 41fa149..968a20e 100644
@@ -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 *);
drivers/scsi/pluto.c +7 -7
index e598a90..1cb8a39 100644
@@ -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;
 }
Signature · 1 line
1.5.3.1
author
#11 lore raw

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

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 <[email protected]>
---
 drivers/fc4/fc.c       |   21 +++++++++++++--------
 drivers/fc4/fcp_impl.h |    2 ++
 drivers/scsi/pluto.c   |   21 +++++++++++++--------
 3 files changed, 28 insertions(+), 16 deletions(-)
drivers/fc4/fc.c +13 -8
index 48c3b62..5a0eba5 100644
@@ -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);
 		}
drivers/fc4/fcp_impl.h +2 -0
index 41fa149..728f36d 100644
@@ -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 *);
drivers/scsi/pluto.c +13 -8
index e598a90..df231d3 100644
@@ -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;
 }
Signature · 1 line
1.5.3.1
#12 lore raw

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

Quoted 1 line · Boaz Harrosh

Some people, me included, might like this approach better

I think I prefer this approach too.

Signature · 4 lines
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."
author
#13 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/fc4/fc.c       |   23 +++++++++++++----------
 drivers/fc4/fcp_impl.h |    2 ++
 drivers/scsi/pluto.c   |   21 +++++++++++++--------
 3 files changed, 28 insertions(+), 18 deletions(-)
drivers/fc4/fc.c +13 -10
index 48c3b62..631e4ad 100644
@@ -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);
 		}
drivers/fc4/fcp_impl.h +2 -0
index 41fa149..728f36d 100644
@@ -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 *);
drivers/scsi/pluto.c +13 -8
index e598a90..df231d3 100644
@@ -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;
 }
Signature · 1 line
1.5.3.1