I had reminded James B of a patch for an issue with the scsi subsystem
not properly finding the parent device for dma operations when we had
nested scsi_hosts..
http://marc.info/?l=linux-scsi&m=125372757108048&w=2
The patch was picked up, and integrated into linux-next, which started
to see issues, and which James B cut an additional patch to deal with
traversing too much of the tree (stopping when dev->parent == NULL)..
The http://marc.info/?l=linux-scsi&m=125414973520985&w=2
The real issue was the traversal of a node that had "dev->type == NULL".
Turns out standard PCI endpoint and bridge/domain objects have NULL types
too - so every traversal went all the way to the root of the tree.
The attached patch, cut against scsi-misc-2.6 - replaces both of the
patches above. Rather than making any assumptions about dev->type values,
it now explicitly matches dev->type structures to known scsi or transport
objects, and only traverses them if they are recognized. To get around
symbol dependencies, and to allow transports as modules to come and go,
a registration interface was put in place to register transport objects
with the routine that does the matching. The FC transport was converted
to use a device_type structure for its objects (note: perhaps other
transports should consider the same ?)
Please apply this patch as soon as possible. At the current time,
NPIV vport dma operation is severely broken.
Thanks
-- james s
Signed-off-by: James Smart <[email protected]>
---
drivers/scsi/hosts.c | 72 +++++++++++++++++++++++++++++++++++++++
drivers/scsi/scsi_lib.c | 2 -
drivers/scsi/scsi_lib_dma.c | 7 ++-
drivers/scsi/scsi_priv.h | 2 +
drivers/scsi/scsi_transport_fc.c | 48 +++++++++++++++++++++++---
include/scsi/scsi_host.h | 17 +++++++++
6 files changed, 141 insertions(+), 7 deletions(-)
diff -upNr a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c 2009-10-26 12:58:17.000000000 -0400
+++ b/drivers/scsi/hosts.c 2009-11-02 17:44:44.000000000 -0500
@@ -40,6 +40,9 @@
#include "scsi_logging.h"
+static DEFINE_SPINLOCK(transport_devs_lock);
+static struct list_head transport_devs;
+
static atomic_t scsi_host_next_hn; /* host_no for next new host */
@@ -504,6 +507,7 @@ EXPORT_SYMBOL(scsi_host_put);
int scsi_init_hosts(void)
{
+ INIT_LIST_HEAD(&transport_devs);
return class_register(&shost_class);
}
@@ -560,3 +564,71 @@ void scsi_flush_work(struct Scsi_Host *s
flush_workqueue(shost->work_q);
}
EXPORT_SYMBOL_GPL(scsi_flush_work);
+
+
+struct scsi_transport_dev {
+ struct list_head devs_list;
+ struct device_type *type;
+};
+
+/**
+ * scsi_reg_transport_dev_type - Register transport object device type that is
+ * to be ignored when searching for the first
+ * non-scsi object in the object tree.
+ * @dev_type: Pointer to a "struct device_type" structure
+ */
+int scsi_reg_transport_dev_type(struct device_type *dev_type)
+{
+ struct scsi_transport_dev *dtype;
+ unsigned long flags;
+
+ dtype = kzalloc(sizeof(struct scsi_transport_dev), GFP_KERNEL);
+ if (!dtype)
+ return 1;
+ dtype->type = dev_type;
+ spin_lock_irqsave(&transport_devs_lock, flags);
+ list_add_tail(&dtype->devs_list, &transport_devs);
+ spin_unlock_irqrestore(&transport_devs_lock, flags);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(scsi_reg_transport_dev_type);
+
+/**
+ * scsi_unreg_transport_dev_type - De-register a transport object device type
+ * @dev_type: Pointer to a "struct device_type" structure
+ */
+void scsi_unreg_transport_dev_type(struct device_type *dev_type)
+{
+ struct scsi_transport_dev *dtype;
+ unsigned long flags;
+
+ spin_lock_irqsave(&transport_devs_lock, flags);
+ list_for_each_entry(dtype, &transport_devs, devs_list) {
+ if (dtype->type == dev_type) {
+ list_del(&dtype->devs_list);
+ kfree(dtype);
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&transport_devs_lock, flags);
+}
+EXPORT_SYMBOL_GPL(scsi_unreg_transport_dev_type);
+
+int scsi_is_transport_device(const struct device *dev)
+{
+ struct scsi_transport_dev *dtype;
+ unsigned long flags;
+
+ spin_lock_irqsave(&transport_devs_lock, flags);
+ list_for_each_entry(dtype, &transport_devs, devs_list) {
+ if (dtype->type == dev->type) {
+ spin_unlock_irqrestore(&transport_devs_lock, flags);
+ return 1;
+ }
+ }
+ spin_unlock_irqrestore(&transport_devs_lock, flags);
+ return 0;
+}
+EXPORT_SYMBOL(scsi_is_transport_device);
+
+
diff -upNr a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c 2009-10-26 12:58:17.000000000 -0400
+++ b/drivers/scsi/scsi_lib.c 2009-11-01 09:21:46.000000000 -0500
@@ -1611,7 +1611,7 @@ struct request_queue *__scsi_alloc_queue
request_fn_proc *request_fn)
{
struct request_queue *q;
- struct device *dev = shost->shost_gendev.parent;
+ struct device *dev = dev_to_nonscsi_dev(shost->shost_gendev.parent);
q = blk_init_queue(request_fn, NULL);
if (!q)
diff -upNr a/drivers/scsi/scsi_lib_dma.c b/drivers/scsi/scsi_lib_dma.c
--- a/drivers/scsi/scsi_lib_dma.c 2009-10-26 12:58:17.000000000 -0400
+++ b/drivers/scsi/scsi_lib_dma.c 2009-11-01 09:21:46.000000000 -0500
@@ -23,7 +23,8 @@ 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;
+ struct device *dev = dev_to_nonscsi_dev(
+ cmd->device->host->shost_gendev.parent);
nseg = dma_map_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
cmd->sc_data_direction);
@@ -41,10 +42,12 @@ EXPORT_SYMBOL(scsi_dma_map);
void scsi_dma_unmap(struct scsi_cmnd *cmd)
{
if (scsi_sg_count(cmd)) {
- struct device *dev = cmd->device->host->shost_gendev.parent;
+ struct device *dev = dev_to_nonscsi_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 -upNr a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
--- a/drivers/scsi/scsi_priv.h 2009-10-26 12:58:17.000000000 -0400
+++ b/drivers/scsi/scsi_priv.h 2009-11-01 10:23:08.000000000 -0500
@@ -23,6 +23,8 @@ struct scsi_nl_hdr;
/* hosts.c */
extern int scsi_init_hosts(void);
extern void scsi_exit_hosts(void);
+extern int scsi_reg_transport_dev_type(struct device_type *dev_type);
+extern void scsi_unreg_transport_dev_type(struct device_type *dev_type);
/* scsi.c */
extern int scsi_dispatch_cmd(struct scsi_cmnd *cmd);
diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
--- a/drivers/scsi/scsi_transport_fc.c 2009-10-26 12:58:17.000000000 -0400
+++ b/drivers/scsi/scsi_transport_fc.c 2009-11-01 10:58:35.000000000 -0500
@@ -48,6 +48,8 @@ static int fc_bsg_hostadd(struct Scsi_Ho
static int fc_bsg_rportadd(struct Scsi_Host *, struct fc_rport *);
static void fc_bsg_remove(struct request_queue *);
static void fc_bsg_goose_queue(struct fc_rport *);
+static int fc_transport_reg_devtypes(void);
+static void fc_transport_unreg_devtypes(void);
/*
* Redefine so that we can have same named attributes in the
@@ -643,6 +645,9 @@ static __init int fc_transport_init(void
atomic_set(&fc_event_seq, 0);
+ error = fc_transport_reg_devtypes();
+ if (error)
+ return error;
error = transport_class_register(&fc_host_class);
if (error)
return error;
@@ -661,6 +666,7 @@ static void __exit fc_transport_exit(voi
transport_class_unregister(&fc_rport_class);
transport_class_unregister(&fc_host_class);
transport_class_unregister(&fc_vport_class);
+ fc_transport_unreg_devtypes();
}
/*
@@ -1854,9 +1860,14 @@ static void fc_rport_dev_release(struct
kfree(rport);
}
+static struct device_type fc_rport_dev_type = {
+ .name = "fc_rport",
+ .release = fc_rport_dev_release,
+};
+
int scsi_is_fc_rport(const struct device *dev)
{
- return dev->release == fc_rport_dev_release;
+ return dev->type == &fc_rport_dev_type;
}
EXPORT_SYMBOL(scsi_is_fc_rport);
@@ -1887,9 +1898,14 @@ static void fc_vport_dev_release(struct
kfree(vport);
}
+static struct device_type fc_vport_dev_type = {
+ .name = "fc_vport",
+ .release = fc_vport_dev_release,
+};
+
int scsi_is_fc_vport(const struct device *dev)
{
- return dev->release == fc_vport_dev_release;
+ return dev->type == &fc_vport_dev_type;
}
EXPORT_SYMBOL(scsi_is_fc_vport);
@@ -1915,6 +1931,30 @@ static int fc_vport_match(struct attribu
/**
+ * fc_transport_reg_devtypes - Registers topology device types that need
+ * to be skipped when traversing the topology
+ * tree to find the first non-scsi object which
+ * is then used for DMA masks, etc.
+ */
+static int fc_transport_reg_devtypes(void)
+{
+ if (scsi_reg_transport_dev_type(&fc_vport_dev_type))
+ return -ENOMEM;
+ return 0;
+}
+
+/**
+ * fc_transport_unreg_devtypes - De-registers topology device types that need
+ * to be skipped when traversing the topology
+ * tree.
+ */
+static void fc_transport_unreg_devtypes(void)
+{
+ scsi_unreg_transport_dev_type(&fc_vport_dev_type);
+}
+
+
+/**
* fc_timed_out - FC Transport I/O timeout intercept handler
* @scmd: The SCSI command which timed out
*
@@ -2510,7 +2550,7 @@ fc_rport_create(struct Scsi_Host *shost,
dev = &rport->dev;
device_initialize(dev); /* takes self reference */
dev->parent = get_device(&shost->shost_gendev); /* parent reference */
- dev->release = fc_rport_dev_release;
+ dev->type = &fc_rport_dev_type;
dev_set_name(dev, "rport-%d:%d-%d",
shost->host_no, channel, rport->number);
transport_setup_device(dev);
@@ -3214,7 +3254,7 @@ fc_vport_setup(struct Scsi_Host *shost,
dev = &vport->dev;
device_initialize(dev); /* takes self reference */
dev->parent = get_device(pdev); /* takes parent reference */
- dev->release = fc_vport_dev_release;
+ dev->type = &fc_vport_dev_type;
dev_set_name(dev, "vport-%d:%d-%d",
shost->host_no, channel, vport->number);
transport_setup_device(dev);
diff -upNr a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h 2009-10-30 10:31:52.000000000 -0400
+++ b/include/scsi/scsi_host.h 2009-11-01 09:36:25.000000000 -0500
@@ -703,7 +703,12 @@ static inline void *shost_priv(struct Sc
}
int scsi_is_host_device(const struct device *);
+int scsi_is_transport_device(const struct device *);
+/*
+ * walks object list backward, to find the first shost object.
+ * Skips over transport objects that may not be stargets, etc
+ */
static inline struct Scsi_Host *dev_to_shost(struct device *dev)
{
while (!scsi_is_host_device(dev)) {
@@ -714,6 +719,18 @@ static inline struct Scsi_Host *dev_to_s
return container_of(dev, struct Scsi_Host, shost_gendev);
}
+/*
+ * walks object list backward, to find the first non-scsi object
+ * Skips over transport objects that may be vports, shosts under vports, etc
+ */
+static inline struct device *dev_to_nonscsi_dev(struct device *dev)
+{
+ while (dev->parent &&
+ (scsi_is_host_device(dev) || scsi_is_transport_device(dev)))
+ dev = dev->parent;
+ return dev;
+}
+
static inline int scsi_host_in_recovery(struct Scsi_Host *shost)
{
return shost->shost_state == SHOST_RECOVERY ||
On Tue, 2009-11-03 at 19:45 -0500, James Smart wrote:
> I had reminded James B of a patch for an issue with the scsi subsystem
> not properly finding the parent device for dma operations when we had
> nested scsi_hosts..
> http://marc.info/?l=linux-scsi&m=125372757108048&w=2
>
> The patch was picked up, and integrated into linux-next, which started
> to see issues, and which James B cut an additional patch to deal with
> traversing too much of the tree (stopping when dev->parent == NULL)..
> The http://marc.info/?l=linux-scsi&m=125414973520985&w=2
>
> The real issue was the traversal of a node that had "dev->type == NULL".
> Turns out standard PCI endpoint and bridge/domain objects have NULL types
> too - so every traversal went all the way to the root of the tree.
>
> The attached patch, cut against scsi-misc-2.6 - replaces both of the
> patches above. Rather than making any assumptions about dev->type values,
> it now explicitly matches dev->type structures to known scsi or transport
> objects, and only traverses them if they are recognized. To get around
> symbol dependencies, and to allow transports as modules to come and go,
> a registration interface was put in place to register transport objects
> with the routine that does the matching. The FC transport was converted
> to use a device_type structure for its objects (note: perhaps other
> transports should consider the same ?)
>
> Please apply this patch as soon as possible. At the current time,
> NPIV vport dma operation is severely broken.
>
> Thanks
>
> -- james s
>
>
>
>
> Signed-off-by: James Smart <[email protected]>
>
> ---
>
> drivers/scsi/hosts.c | 72 +++++++++++++++++++++++++++++++++++++++
> drivers/scsi/scsi_lib.c | 2 -
> drivers/scsi/scsi_lib_dma.c | 7 ++-
> drivers/scsi/scsi_priv.h | 2 +
> drivers/scsi/scsi_transport_fc.c | 48 +++++++++++++++++++++++---
> include/scsi/scsi_host.h | 17 +++++++++
> 6 files changed, 141 insertions(+), 7 deletions(-)
141 lines plus a static list to solve a simple problem is getting a bit
icky to say the least.
What about being more simplistic and simply making the host cache a
pointer to the physical bus device? I probably objected to this a long
time ago because using the parent pointers is more elegant, but I think
this patch demonstrates conclusively it's not worth this amount of code
for the sake of alleged elegance.
James
---
drivers/scsi/hosts.c | 13 ++++++++++---
drivers/scsi/lpfc/lpfc_init.c | 2 +-
drivers/scsi/qla2xxx/qla_attr.c | 3 ++-
drivers/scsi/scsi_lib_dma.c | 4 ++--
include/scsi/scsi_host.h | 16 +++++++++++++++-
5 files changed, 30 insertions(+), 8 deletions(-)
---
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 5fd2da4..28a753d 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -180,14 +180,20 @@ void scsi_remove_host(struct Scsi_Host *shost)
EXPORT_SYMBOL(scsi_remove_host);
/**
- * scsi_add_host - add a scsi host
+ * scsi_add_host_with_dma - add a scsi host with dma device
* @shost: scsi host pointer to add
* @dev: a struct device of type scsi class
+ * @dma_dev: dma device for the host
+ *
+ * Note: You rarely need to worry about this unless you're in a
+ * virtualised host environments, so use the simpler scsi_add_host()
+ * function instead.
*
* Return value:
* 0 on success / != 0 for error
**/
-int scsi_add_host(struct Scsi_Host *shost, struct device *dev)
+int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
+ struct device *dma_dev)
{
struct scsi_host_template *sht = shost->hostt;
int error = -EINVAL;
@@ -207,6 +213,7 @@ int scsi_add_host(struct Scsi_Host *shost, struct device *dev)
if (!shost->shost_gendev.parent)
shost->shost_gendev.parent = dev ? dev : &platform_bus;
+ shost->dma_dev = dma_dev;
error = device_add(&shost->shost_gendev);
if (error)
@@ -262,7 +269,7 @@ int scsi_add_host(struct Scsi_Host *shost, struct device *dev)
fail:
return error;
}
-EXPORT_SYMBOL(scsi_add_host);
+EXPORT_SYMBOL(scsi_add_host_with_dma);
static void scsi_host_dev_release(struct device *dev)
{
diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 562d8ce..f913f1e 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -2408,7 +2408,7 @@ lpfc_create_port(struct lpfc_hba *phba, int instance, struct device *dev)
vport->els_tmofunc.function = lpfc_els_timeout;
vport->els_tmofunc.data = (unsigned long)vport;
- error = scsi_add_host(shost, dev);
+ error = scsi_add_host_with_dma(shost, dev, &phba->pcidev->dev);
if (error)
goto out_put_shost;
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index fbcb82a..21e2bc4 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -1654,7 +1654,8 @@ qla24xx_vport_create(struct fc_vport *fc_vport, bool disable)
fc_vport_set_state(fc_vport, FC_VPORT_LINKDOWN);
}
- if (scsi_add_host(vha->host, &fc_vport->dev)) {
+ if (scsi_add_host_with_dma(vha->host, &fc_vport->dev,
+ &ha->pdev->dev)) {
DEBUG15(printk("scsi(%ld): scsi_add_host failure for VP[%d].\n",
vha->host_no, vha->vp_idx));
goto vport_create_failed_2;
diff --git a/drivers/scsi/scsi_lib_dma.c b/drivers/scsi/scsi_lib_dma.c
index ac6855c..dcd1285 100644
--- a/drivers/scsi/scsi_lib_dma.c
+++ b/drivers/scsi/scsi_lib_dma.c
@@ -23,7 +23,7 @@ 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;
+ struct device *dev = cmd->device->host->dma_dev;
nseg = dma_map_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
cmd->sc_data_direction);
@@ -41,7 +41,7 @@ EXPORT_SYMBOL(scsi_dma_map);
void scsi_dma_unmap(struct scsi_cmnd *cmd)
{
if (scsi_sg_count(cmd)) {
- struct device *dev = cmd->device->host->shost_gendev.parent;
+ struct device *dev = cmd->device->host->dma_dev;
dma_unmap_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
cmd->sc_data_direction);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 943f5df..68338be 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -683,6 +683,12 @@ struct Scsi_Host {
void *shost_data;
/*
+ * Points to the physical bus device we'd use to do DMA
+ * Needed just in case we have virtual hosts.
+ */
+ struct device *dma_dev;
+
+ /*
* We should ensure that this is aligned, both for better performance
* and also because some compilers (m68k) don't automatically force
* alignment to a long boundary.
@@ -726,7 +732,9 @@ extern int scsi_queue_work(struct Scsi_Host *, struct work_struct *);
extern void scsi_flush_work(struct Scsi_Host *);
extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
-extern int __must_check scsi_add_host(struct Scsi_Host *, struct device *);
+extern int __must_check scsi_add_host_with_dma(struct Scsi_Host *,
+ struct device *,
+ struct device *);
extern void scsi_scan_host(struct Scsi_Host *);
extern void scsi_rescan_device(struct device *);
extern void scsi_remove_host(struct Scsi_Host *);
@@ -737,6 +745,12 @@ extern const char *scsi_host_state_name(enum scsi_host_state);
extern u64 scsi_calculate_bounce_limit(struct Scsi_Host *);
+static inline int __must_check scsi_add_host(struct Scsi_Host *host,
+ struct device *dev)
+{
+ return scsi_add_host_with_dma(host, dev, dev);
+}
+
static inline struct device *scsi_get_device(struct Scsi_Host *shost)
{
return shost->shost_gendev.parent;
James,
This updated patch works fine for lpfc and our vports...
Acked-By: James Smart <[email protected]>
-- james s
James Bottomley wrote:
>
> 141 lines plus a static list to solve a simple problem is getting a bit
> icky to say the least.
>
> What about being more simplistic and simply making the host cache a
> pointer to the physical bus device? I probably objected to this a long
> time ago because using the parent pointers is more elegant, but I think
> this patch demonstrates conclusively it's not worth this amount of code
> for the sake of alleged elegance.
>
> James
>
> ---
> drivers/scsi/hosts.c | 13 ++++++++++---
> drivers/scsi/lpfc/lpfc_init.c | 2 +-
> drivers/scsi/qla2xxx/qla_attr.c | 3 ++-
> drivers/scsi/scsi_lib_dma.c | 4 ++--
> include/scsi/scsi_host.h | 16 +++++++++++++++-
> 5 files changed, 30 insertions(+), 8 deletions(-)