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

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

[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.

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 <james.smart@emulex.com>

 ---

 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
--- a/drivers/scsi/scsi_lib_dma.c	2008-09-23 15:12:13.000000000 -0400
+++ b2/drivers/scsi/scsi_lib_dma.c	2008-10-03 12:19:39.000000000 -0400
@@ -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);
+
diff -upNr a/include/scsi/scsi_host.h b2/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	2008-09-23 15:12:20.000000000 -0400
+++ b2/include/scsi/scsi_host.h	2008-10-03 12:16:46.000000000 -0400
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi_lib_dma.c : fix bug w/ dma maps on virtual fc ports

[James Bottomley]

On Tue, 2008-10-07 at 09:35 -0400, James Smart wrote:
> 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.

> 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().

> -- james s
> 
> 
>  Signed-off-by: James Smart <james.smart@emulex.com>
> 
>  ---
> 
>  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
> --- a/drivers/scsi/scsi_lib_dma.c	2008-09-23 15:12:13.000000000 -0400
> +++ b2/drivers/scsi/scsi_lib_dma.c	2008-10-03 12:19:39.000000000 -0400
> @@ -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);

> -		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
> --- a/include/scsi/scsi_host.h	2008-09-23 15:12:20.000000000 -0400
> +++ b2/include/scsi/scsi_host.h	2008-10-03 12:16:46.000000000 -0400
> @@ -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 ||

James


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

Re: [PATCH] scsi_lib_dma.c : fix bug w/ dma maps on virtual fc ports

[FUJITA Tomonori]

On Tue, 07 Oct 2008 08:53:49 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Tue, 2008-10-07 at 09:35 -0400, James Smart wrote:
> > 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.

Re: [PATCH] scsi_lib_dma.c : fix bug w/ dma maps on virtual fc ports

[Mike Christie]

James Smart wrote:
>  
> +/*
> + * 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;
> +}
> +

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

Re: [PATCH] scsi_lib_dma.c : fix bug w/ dma maps on virtual fc ports

[Mike Christie]

Mike Christie wrote:
> James Smart wrote:
>>  
>> +/*
>> + * 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;
>> +}
>> +
> 
> I think you will want to call this in __scsi_alloc_queue too.

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