scsi_lib_dma.c : fix bug w/ dma maps on virtual fc ports

PATCH In review James Smart 4 participants 5 messages Oct 7, 2008 – Oct 13, 2008 SCSI

Share & tools

James Smart

author

2008-10-07 13:35 UTC #0 lore raw

PATCH 1/1 scsi_lib_dma.c : fix bug w/ dma maps on virtual fc ports

When the updated scsi dma code was introduced recently, it assumed the physical host/adapter was the parent of the scsi host. Unfortunately, on FC virtual ports, the parent of the scsi host is the virtual port, which does not have dma information.

I have updated the dma map routines to use a function that finds the top-most scsi host, then use the parent of that host. This is a bit ugly as every dma request does this walking, to the top of the object tree.

A different option that was considered was taking the parent dev, then walking the dev list backward to find the first device with a dma mask, then using that device.
Eg: while (!dev->dma_mask) dev=dev->parent;

If there is a better solution, please let me know.

-- james s

Signed-off-by James Smart <[email protected]>

---

 drivers/scsi/scsi_lib_dma.c |   13 +++++++++----
 include/scsi/scsi_host.h    |   21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

drivers/scsi/scsi_lib_dma.c +9 -4

diff -upNr a/drivers/scsi/scsi_lib_dma.c b2/drivers/scsi/scsi_lib_dma.c
@@ -23,9 +23,11 @@ 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 Scsi_Host *shost;
 
-		nseg = dma_map_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
+		shost = dev_to_phys_shost(&cmd->device->sdev_gendev);
+		nseg = dma_map_sg(shost->shost_gendev.parent,
+				  scsi_sglist(cmd), scsi_sg_count(cmd),
 				  cmd->sc_data_direction);
 		if (unlikely(!nseg))
 			return -ENOMEM;

@@ -41,10 +43,13 @@ 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 Scsi_Host *shost;
 
-		dma_unmap_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
+		shost = dev_to_phys_shost(&cmd->device->sdev_gendev);
+		dma_unmap_sg(shost->shost_gendev.parent,
+			     scsi_sglist(cmd), scsi_sg_count(cmd),
 			     cmd->sc_data_direction);
 	}
 }
 EXPORT_SYMBOL(scsi_dma_unmap);
+

include/scsi/scsi_host.h +7 -0

diff -upNr a/include/scsi/scsi_host.h b2/include/scsi/scsi_host.h
@@ -696,6 +696,10 @@ static inline void *shost_priv(struct Sc
 
 int scsi_is_host_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)) {

@@ -706,6 +710,23 @@ static inline struct Scsi_Host *dev_to_s
 	return container_of(dev, struct Scsi_Host, shost_gendev);
 }
 
+/*
+ * walks object list backward, to find the top-most shost object.
+ * Skips over transport objects that may be vports, shosts under

vports, etc
+ */
+static inline struct Scsi_Host *dev_to_phys_shost(struct device *dev) +{
+ struct Scsi_Host *shost_new = dev_to_shost(dev);
+ struct Scsi_Host *shost = shost_new;
+
+ while (shost_new) {
+ shost_new = dev_to_shost(shost->shost_gendev.parent);
+ if (shost_new)
+ shost = shost_new;
+ }
+ return shost;
+}
+
static inline int scsi_host_in_recovery(struct Scsi_Host *shost) {
return shost->shost_state == SHOST_RECOVERY ||

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2008-10-07 13:54 UTC #1 lore raw

Quoted 4 lines · James Smart

When the updated scsi dma code was introduced recently, it assumed the physical host/adapter was the parent of the scsi host. Unfortunately, on FC virtual ports, the parent of the scsi host is the virtual port, which does not have dma information.

Hmm, so now we have nested hosts; I'd missed that piece; we might be in trouble in other places than just here.

Quoted 11 lines

I have updated the dma map routines to use a function that finds the top-most scsi host, then use the parent of that host. This is a bit ugly as every dma request does this walking, to the top of the object tree.
A different option that was considered was taking the parent dev, then walking the dev list backward to find the first device with a dma mask, then using that device.
Eg: while (!dev->dma_mask) dev=dev->parent;
If there is a better solution, please let me know.

I think there is: just do what you want. i.e. an accessor that finds the parent bus device. So you walk up the tree and continue walking if either the type is NULL or the type is SCSI ... that way the first thing you come across should be the physical bus attached device. Call it something like to_non_scsi_dev().

Quoted 16 lines

-- james s
Signed-off-by James Smart <[email protected]>
---
 drivers/scsi/scsi_lib_dma.c |   13 +++++++++----
 include/scsi/scsi_host.h    |   21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)
diff -upNr a/drivers/scsi/scsi_lib_dma.c b2/drivers/scsi/scsi_lib_dma.c

drivers/scsi/scsi_lib_dma.c int scsi_dma_map(struct scsi_cmnd *cmd) · line 26

Quoted 5 lines

drivers/scsi/scsi_lib_dma.c +1 -1

@@ -23,9 +23,11 @@ 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 Scsi_Host *shost;

You have a bit of a penchant for code churn here. What's wrong with

struct device *dev = dev_to_phys_shost(&cmd->device->sdev_gendev)->shost_gendev.parent;

Then none of the code below needs changing.

or if you use to_non_scsi_dev() it's even simpler:

struct device *dev = to_non_scsi_dev(&cmd->device->sdev_gendev);

Quoted 52 lines

drivers/scsi/scsi_lib_dma.c +8 -3

-		nseg = dma_map_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
+		shost = dev_to_phys_shost(&cmd->device->sdev_gendev);
+		nseg = dma_map_sg(shost->shost_gendev.parent,
+				  scsi_sglist(cmd), scsi_sg_count(cmd),
 				  cmd->sc_data_direction);
 		if (unlikely(!nseg))
 			return -ENOMEM;

@@ -41,10 +43,13 @@ 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 Scsi_Host *shost;
 
-		dma_unmap_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
+		shost = dev_to_phys_shost(&cmd->device->sdev_gendev);
+		dma_unmap_sg(shost->shost_gendev.parent,
+			     scsi_sglist(cmd), scsi_sg_count(cmd),
 			     cmd->sc_data_direction);
 	}
 }
 EXPORT_SYMBOL(scsi_dma_unmap);
+

diff -upNr a/include/scsi/scsi_host.h b2/include/scsi/scsi_host.h

include/scsi/scsi_host.h +7 -0

@@ -696,6 +696,10 @@ static inline void *shost_priv(struct Sc
 
 int scsi_is_host_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)) {

@@ -706,6 +710,23 @@ static inline struct Scsi_Host *dev_to_s
 	return container_of(dev, struct Scsi_Host, shost_gendev);
 }
 
+/*
+ * walks object list backward, to find the top-most shost object.
+ * Skips over transport objects that may be vports, shosts under

vports, etc
+ */

include/scsi/scsi_host.h +6 -0

+static inline struct Scsi_Host *dev_to_phys_shost(struct device *dev)
+{
+	struct Scsi_Host *shost_new = dev_to_shost(dev);
+	struct Scsi_Host *shost = shost_new;
+
+	while (shost_new) {

include/scsi/scsi_host.h

Quoted 10 lines

include/scsi/scsi_host.h +7 -0

+		shost_new = dev_to_shost(shost->shost_gendev.parent);
+		if (shost_new)
+			shost = shost_new;
+	}
+	return shost;
+}
+
 static inline int scsi_host_in_recovery(struct Scsi_Host *shost)
 {
 	return shost->shost_state == SHOST_RECOVERY ||

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

FUJITA Tomonori

2008-10-07 15:48 UTC #2 lore raw

Quoted 8 lines · On Tue, 07 Oct 2008 08:53:49 -0500 James Bottomley…

Quoted 4 lines · James Smart
When the updated scsi dma code was introduced recently, it assumed the physical host/adapter was the parent of the scsi host. Unfortunately, on FC virtual ports, the parent of the scsi host is the virtual port, which does not have dma information.
Hmm, so now we have nested hosts; I'd missed that piece; we might be in trouble in other places than just here.

This is a problem that we met one year ago?

http://marc.info/?t=118312464100002&r=1&w=2

vport's shost is not initialized like a normal shost so it doesn't have dma information.

The updated patch looks nice. I should have done it at the time.

Mike Christie

2008-10-13 18:37 UTC #3 lore raw

In reply to James Smart

Quoted 9 lines · James Smart

+/*
+ * walks object list backward, to find the top-most shost object.
+ * Skips over transport objects that may be vports, shosts under vports, etc
+ */
(quoted diff) +3 -0
+static inline struct Scsi_Host *dev_to_phys_shost(struct device *dev)
+{
+	struct Scsi_Host *shost_new = dev_to_shost(dev);

Review

Quoted 10 lines

(quoted diff) +10 -0

+	struct Scsi_Host *shost = shost_new;
+
+	while (shost_new) {
+		shost_new = dev_to_shost(shost->shost_gendev.parent);
+		if (shost_new)
+			shost = shost_new;
+	}
+	return shost;
+}
+

I think you will want to call this in __scsi_alloc_queue too.

Mike Christie

2008-10-13 18:39 UTC #4 lore raw

Quoted 13 lines · Mike Christie

Quoted 12 lines · James Smart
+/*
+ * walks object list backward, to find the top-most shost object.
+ * Skips over transport objects that may be vports, shosts under vports, etc + */
(quoted diff) +7 -0
+static inline struct Scsi_Host *dev_to_phys_shost(struct device *dev)
+{
+    struct Scsi_Host *shost_new = dev_to_shost(dev);
+    struct Scsi_Host *shost = shost_new;
+
+    while (shost_new) {
+        shost_new = dev_to_shost(shost->shost_gendev.parent);

Quoted 8 lines

Review
Quoted 6 lines
(quoted diff) +6 -0
+        if (shost_new)
+            shost = shost_new;
+    }
+    return shost;
+}
+
I think you will want to call this in __scsi_alloc_queue too.

Ignore. This I see it fixed in the updated patch.