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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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