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@xxxxxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
---
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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
