[PATCH 1/19] add data buffer accessors

[PATCH 1/19] add data buffer accessors

[FUJITA Tomonori]

Add a set of accessors for the scsi data buffer. This is in
preparation for chaining sg lists and bidirectional requests (and
possibly, the mid-layer dma mapping).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/scsi_lib.c  |   26 ++++++++++++++++++++++++++
 include/scsi/scsi_cmnd.h |   11 +++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1f5a07b..a2ebe61 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2290,3 +2290,29 @@ void scsi_kunmap_atomic_sg(void *virt)
 	kunmap_atomic(virt, KM_BIO_SRC_IRQ);
 }
 EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
+
+int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
+{
+	int nseg = 0;
+
+	if (cmd->use_sg) {
+		struct scatterlist *sg =
+			(struct scatterlist *) cmd->request_buffer;
+
+		nseg = dma_map_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
+		if (unlikely(!nseg))
+			return -ENOMEM;
+	}
+	return nseg;
+}
+EXPORT_SYMBOL(scsi_dma_map);
+
+void scsi_dma_unmap(struct device *dev, struct scsi_cmnd *cmd)
+{
+	if (cmd->use_sg) {
+		struct scatterlist *sg =
+			(struct scatterlist *) cmd->request_buffer;
+		dma_unmap_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
+	}
+}
+EXPORT_SYMBOL(scsi_dma_unmap);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a2e0c10..5f5a179 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -135,4 +135,15 @@ extern void scsi_kunmap_atomic_sg(void *
 extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(struct scatterlist *, int);
 
+extern int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd);
+extern void scsi_dma_unmap(struct device *dev, struct scsi_cmnd *cmd);
+
+#define scsi_sg_count(cmd) ((cmd)->use_sg)
+#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
+#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+#define scsi_resid(cmd) ((cmd)->resid)
+
+#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
+	for (__i = 0, sg = scsi_sglist(cmd); __i < (nseg); __i++, (sg)++)
+
 #endif /* _SCSI_SCSI_CMND_H */
-- 
1.4.3.2

Re: [PATCH 1/19] add data buffer accessors

[James Bottomley]

On Sat, 2007-05-12 at 19:05 +0900, FUJITA Tomonori wrote:
> Add a set of accessors for the scsi data buffer. This is in
> preparation for chaining sg lists and bidirectional requests (and
> possibly, the mid-layer dma mapping).
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/scsi_lib.c  |   26 ++++++++++++++++++++++++++
>  include/scsi/scsi_cmnd.h |   11 +++++++++++
>  2 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 1f5a07b..a2ebe61 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2290,3 +2290,29 @@ void scsi_kunmap_atomic_sg(void *virt)
>  	kunmap_atomic(virt, KM_BIO_SRC_IRQ);
>  }
>  EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
> +
> +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)

Actually, this is redundant.  We make sure the
shost->shost_gendev.parent is the device which should have been passed
in to scsi_add_host().

> +{
> +	int nseg = 0;
> +
> +	if (cmd->use_sg) {
> +		struct scatterlist *sg =
> +			(struct scatterlist *) cmd->request_buffer;
> +
> +		nseg = dma_map_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
> +		if (unlikely(!nseg))
> +			return -ENOMEM;
> +	}
> +	return nseg;
> +}
> +EXPORT_SYMBOL(scsi_dma_map);
> +
> +void scsi_dma_unmap(struct device *dev, struct scsi_cmnd *cmd)

Same here.

James

Re: [PATCH 1/19] add data buffer accessors

[Christoph Hellwig]

> +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
> +{
> +	int nseg = 0;
> +
> +	if (cmd->use_sg) {
> +		struct scatterlist *sg =
> +			(struct scatterlist *) cmd->request_buffer;
> +
> +		nseg = dma_map_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
> +		if (unlikely(!nseg))
> +			return -ENOMEM;
> +	}
> +	return nseg;
> +}
> +EXPORT_SYMBOL(scsi_dma_map);

As James already said the device argument should not be needed at all.
Also please add kerneldoc comment describing exported functions.

Re: [PATCH 1/19] add data buffer accessors

[FUJITA Tomonori]

From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 1/19] add data buffer accessors
Date: Sat, 12 May 2007 16:31:51 +0100

> > +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
> > +{
> > +	int nseg = 0;
> > +
> > +	if (cmd->use_sg) {
> > +		struct scatterlist *sg =
> > +			(struct scatterlist *) cmd->request_buffer;
> > +
> > +		nseg = dma_map_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
> > +		if (unlikely(!nseg))
> > +			return -ENOMEM;
> > +	}
> > +	return nseg;
> > +}
> > +EXPORT_SYMBOL(scsi_dma_map);
> 
> As James already said the device argument should not be needed at all.
> Also please add kerneldoc comment describing exported functions.

Thanks. How about the this one?

I've also updated the drivers though I don't send them since the
changes are just about scsi_dma_map/unmap():

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


James, do you want me to send them to the mailing list, send them
after unifying two patches for each driver, or wait for other guys'
comments for for some time?


>From 8fc2ab69fc110117b8a6a0af8fb6ba2a784b918e Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Sun, 13 May 2007 03:34:57 +0900
Subject: [PATCH] add data buffer accessors

This adds a set of accessors for the scsi data buffer. This is in
preparation for chaining sg lists and bidirectional requests (and
possibly, the mid-layer dma mapping).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/scsi_lib.c  |   38 ++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_cmnd.h |   11 +++++++++++
 2 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1f5a07b..70454b4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2290,3 +2290,41 @@ void scsi_kunmap_atomic_sg(void *virt)
 	kunmap_atomic(virt, KM_BIO_SRC_IRQ);
 }
 EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
+
+/**
+ * scsi_dma_map - perform DMA mapping against command's sg lists
+ * @cmd:	scsi command
+ *
+ * Returns the number of sg lists actually used, zero if the sg lists
+ * is NULL, or -ENOMEM if the mapping failed.
+ */
+int scsi_dma_map(struct scsi_cmnd *cmd)
+{
+	int nseg = 0;
+
+	if (scsi_sg_count(cmd)) {
+		struct device *dev = cmd->device->host->shost_gendev.parent;
+
+		nseg = dma_map_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
+				  cmd->sc_data_direction);
+		if (unlikely(!nseg))
+			return -ENOMEM;
+	}
+	return nseg;
+}
+EXPORT_SYMBOL(scsi_dma_map);
+
+/**
+ * scsi_dma_unmap - unmap command's sg lists mapped by scsi_dma_map
+ * @cmd:	scsi command
+ */
+void scsi_dma_unmap(struct scsi_cmnd *cmd)
+{
+	if (scsi_sg_count(cmd)) {
+		struct device *dev = cmd->device->host->shost_gendev.parent;
+
+		dma_unmap_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
+			     cmd->sc_data_direction);
+	}
+}
+EXPORT_SYMBOL(scsi_dma_unmap);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a2e0c10..996ff36 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -135,4 +135,15 @@ extern void scsi_kunmap_atomic_sg(void *
 extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(struct scatterlist *, int);
 
+extern int scsi_dma_map(struct scsi_cmnd *cmd);
+extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
+
+#define scsi_sg_count(cmd) ((cmd)->use_sg)
+#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
+#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+#define scsi_resid(cmd) ((cmd)->resid)
+
+#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
+	for (__i = 0, sg = scsi_sglist(cmd); __i < (nseg); __i++, (sg)++)
+
 #endif /* _SCSI_SCSI_CMND_H */
-- 
1.4.3.2

Re: [PATCH 1/19] add data buffer accessors

[Stefan Richter]

FUJITA Tomonori wrote:
> From: Christoph Hellwig <hch@infradead.org>
...
>> As James already said the device argument should not be needed at all.
...
> +int scsi_dma_map(struct scsi_cmnd *cmd)
> +{
> +	int nseg = 0;
> +
> +	if (scsi_sg_count(cmd)) {
> +		struct device *dev = cmd->device->host->shost_gendev.parent;
...

As a side note:

What is currently not supported by SCSI mid layer is that an sdev could
be moved from one initiator port to another.  If that happens, transport
layer drivers have to destroy the sdev and create a new one.  Or they
could represent /all/ initiator ports by a single shost, but then they
can't use the two new scsi_dma_ wrappers.  (Assuming there aren't any
other issues which would prevent the single-shost approach.)
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/

Re: [PATCH 1/19] add data buffer accessors

[Benny Halevy]

FUJITA Tomonori wrote:
> +#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
> +	for (__i = 0, sg = scsi_sglist(cmd); __i < (nseg); __i++, (sg)++)
> +

This feels like a layering violation, why not use for_each_sg()?

+#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
	for_each_sg(scsi_sglist(cmd), (sg), (nseg), (__i))	\

That said, I'm not sure that scsi_for_each_sg() is worth abstracting
since the caller can just as well do for_each_sg() directly
as sketched above...

Benny

Re: [PATCH 1/19] add data buffer accessors

[FUJITA Tomonori]

From: Benny Halevy <bhalevy@panasas.com>
Subject: Re: [PATCH 1/19] add data buffer accessors
Date: Mon, 14 May 2007 10:57:08 +0300

> FUJITA Tomonori wrote:
> > +#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
> > +	for (__i = 0, sg = scsi_sglist(cmd); __i < (nseg); __i++, (sg)++)
> > +
> 
> This feels like a layering violation, why not use for_each_sg()?
> 
> +#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
> 	for_each_sg(scsi_sglist(cmd), (sg), (nseg), (__i))	\

As I said before, when for_each_sg is ready, we'll convert
scsi_for_each_sg to use for_each_sg.


> That said, I'm not sure that scsi_for_each_sg() is worth abstracting
> since the caller can just as well do for_each_sg() directly
> as sketched above...

I'm not sure why you think it's a layering violation.

With scsi_for_each_sg(), many drivers don't need scsi_sglist().

Re: [PATCH 1/19] add data buffer accessors

[Benny Halevy]

FUJITA Tomonori wrote:
> From: Benny Halevy <bhalevy@panasas.com>
> Subject: Re: [PATCH 1/19] add data buffer accessors
> Date: Mon, 14 May 2007 10:57:08 +0300
> 
>> FUJITA Tomonori wrote:
>>> +#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
>>> +	for (__i = 0, sg = scsi_sglist(cmd); __i < (nseg); __i++, (sg)++)
>>> +
>> This feels like a layering violation, why not use for_each_sg()?
>>
>> +#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
>> 	for_each_sg(scsi_sglist(cmd), (sg), (nseg), (__i))	\
> 
> As I said before, when for_each_sg is ready, we'll convert
> scsi_for_each_sg to use for_each_sg.

thanks. works for me.

> 
> 
>> That said, I'm not sure that scsi_for_each_sg() is worth abstracting
>> since the caller can just as well do for_each_sg() directly
>> as sketched above...
> 
> I'm not sure why you think it's a layering violation.

I'd like to think of struct scatterlist as an abstract data type
with its own traversal method that hides its internals.
Not a layer per-se but more of an abstraction...

> 
> With scsi_for_each_sg(), many drivers don't need scsi_sglist().

Sure, just my two cents...

NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)

[Andrew Vasquez]

On Sat, 12 May 2007, James Bottomley wrote:

> On Sat, 2007-05-12 at 19:05 +0900, FUJITA Tomonori wrote:
> > Add a set of accessors for the scsi data buffer. This is in
> > preparation for chaining sg lists and bidirectional requests (and
> > possibly, the mid-layer dma mapping).
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  drivers/scsi/scsi_lib.c  |   26 ++++++++++++++++++++++++++
> >  include/scsi/scsi_cmnd.h |   11 +++++++++++
> >  2 files changed, 37 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 1f5a07b..a2ebe61 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -2290,3 +2290,29 @@ void scsi_kunmap_atomic_sg(void *virt)
> >  	kunmap_atomic(virt, KM_BIO_SRC_IRQ);
> >  }
> >  EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
> > +
> > +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
> 
> Actually, this is redundant.  We make sure the
> shost->shost_gendev.parent is the device which should have been passed
> in to scsi_add_host().

Well, there's perhaps an unintended side-effect with this assumption,
NPIV-based 'vports' (and their subsequent 'struct device' members) are
not fully initialized objects.

This is what we've run into while working on our NPIV (based) driver
with the 'data buffer' accessors works, while queueing the first
command to an sdev of a freshly created vport:

	[  366.860758] Unable to handle kernel NULL pointer dereference at 0000000000000000 RIP: 
	[  366.860762]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
	[  366.860778] PGD 0 
	[  366.860782] Oops: 0000 [1] SMP 
	[  366.860787] CPU 0 
	[  366.860790] Modules linked in: qla2xxx scsi_transport_fc
	[  366.860798] Pid: 5757, comm: scsi_wq_2 Not tainted 2.6.22-rc6 #4
	[  366.860802] RIP: 0010:[<ffffffff8020fdf6>]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
	[  366.860812] RSP: 0018:ffff810073979960  EFLAGS: 00010082
	[  366.860816] RAX: 0000000000000000 RBX: ffff810037cda070 RCX: 0000000000000024
	[  366.860821] RDX: 000000007cd86188 RSI: ffff81007d800ef8 RDI: ffffffff804c30e1
	[  366.860824] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
	[  366.860828] R10: 00000000ffffffff R11: 0000000000000006 R12: 0000000000000001
	[  366.860834] R13: ffff81007d800ef8 R14: ffff810075f9fda0 R15: ffff810076d66af8
	[  366.860839] FS:  0000000000000000(0000) GS:ffffffff80570000(0000) knlGS:0000000000000000
	[  366.860844] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
	[  366.860847] CR2: 0000000000000000 CR3: 000000007ec6b000 CR4: 00000000000006e0
	[  366.860853] Process scsi_wq_2 (pid: 5757, threadinfo ffff810073978000, task ffff810037c6ce00)
	[  366.860856] Stack:  0000000000011220 ffffffff8020fea6 0000000000000246 00000000000000e1
	[  366.860866]  00000000000000e1 ffff810076908608 ffff810076d66af8 ffffffff803a440c
	[  366.860876]  ffff810076908608 ffffffff8801cdd2 ffff810076908688 ffff810073979a18
	[  366.860885] Call Trace:
	[  366.860891]  [<ffffffff8020fea6>] nommu_map_sg+0x76/0x8f
	[  366.860900]  [<ffffffff803a440c>] scsi_dma_map+0x45/0x5c
	[  366.860922]  [<ffffffff8801cdd2>] :qla2xxx:qla24xx_start_scsi+0x118/0x4fb
	[  366.860928]  [<ffffffff8039fa74>] scsi_done+0x0/0x18
	[  366.860942]  [<ffffffff880116f0>] :qla2xxx:qla24xx_queuecommand+0x148/0x17a
	[  366.860948]  [<ffffffff803a01f7>] scsi_dispatch_cmd+0x1f6/0x313
	[  366.860953]  [<ffffffff803a622b>] scsi_request_fn+0x1d7/0x3b8

the failure point is this check (line 16) in check_addr():

	(gdb) l* check_addr+0x10
	0xffffffff8020fdf6 is in check_addr (arch/x86_64/kernel/pci-nommu.c:16).
	11      #include <asm/dma.h>
	12
	13      static int
	14      check_addr(char *name, struct device *hwdev, dma_addr_t bus, size_t size)
	15      {
	16              if (hwdev && bus + size > *hwdev->dma_mask) {
	17                      if (*hwdev->dma_mask >= DMA_32BIT_MASK)
	18                              printk(KERN_ERR
	19                                  "nommu_%s: overflow %Lx+%zu of device mask %Lx\n",
	20                                      name, (long long)bus, size,

hwdev->dma_mask (an 'u64 *') is NULL, as a corresponding 'set'
function is never getting called via pci_set_dma_mask() and
pci_set_consistent_dma_mask() on a vport.

I don't believe we'd want to:

1) have each LLD cache the physical-port's previously determined
   dma-mask, and call pci_set_[consistent_]dma_mask() for each
   instantiated vport.

   the problem with this approach is 'knowing' how much data needs to
   be propagated to a vport's underlying 'struct device'.

2) pass the physical-port's 'scsi_host' structure during a vport's
   scsi_add_host() call:

	if (scsi_add_host(vha->host, &fc_vport->dev)) {

   as that would lead to some device-model ugliness...


Abstractions such as these 'data-accessor functions' which don't allow
a LLD the opprotunity to be more selective on the
'physical-characteristics' of a 'virtual' device are going to lead to
more subtle bugs going forward.

One possiblity (the least intrusive) would be to add a
scsi_dma_map_with_device() function which takes the proper (LLD
defined) 'struct device' as a parameter, as was originally proposed,
and have NPIV-aware drivers call that function during the mappings of
physical *and* virtual dma-mappings.

RE: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)

[Seokmann Ju]

[-- Attachment #1: Type: text/plain, Size: 4459 bytes --]

On Fri, 29 June 2007, Andrew Vasquez wrote:
> On Sat, 12 May 2007, James Bottomley wrote:
> 
> > On Sat, 2007-05-12 at 19:05 +0900, FUJITA Tomonori wrote:
> > > Add a set of accessors for the scsi data buffer. This is in 
> > > preparation for chaining sg lists and bidirectional requests (and 
> > > possibly, the mid-layer dma mapping).
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > ---
> > >  drivers/scsi/scsi_lib.c  |   26 ++++++++++++++++++++++++++
> > >  include/scsi/scsi_cmnd.h |   11 +++++++++++
> > >  2 files changed, 37 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c 
> b/drivers/scsi/scsi_lib.c index 
> > > 1f5a07b..a2ebe61 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -2290,3 +2290,29 @@ void scsi_kunmap_atomic_sg(void *virt)
> > >  	kunmap_atomic(virt, KM_BIO_SRC_IRQ);  }  
> > > EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
> > > +
> > > +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
> > 
> > Actually, this is redundant.  We make sure the
> > shost->shost_gendev.parent is the device which should have 
> been passed
> > in to scsi_add_host().
> 
> Well, there's perhaps an unintended side-effect with this 
> assumption, NPIV-based 'vports' (and their subsequent 'struct 
> device' members) are not fully initialized objects.
> 
> This is what we've run into while working on our NPIV (based) 
> driver with the 'data buffer' accessors works, while queueing 
> the first command to an sdev of a freshly created vport:
> ... 
> One possiblity (the least intrusive) would be to add a
> scsi_dma_map_with_device() function which takes the proper (LLD
> defined) 'struct device' as a parameter, as was originally 
> proposed, and have NPIV-aware drivers call that function 
> during the mappings of physical *and* virtual dma-mappings.
As a one possible solution, scsi_dma_map_with_device() has added.
With this change, the QLogic NPIV enabled driver has tested and it works
fine.
With the patch attached, would like to stroke this thread so that it can
move forward.

Thank you,
Seokmann
---
>From cddc43e416deeb875982f371d27c4fb074d5970e Mon Sep 17 00:00:00 2001
From: Seokmann Ju <seokmann.ju@qlogic.com>
Date: Tue, 3 Jul 2007 11:53:34 -0700
Subject: [PATCH] add data buffer accessor with device structure.

- in addition to scsi_dma_map(), scsi_dma_map_with_device() has added.
- this addition is for not fully instantiated host, like virtual hosts
on
  NPIV based implementation.

Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
---
 drivers/scsi/scsi_lib.c  |   22 ++++++++++++++++++++++
 include/scsi/scsi_cmnd.h |    1 +
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 70454b4..90601ed 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2315,6 +2315,28 @@ int scsi_dma_map(struct scsi_cmnd *cmd)
 EXPORT_SYMBOL(scsi_dma_map);

 /**
+ * scsi_dma_map_with_device - perform DMA mapping against command's sg
lists
+ * @dev:       device structure
+ * @cmd:       scsi command
+ *
+ * Returns the number of sg lists actually used, zero if the sg lists
+ * is NULL, or -ENOMEM if the mapping failed.
+ */
+int scsi_dma_map_with_device(struct device *dev, struct scsi_cmnd *cmd)
+{
+       int nseg = 0;
+
+       if (scsi_sg_count(cmd)) {
+               nseg = dma_map_sg(dev, scsi_sglist(cmd),
scsi_sg_count(cmd),
+                                 cmd->sc_data_direction);
+               if (unlikely(!nseg))
+                       return -ENOMEM;
+       }
+       return nseg;
+}
+EXPORT_SYMBOL(scsi_dma_map_with_device);
+
+/**
  * scsi_dma_unmap - unmap command's sg lists mapped by scsi_dma_map
  * @cmd:       scsi command
  */
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 53e1705..a223331 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -136,6 +136,7 @@ extern struct scatterlist *scsi_alloc_sgtable(struct
scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(struct scatterlist *, int);

 extern int scsi_dma_map(struct scsi_cmnd *cmd);
+extern int scsi_dma_map_with_device(struct device *dev, struct
scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);

 #define scsi_sg_count(cmd) ((cmd)->use_sg)
--
1.5.2.2.603.g7c851
---

[-- Attachment #2: 0001-add-data-buffer-accessor-with-device-structure.patch --]
[-- Type: application/octet-stream, Size: 2093 bytes --]

From cddc43e416deeb875982f371d27c4fb074d5970e Mon Sep 17 00:00:00 2001
From: Seokmann Ju <seokmann.ju@qlogic.com>
Date: Tue, 3 Jul 2007 11:53:34 -0700
Subject: [PATCH] add data buffer accessor with device structure.

- in addition to scsi_dma_map(), scsi_dma_map_with_device() has added.
- this addition is for not fully instantiated host, like virtual hosts on
  NPIV based implementation.

Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
---
 drivers/scsi/scsi_lib.c  |   22 ++++++++++++++++++++++
 include/scsi/scsi_cmnd.h |    1 +
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 70454b4..90601ed 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2315,6 +2315,28 @@ int scsi_dma_map(struct scsi_cmnd *cmd)
 EXPORT_SYMBOL(scsi_dma_map);
 
 /**
+ * scsi_dma_map_with_device - perform DMA mapping against command's sg lists
+ * @dev:	device structure
+ * @cmd:	scsi command
+ *
+ * Returns the number of sg lists actually used, zero if the sg lists
+ * is NULL, or -ENOMEM if the mapping failed.
+ */
+int scsi_dma_map_with_device(struct device *dev, struct scsi_cmnd *cmd)
+{
+	int nseg = 0;
+
+	if (scsi_sg_count(cmd)) {
+		nseg = dma_map_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
+				  cmd->sc_data_direction);
+		if (unlikely(!nseg))
+			return -ENOMEM;
+	}
+	return nseg;
+}
+EXPORT_SYMBOL(scsi_dma_map_with_device);
+
+/**
  * scsi_dma_unmap - unmap command's sg lists mapped by scsi_dma_map
  * @cmd:	scsi command
  */
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 53e1705..a223331 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -136,6 +136,7 @@ extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(struct scatterlist *, int);
 
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
+extern int scsi_dma_map_with_device(struct device *dev, struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
 #define scsi_sg_count(cmd) ((cmd)->use_sg)
-- 
1.5.2.2.603.g7c851

Re: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)

[FUJITA Tomonori]

Sorry for the delay...

From: Andrew Vasquez <andrew.vasquez@qlogic.com>
Subject: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)
Date: Fri, 29 Jun 2007 06:23:32 -0700

> On Sat, 12 May 2007, James Bottomley wrote:
> 
> > On Sat, 2007-05-12 at 19:05 +0900, FUJITA Tomonori wrote:
> > > Add a set of accessors for the scsi data buffer. This is in
> > > preparation for chaining sg lists and bidirectional requests (and
> > > possibly, the mid-layer dma mapping).
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > ---
> > >  drivers/scsi/scsi_lib.c  |   26 ++++++++++++++++++++++++++
> > >  include/scsi/scsi_cmnd.h |   11 +++++++++++
> > >  2 files changed, 37 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index 1f5a07b..a2ebe61 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -2290,3 +2290,29 @@ void scsi_kunmap_atomic_sg(void *virt)
> > >  	kunmap_atomic(virt, KM_BIO_SRC_IRQ);
> > >  }
> > >  EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
> > > +
> > > +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
> > 
> > Actually, this is redundant.  We make sure the
> > shost->shost_gendev.parent is the device which should have been passed
> > in to scsi_add_host().
> 
> Well, there's perhaps an unintended side-effect with this assumption,
> NPIV-based 'vports' (and their subsequent 'struct device' members) are
> not fully initialized objects.
> 
> This is what we've run into while working on our NPIV (based) driver
> with the 'data buffer' accessors works, while queueing the first
> command to an sdev of a freshly created vport:
> 
> 	[  366.860758] Unable to handle kernel NULL pointer dereference at 0000000000000000 RIP: 
> 	[  366.860762]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
> 	[  366.860778] PGD 0 
> 	[  366.860782] Oops: 0000 [1] SMP 
> 	[  366.860787] CPU 0 
> 	[  366.860790] Modules linked in: qla2xxx scsi_transport_fc
> 	[  366.860798] Pid: 5757, comm: scsi_wq_2 Not tainted 2.6.22-rc6 #4
> 	[  366.860802] RIP: 0010:[<ffffffff8020fdf6>]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
> 	[  366.860812] RSP: 0018:ffff810073979960  EFLAGS: 00010082
> 	[  366.860816] RAX: 0000000000000000 RBX: ffff810037cda070 RCX: 0000000000000024
> 	[  366.860821] RDX: 000000007cd86188 RSI: ffff81007d800ef8 RDI: ffffffff804c30e1
> 	[  366.860824] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> 	[  366.860828] R10: 00000000ffffffff R11: 0000000000000006 R12: 0000000000000001
> 	[  366.860834] R13: ffff81007d800ef8 R14: ffff810075f9fda0 R15: ffff810076d66af8
> 	[  366.860839] FS:  0000000000000000(0000) GS:ffffffff80570000(0000) knlGS:0000000000000000
> 	[  366.860844] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> 	[  366.860847] CR2: 0000000000000000 CR3: 000000007ec6b000 CR4: 00000000000006e0
> 	[  366.860853] Process scsi_wq_2 (pid: 5757, threadinfo ffff810073978000, task ffff810037c6ce00)
> 	[  366.860856] Stack:  0000000000011220 ffffffff8020fea6 0000000000000246 00000000000000e1
> 	[  366.860866]  00000000000000e1 ffff810076908608 ffff810076d66af8 ffffffff803a440c
> 	[  366.860876]  ffff810076908608 ffffffff8801cdd2 ffff810076908688 ffff810073979a18
> 	[  366.860885] Call Trace:
> 	[  366.860891]  [<ffffffff8020fea6>] nommu_map_sg+0x76/0x8f
> 	[  366.860900]  [<ffffffff803a440c>] scsi_dma_map+0x45/0x5c
> 	[  366.860922]  [<ffffffff8801cdd2>] :qla2xxx:qla24xx_start_scsi+0x118/0x4fb
> 	[  366.860928]  [<ffffffff8039fa74>] scsi_done+0x0/0x18
> 	[  366.860942]  [<ffffffff880116f0>] :qla2xxx:qla24xx_queuecommand+0x148/0x17a
> 	[  366.860948]  [<ffffffff803a01f7>] scsi_dispatch_cmd+0x1f6/0x313
> 	[  366.860953]  [<ffffffff803a622b>] scsi_request_fn+0x1d7/0x3b8
> 
> the failure point is this check (line 16) in check_addr():
> 
> 	(gdb) l* check_addr+0x10
> 	0xffffffff8020fdf6 is in check_addr (arch/x86_64/kernel/pci-nommu.c:16).
> 	11      #include <asm/dma.h>
> 	12
> 	13      static int
> 	14      check_addr(char *name, struct device *hwdev, dma_addr_t bus, size_t size)
> 	15      {
> 	16              if (hwdev && bus + size > *hwdev->dma_mask) {
> 	17                      if (*hwdev->dma_mask >= DMA_32BIT_MASK)
> 	18                              printk(KERN_ERR
> 	19                                  "nommu_%s: overflow %Lx+%zu of device mask %Lx\n",
> 	20                                      name, (long long)bus, size,
> 
> hwdev->dma_mask (an 'u64 *') is NULL, as a corresponding 'set'
> function is never getting called via pci_set_dma_mask() and
> pci_set_consistent_dma_mask() on a vport.

Yeah, I use lpfc's NPIV but haven't hit this because I use swiotlb...


> I don't believe we'd want to:
> 
> 1) have each LLD cache the physical-port's previously determined
>    dma-mask, and call pci_set_[consistent_]dma_mask() for each
>    instantiated vport.
> 
>    the problem with this approach is 'knowing' how much data needs to
>    be propagated to a vport's underlying 'struct device'.
>
> 2) pass the physical-port's 'scsi_host' structure during a vport's
>    scsi_add_host() call:
> 
> 	if (scsi_add_host(vha->host, &fc_vport->dev)) {
> 
>    as that would lead to some device-model ugliness...

Yeah, we shouldn't do this...


> Abstractions such as these 'data-accessor functions' which don't allow
> a LLD the opprotunity to be more selective on the
> 'physical-characteristics' of a 'virtual' device are going to lead to
> more subtle bugs going forward.
> 
> One possiblity (the least intrusive) would be to add a
> scsi_dma_map_with_device() function which takes the proper (LLD
> defined) 'struct device' as a parameter, as was originally proposed,
> and have NPIV-aware drivers call that function during the mappings of
> physical *and* virtual dma-mappings.

It's ok. But if only NPIV-aware drivers need to handle this, would it
be better to just use dma_map_sg instead of scsi_dma_map?

Re: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)

[FUJITA Tomonori]

I forgot to CC James Smart and send a lpfc patch.

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: Re: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)
Date: Wed, 04 Jul 2007 17:25:36 +0900

> Sorry for the delay...
> 
> From: Andrew Vasquez <andrew.vasquez@qlogic.com>
> Subject: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)
> Date: Fri, 29 Jun 2007 06:23:32 -0700
> 
> > On Sat, 12 May 2007, James Bottomley wrote:
> > 
> > > On Sat, 2007-05-12 at 19:05 +0900, FUJITA Tomonori wrote:
> > > > Add a set of accessors for the scsi data buffer. This is in
> > > > preparation for chaining sg lists and bidirectional requests (and
> > > > possibly, the mid-layer dma mapping).
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > ---
> > > >  drivers/scsi/scsi_lib.c  |   26 ++++++++++++++++++++++++++
> > > >  include/scsi/scsi_cmnd.h |   11 +++++++++++
> > > >  2 files changed, 37 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > > index 1f5a07b..a2ebe61 100644
> > > > --- a/drivers/scsi/scsi_lib.c
> > > > +++ b/drivers/scsi/scsi_lib.c
> > > > @@ -2290,3 +2290,29 @@ void scsi_kunmap_atomic_sg(void *virt)
> > > >  	kunmap_atomic(virt, KM_BIO_SRC_IRQ);
> > > >  }
> > > >  EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
> > > > +
> > > > +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
> > > 
> > > Actually, this is redundant.  We make sure the
> > > shost->shost_gendev.parent is the device which should have been passed
> > > in to scsi_add_host().
> > 
> > Well, there's perhaps an unintended side-effect with this assumption,
> > NPIV-based 'vports' (and their subsequent 'struct device' members) are
> > not fully initialized objects.
> > 
> > This is what we've run into while working on our NPIV (based) driver
> > with the 'data buffer' accessors works, while queueing the first
> > command to an sdev of a freshly created vport:
> > 
> > 	[  366.860758] Unable to handle kernel NULL pointer dereference at 0000000000000000 RIP: 
> > 	[  366.860762]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
> > 	[  366.860778] PGD 0 
> > 	[  366.860782] Oops: 0000 [1] SMP 
> > 	[  366.860787] CPU 0 
> > 	[  366.860790] Modules linked in: qla2xxx scsi_transport_fc
> > 	[  366.860798] Pid: 5757, comm: scsi_wq_2 Not tainted 2.6.22-rc6 #4
> > 	[  366.860802] RIP: 0010:[<ffffffff8020fdf6>]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
> > 	[  366.860812] RSP: 0018:ffff810073979960  EFLAGS: 00010082
> > 	[  366.860816] RAX: 0000000000000000 RBX: ffff810037cda070 RCX: 0000000000000024
> > 	[  366.860821] RDX: 000000007cd86188 RSI: ffff81007d800ef8 RDI: ffffffff804c30e1
> > 	[  366.860824] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> > 	[  366.860828] R10: 00000000ffffffff R11: 0000000000000006 R12: 0000000000000001
> > 	[  366.860834] R13: ffff81007d800ef8 R14: ffff810075f9fda0 R15: ffff810076d66af8
> > 	[  366.860839] FS:  0000000000000000(0000) GS:ffffffff80570000(0000) knlGS:0000000000000000
> > 	[  366.860844] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> > 	[  366.860847] CR2: 0000000000000000 CR3: 000000007ec6b000 CR4: 00000000000006e0
> > 	[  366.860853] Process scsi_wq_2 (pid: 5757, threadinfo ffff810073978000, task ffff810037c6ce00)
> > 	[  366.860856] Stack:  0000000000011220 ffffffff8020fea6 0000000000000246 00000000000000e1
> > 	[  366.860866]  00000000000000e1 ffff810076908608 ffff810076d66af8 ffffffff803a440c
> > 	[  366.860876]  ffff810076908608 ffffffff8801cdd2 ffff810076908688 ffff810073979a18
> > 	[  366.860885] Call Trace:
> > 	[  366.860891]  [<ffffffff8020fea6>] nommu_map_sg+0x76/0x8f
> > 	[  366.860900]  [<ffffffff803a440c>] scsi_dma_map+0x45/0x5c
> > 	[  366.860922]  [<ffffffff8801cdd2>] :qla2xxx:qla24xx_start_scsi+0x118/0x4fb
> > 	[  366.860928]  [<ffffffff8039fa74>] scsi_done+0x0/0x18
> > 	[  366.860942]  [<ffffffff880116f0>] :qla2xxx:qla24xx_queuecommand+0x148/0x17a
> > 	[  366.860948]  [<ffffffff803a01f7>] scsi_dispatch_cmd+0x1f6/0x313
> > 	[  366.860953]  [<ffffffff803a622b>] scsi_request_fn+0x1d7/0x3b8
> > 
> > the failure point is this check (line 16) in check_addr():
> > 
> > 	(gdb) l* check_addr+0x10
> > 	0xffffffff8020fdf6 is in check_addr (arch/x86_64/kernel/pci-nommu.c:16).
> > 	11      #include <asm/dma.h>
> > 	12
> > 	13      static int
> > 	14      check_addr(char *name, struct device *hwdev, dma_addr_t bus, size_t size)
> > 	15      {
> > 	16              if (hwdev && bus + size > *hwdev->dma_mask) {
> > 	17                      if (*hwdev->dma_mask >= DMA_32BIT_MASK)
> > 	18                              printk(KERN_ERR
> > 	19                                  "nommu_%s: overflow %Lx+%zu of device mask %Lx\n",
> > 	20                                      name, (long long)bus, size,
> > 
> > hwdev->dma_mask (an 'u64 *') is NULL, as a corresponding 'set'
> > function is never getting called via pci_set_dma_mask() and
> > pci_set_consistent_dma_mask() on a vport.
> 
> Yeah, I use lpfc's NPIV but haven't hit this because I use swiotlb...
> 
> 
> > I don't believe we'd want to:
> > 
> > 1) have each LLD cache the physical-port's previously determined
> >    dma-mask, and call pci_set_[consistent_]dma_mask() for each
> >    instantiated vport.
> > 
> >    the problem with this approach is 'knowing' how much data needs to
> >    be propagated to a vport's underlying 'struct device'.
> >
> > 2) pass the physical-port's 'scsi_host' structure during a vport's
> >    scsi_add_host() call:
> > 
> > 	if (scsi_add_host(vha->host, &fc_vport->dev)) {
> > 
> >    as that would lead to some device-model ugliness...
> 
> Yeah, we shouldn't do this...
> 
> 
> > Abstractions such as these 'data-accessor functions' which don't allow
> > a LLD the opprotunity to be more selective on the
> > 'physical-characteristics' of a 'virtual' device are going to lead to
> > more subtle bugs going forward.
> > 
> > One possiblity (the least intrusive) would be to add a
> > scsi_dma_map_with_device() function which takes the proper (LLD
> > defined) 'struct device' as a parameter, as was originally proposed,
> > and have NPIV-aware drivers call that function during the mappings of
> > physical *and* virtual dma-mappings.
> 
> It's ok. But if only NPIV-aware drivers need to handle this, would it
> be better to just use dma_map_sg instead of scsi_dma_map?

---
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

This patch uses dma_map_sg with phba->pcidev->dev instead of
scsi_dma_map.

scsi_dma_map doesn't work for NPIV since fc_vport->dev isn't fully
initialized. check_addr() in arch/x86_64/kernel/pci-nommu.c leads to
the crash since dev->dma_mask is NULL.

For more details:

http://marc.info/?l=linux-scsi&m=118312448030633&w=2

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/lpfc/lpfc_scsi.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index 5d2e3de..94c5f0c 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -332,8 +332,7 @@ lpfc_scsi_prep_dma_buf(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
 	 * data bde entry.
 	 */
 	bpl += 2;
-	nseg = scsi_dma_map(scsi_cmnd);
-	if (nseg > 0) {
+	if (scsi_sg_count(scsi_cmnd)) {
 		/*
 		 * The driver stores the segment count returned from pci_map_sg
 		 * because this a count of dma-mappings used to map the use_sg
@@ -341,6 +340,11 @@ lpfc_scsi_prep_dma_buf(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
 		 * architectures that implement an IOMMU.
 		 */
 
+		nseg = dma_map_sg(&phba->pcidev->dev, scsi_sglist(scsi_cmnd),
+				  scsi_sg_count(scsi_cmnd), datadir);
+		if (unlikely(!nseg))
+			return 1;
+
 		lpfc_cmd->seg_cnt = nseg;
 		if (lpfc_cmd->seg_cnt > phba->cfg_sg_seg_cnt) {
 			printk(KERN_ERR "%s: Too many sg segments from "
@@ -370,8 +374,7 @@ lpfc_scsi_prep_dma_buf(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
 			bpl++;
 			num_bde++;
 		}
-	} else if (nseg < 0)
-		return 1;
+	}
 
 	/*
 	 * Finish initializing those IOCB fields that are dependent on the
-- 
1.4.4.4

RE: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)

[Seokmann Ju]

On Wed, 2007-07-04 at 1:26 +0900, FUJITA Tomonori wrote:
> On Fri, 29 Jun 2007 06:23:32 -0700 Andrew Vasquez wrote:
> > One possiblity (the least intrusive) would be to add a
> > scsi_dma_map_with_device() function which takes the proper (LLD
> > defined) 'struct device' as a parameter, as was originally 
> proposed, 
> > and have NPIV-aware drivers call that function during the 
> mappings of 
> > physical *and* virtual dma-mappings.
> 
> It's ok. But if only NPIV-aware drivers need to handle this, 
> would it be better to just use dma_map_sg instead of scsi_dma_map?
I agree with you on the suggestion.
A patch for NPIV support on qla2xxx module that includes this change
will be followed.

Seokmann

Re: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)

[James Smart]

ACK - looks fine..

Thanks

-- james s

FUJITA Tomonori wrote:
> I forgot to CC James Smart and send a lpfc patch.
> 
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: Re: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)
> Date: Wed, 04 Jul 2007 17:25:36 +0900
> 
>> Sorry for the delay...
>>
>> From: Andrew Vasquez <andrew.vasquez@qlogic.com>
>> Subject: NPIV mapping problems (was Re: [PATCH 1/19] add data buffer accessors)
>> Date: Fri, 29 Jun 2007 06:23:32 -0700
>>
>>> On Sat, 12 May 2007, James Bottomley wrote:
>>>
>>>> On Sat, 2007-05-12 at 19:05 +0900, FUJITA Tomonori wrote:
>>>>> Add a set of accessors for the scsi data buffer. This is in
>>>>> preparation for chaining sg lists and bidirectional requests (and
>>>>> possibly, the mid-layer dma mapping).
>>>>>
>>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>>>> ---
>>>>>  drivers/scsi/scsi_lib.c  |   26 ++++++++++++++++++++++++++
>>>>>  include/scsi/scsi_cmnd.h |   11 +++++++++++
>>>>>  2 files changed, 37 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>>> index 1f5a07b..a2ebe61 100644
>>>>> --- a/drivers/scsi/scsi_lib.c
>>>>> +++ b/drivers/scsi/scsi_lib.c
>>>>> @@ -2290,3 +2290,29 @@ void scsi_kunmap_atomic_sg(void *virt)
>>>>>  	kunmap_atomic(virt, KM_BIO_SRC_IRQ);
>>>>>  }
>>>>>  EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
>>>>> +
>>>>> +int scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
>>>> Actually, this is redundant.  We make sure the
>>>> shost->shost_gendev.parent is the device which should have been passed
>>>> in to scsi_add_host().
>>> Well, there's perhaps an unintended side-effect with this assumption,
>>> NPIV-based 'vports' (and their subsequent 'struct device' members) are
>>> not fully initialized objects.
>>>
>>> This is what we've run into while working on our NPIV (based) driver
>>> with the 'data buffer' accessors works, while queueing the first
>>> command to an sdev of a freshly created vport:
>>>
>>> 	[  366.860758] Unable to handle kernel NULL pointer dereference at 0000000000000000 RIP: 
>>> 	[  366.860762]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
>>> 	[  366.860778] PGD 0 
>>> 	[  366.860782] Oops: 0000 [1] SMP 
>>> 	[  366.860787] CPU 0 
>>> 	[  366.860790] Modules linked in: qla2xxx scsi_transport_fc
>>> 	[  366.860798] Pid: 5757, comm: scsi_wq_2 Not tainted 2.6.22-rc6 #4
>>> 	[  366.860802] RIP: 0010:[<ffffffff8020fdf6>]  [<ffffffff8020fdf6>] check_addr+0x10/0x4a
>>> 	[  366.860812] RSP: 0018:ffff810073979960  EFLAGS: 00010082
>>> 	[  366.860816] RAX: 0000000000000000 RBX: ffff810037cda070 RCX: 0000000000000024
>>> 	[  366.860821] RDX: 000000007cd86188 RSI: ffff81007d800ef8 RDI: ffffffff804c30e1
>>> 	[  366.860824] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
>>> 	[  366.860828] R10: 00000000ffffffff R11: 0000000000000006 R12: 0000000000000001
>>> 	[  366.860834] R13: ffff81007d800ef8 R14: ffff810075f9fda0 R15: ffff810076d66af8
>>> 	[  366.860839] FS:  0000000000000000(0000) GS:ffffffff80570000(0000) knlGS:0000000000000000
>>> 	[  366.860844] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
>>> 	[  366.860847] CR2: 0000000000000000 CR3: 000000007ec6b000 CR4: 00000000000006e0
>>> 	[  366.860853] Process scsi_wq_2 (pid: 5757, threadinfo ffff810073978000, task ffff810037c6ce00)
>>> 	[  366.860856] Stack:  0000000000011220 ffffffff8020fea6 0000000000000246 00000000000000e1
>>> 	[  366.860866]  00000000000000e1 ffff810076908608 ffff810076d66af8 ffffffff803a440c
>>> 	[  366.860876]  ffff810076908608 ffffffff8801cdd2 ffff810076908688 ffff810073979a18
>>> 	[  366.860885] Call Trace:
>>> 	[  366.860891]  [<ffffffff8020fea6>] nommu_map_sg+0x76/0x8f
>>> 	[  366.860900]  [<ffffffff803a440c>] scsi_dma_map+0x45/0x5c
>>> 	[  366.860922]  [<ffffffff8801cdd2>] :qla2xxx:qla24xx_start_scsi+0x118/0x4fb
>>> 	[  366.860928]  [<ffffffff8039fa74>] scsi_done+0x0/0x18
>>> 	[  366.860942]  [<ffffffff880116f0>] :qla2xxx:qla24xx_queuecommand+0x148/0x17a
>>> 	[  366.860948]  [<ffffffff803a01f7>] scsi_dispatch_cmd+0x1f6/0x313
>>> 	[  366.860953]  [<ffffffff803a622b>] scsi_request_fn+0x1d7/0x3b8
>>>
>>> the failure point is this check (line 16) in check_addr():
>>>
>>> 	(gdb) l* check_addr+0x10
>>> 	0xffffffff8020fdf6 is in check_addr (arch/x86_64/kernel/pci-nommu.c:16).
>>> 	11      #include <asm/dma.h>
>>> 	12
>>> 	13      static int
>>> 	14      check_addr(char *name, struct device *hwdev, dma_addr_t bus, size_t size)
>>> 	15      {
>>> 	16              if (hwdev && bus + size > *hwdev->dma_mask) {
>>> 	17                      if (*hwdev->dma_mask >= DMA_32BIT_MASK)
>>> 	18                              printk(KERN_ERR
>>> 	19                                  "nommu_%s: overflow %Lx+%zu of device mask %Lx\n",
>>> 	20                                      name, (long long)bus, size,
>>>
>>> hwdev->dma_mask (an 'u64 *') is NULL, as a corresponding 'set'
>>> function is never getting called via pci_set_dma_mask() and
>>> pci_set_consistent_dma_mask() on a vport.
>> Yeah, I use lpfc's NPIV but haven't hit this because I use swiotlb...
>>
>>
>>> I don't believe we'd want to:
>>>
>>> 1) have each LLD cache the physical-port's previously determined
>>>    dma-mask, and call pci_set_[consistent_]dma_mask() for each
>>>    instantiated vport.
>>>
>>>    the problem with this approach is 'knowing' how much data needs to
>>>    be propagated to a vport's underlying 'struct device'.
>>>
>>> 2) pass the physical-port's 'scsi_host' structure during a vport's
>>>    scsi_add_host() call:
>>>
>>> 	if (scsi_add_host(vha->host, &fc_vport->dev)) {
>>>
>>>    as that would lead to some device-model ugliness...
>> Yeah, we shouldn't do this...
>>
>>
>>> Abstractions such as these 'data-accessor functions' which don't allow
>>> a LLD the opprotunity to be more selective on the
>>> 'physical-characteristics' of a 'virtual' device are going to lead to
>>> more subtle bugs going forward.
>>>
>>> One possiblity (the least intrusive) would be to add a
>>> scsi_dma_map_with_device() function which takes the proper (LLD
>>> defined) 'struct device' as a parameter, as was originally proposed,
>>> and have NPIV-aware drivers call that function during the mappings of
>>> physical *and* virtual dma-mappings.
>> It's ok. But if only NPIV-aware drivers need to handle this, would it
>> be better to just use dma_map_sg instead of scsi_dma_map?
> 
> ---
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> This patch uses dma_map_sg with phba->pcidev->dev instead of
> scsi_dma_map.
> 
> scsi_dma_map doesn't work for NPIV since fc_vport->dev isn't fully
> initialized. check_addr() in arch/x86_64/kernel/pci-nommu.c leads to
> the crash since dev->dma_mask is NULL.
> 
> For more details:
> 
> http://marc.info/?l=linux-scsi&m=118312448030633&w=2
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/scsi/lpfc/lpfc_scsi.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index 5d2e3de..94c5f0c 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -332,8 +332,7 @@ lpfc_scsi_prep_dma_buf(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
>  	 * data bde entry.
>  	 */
>  	bpl += 2;
> -	nseg = scsi_dma_map(scsi_cmnd);
> -	if (nseg > 0) {
> +	if (scsi_sg_count(scsi_cmnd)) {
>  		/*
>  		 * The driver stores the segment count returned from pci_map_sg
>  		 * because this a count of dma-mappings used to map the use_sg
> @@ -341,6 +340,11 @@ lpfc_scsi_prep_dma_buf(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
>  		 * architectures that implement an IOMMU.
>  		 */
>  
> +		nseg = dma_map_sg(&phba->pcidev->dev, scsi_sglist(scsi_cmnd),
> +				  scsi_sg_count(scsi_cmnd), datadir);
> +		if (unlikely(!nseg))
> +			return 1;
> +
>  		lpfc_cmd->seg_cnt = nseg;
>  		if (lpfc_cmd->seg_cnt > phba->cfg_sg_seg_cnt) {
>  			printk(KERN_ERR "%s: Too many sg segments from "
> @@ -370,8 +374,7 @@ lpfc_scsi_prep_dma_buf(struct lpfc_hba *phba, struct lpfc_scsi_buf *lpfc_cmd)
>  			bpl++;
>  			num_bde++;
>  		}
> -	} else if (nseg < 0)
> -		return 1;
> +	}
>  
>  	/*
>  	 * Finish initializing those IOCB fields that are dependent on the