[PATCH v2 0/4] Series short description

[PATCH v2 0/4] Series short description

[Mike Marciniszyn]

This patch series modified the ib_sg_dma API to
eliminate the .dma_len and .dma_address methods.

In all present cases that overload these methods
(ipath, qib, ehca), the lack of these methods are
compensated for by code changes to the driver .map_sg
to insure that the vanilla sg_dma_address() and
sg_dma_len() will do the same thing as the equivalent
former ib_sg_dma_address() and ib_sg_dma_len() calls
into the drivers.

This patch series is a followup to this recent submission
http://marc.info/?l=linux-rdma&m=139602422108727&w=2
and Bart's similar comment in
http://marc.info/?l=linux-netdev&m=135643746610259&w=2.

Roland, I'm not sure of the history of these methods and
what rationale caused them to be added?

There is obviously an interim step that could be done
here to preseve the overload by not changing verbs, yet
change qib/ipath/echa to not overload them.  This would
allow the drivers to work even when the "correct" API's
had not been used by the ULP.

This version reorders the patch series order based
on comments from Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
so that the overload removal is last.

The commit messages are enhanced a bit as well.

---

Mike Marciniszyn (4):
      IB/qib: remove ib_sg_dma_address() and ib_sg_dma_len() overloads
      IB/ipath: remove ib_sg_dma_address() and ib_sg_dma_len() overloads
      IB/ehca: remove ib_sg_dma_address() and ib_sg_dma_len() overloads
      IB/core: Remove overload in ib_sg_dma*


 drivers/infiniband/hw/ehca/ehca_mrmw.c  |   12 ---------
 drivers/infiniband/hw/ipath/ipath_dma.c |   43 +++++++++++--------------------
 drivers/infiniband/hw/qib/qib_dma.c     |   21 +++------------
 include/rdma/ib_verbs.h                 |   14 ++++------
 4 files changed, 25 insertions(+), 65 deletions(-)

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

[PATCH v2 1/4] IB/qib: remove ib_sg_dma_address() and ib_sg_dma_len() overloads

[Mike Marciniszyn]

Remove the overload for .dma_len and .dma_address

The lack of these methods is compensated for by code changes
to .map_sg to insure that the vanilla sg_dma_address() and
sg_dma_len() will do the same thing as the equivalent
former ib_sg_dma_address() and ib_sg_dma_len() calls
into the drivers.

Suggested-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Tested-by: Vinod Kumar <vinod.kumar-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/hw/qib/qib_dma.c |   21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_dma.c b/drivers/infiniband/hw/qib/qib_dma.c
index 2920bb3..59fe092 100644
--- a/drivers/infiniband/hw/qib/qib_dma.c
+++ b/drivers/infiniband/hw/qib/qib_dma.c
@@ -108,6 +108,10 @@ static int qib_map_sg(struct ib_device *dev, struct scatterlist *sgl,
 			ret = 0;
 			break;
 		}
+		sg->dma_address = addr + sg->offset;
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+		sg->dma_length = sg->length;
+#endif
 	}
 	return ret;
 }
@@ -119,21 +123,6 @@ static void qib_unmap_sg(struct ib_device *dev,
 	BUG_ON(!valid_dma_direction(direction));
 }
 
-static u64 qib_sg_dma_address(struct ib_device *dev, struct scatterlist *sg)
-{
-	u64 addr = (u64) page_address(sg_page(sg));
-
-	if (addr)
-		addr += sg->offset;
-	return addr;
-}
-
-static unsigned int qib_sg_dma_len(struct ib_device *dev,
-				   struct scatterlist *sg)
-{
-	return sg->length;
-}
-
 static void qib_sync_single_for_cpu(struct ib_device *dev, u64 addr,
 				    size_t size, enum dma_data_direction dir)
 {
@@ -173,8 +162,6 @@ struct ib_dma_mapping_ops qib_dma_mapping_ops = {
 	.unmap_page = qib_dma_unmap_page,
 	.map_sg = qib_map_sg,
 	.unmap_sg = qib_unmap_sg,
-	.dma_address = qib_sg_dma_address,
-	.dma_len = qib_sg_dma_len,
 	.sync_single_for_cpu = qib_sync_single_for_cpu,
 	.sync_single_for_device = qib_sync_single_for_device,
 	.alloc_coherent = qib_dma_alloc_coherent,

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

[PATCH v2 2/4] IB/ipath: remove ib_sg_dma_address() and ib_sg_dma_len() overloads

[Mike Marciniszyn]

The lack of these methods is compensated for by code changes
to .map_sg to insure that the vanilla sg_dma_address() and
sg_dma_len() will do the same thing as the equivalent
former ib_sg_dma_address() and ib_sg_dma_len() calls
into the drivers.

Suggested-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/hw/ipath/ipath_dma.c |   43 +++++++++++--------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/infiniband/hw/ipath/ipath_dma.c b/drivers/infiniband/hw/ipath/ipath_dma.c
index 644c2c7..123a8c0 100644
--- a/drivers/infiniband/hw/ipath/ipath_dma.c
+++ b/drivers/infiniband/hw/ipath/ipath_dma.c
@@ -115,6 +115,10 @@ static int ipath_map_sg(struct ib_device *dev, struct scatterlist *sgl,
 			ret = 0;
 			break;
 		}
+		sg->dma_address = addr + sg->offset;
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+		sg->dma_length = sg->length;
+#endif
 	}
 	return ret;
 }
@@ -126,21 +130,6 @@ static void ipath_unmap_sg(struct ib_device *dev,
 	BUG_ON(!valid_dma_direction(direction));
 }
 
-static u64 ipath_sg_dma_address(struct ib_device *dev, struct scatterlist *sg)
-{
-	u64 addr = (u64) page_address(sg_page(sg));
-
-	if (addr)
-		addr += sg->offset;
-	return addr;
-}
-
-static unsigned int ipath_sg_dma_len(struct ib_device *dev,
-				     struct scatterlist *sg)
-{
-	return sg->length;
-}
-
 static void ipath_sync_single_for_cpu(struct ib_device *dev,
 				      u64 addr,
 				      size_t size,
@@ -176,17 +165,15 @@ static void ipath_dma_free_coherent(struct ib_device *dev, size_t size,
 }
 
 struct ib_dma_mapping_ops ipath_dma_mapping_ops = {
-	ipath_mapping_error,
-	ipath_dma_map_single,
-	ipath_dma_unmap_single,
-	ipath_dma_map_page,
-	ipath_dma_unmap_page,
-	ipath_map_sg,
-	ipath_unmap_sg,
-	ipath_sg_dma_address,
-	ipath_sg_dma_len,
-	ipath_sync_single_for_cpu,
-	ipath_sync_single_for_device,
-	ipath_dma_alloc_coherent,
-	ipath_dma_free_coherent
+	.mapping_error = ipath_mapping_error,
+	.map_single = ipath_dma_map_single,
+	.unmap_single = ipath_dma_unmap_single,
+	.map_page = ipath_dma_map_page,
+	.unmap_page = ipath_dma_unmap_page,
+	.map_sg = ipath_map_sg,
+	.unmap_sg = ipath_unmap_sg,
+	.sync_single_for_cpu = ipath_sync_single_for_cpu,
+	.sync_single_for_device = ipath_sync_single_for_device,
+	.alloc_coherent = ipath_dma_alloc_coherent,
+	.free_coherent = ipath_dma_free_coherent
 };

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

Re: [PATCH v2 2/4] IB/ipath: remove ib_sg_dma_address() and ib_sg_dma_len() overloads

[Yann Droneaud]

Le vendredi 28 mars 2014 à 13:26 -0400, Mike Marciniszyn a écrit :
> The lack of these methods is compensated for by code changes
> to .map_sg to insure that the vanilla sg_dma_address() and
> sg_dma_len() will do the same thing as the equivalent
> former ib_sg_dma_address() and ib_sg_dma_len() calls
> into the drivers.
> 

And struct ipath_dma_mapping_ops was converted to C99 initializer.

> Suggested-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Cc: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/infiniband/hw/ipath/ipath_dma.c |   43 +++++++++++--------------------
>  1 file changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/ipath/ipath_dma.c b/drivers/infiniband/hw/ipath/ipath_dma.c
> index 644c2c7..123a8c0 100644
> --- a/drivers/infiniband/hw/ipath/ipath_dma.c
> +++ b/drivers/infiniband/hw/ipath/ipath_dma.c
> @@ -115,6 +115,10 @@ static int ipath_map_sg(struct ib_device *dev, struct scatterlist *sgl,
>  			ret = 0;
>  			break;
>  		}
> +		sg->dma_address = addr + sg->offset;
> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
> +		sg->dma_length = sg->length;
> +#endif
>  	}
>  	return ret;
>  }
> @@ -126,21 +130,6 @@ static void ipath_unmap_sg(struct ib_device *dev,
>  	BUG_ON(!valid_dma_direction(direction));
>  }
>  
> -static u64 ipath_sg_dma_address(struct ib_device *dev, struct scatterlist *sg)
> -{
> -	u64 addr = (u64) page_address(sg_page(sg));
> -
> -	if (addr)
> -		addr += sg->offset;
> -	return addr;
> -}
> -
> -static unsigned int ipath_sg_dma_len(struct ib_device *dev,
> -				     struct scatterlist *sg)
> -{
> -	return sg->length;
> -}
> -
>  static void ipath_sync_single_for_cpu(struct ib_device *dev,
>  				      u64 addr,
>  				      size_t size,
> @@ -176,17 +165,15 @@ static void ipath_dma_free_coherent(struct ib_device *dev, size_t size,
>  }
>  
>  struct ib_dma_mapping_ops ipath_dma_mapping_ops = {
> -	ipath_mapping_error,
> -	ipath_dma_map_single,
> -	ipath_dma_unmap_single,
> -	ipath_dma_map_page,
> -	ipath_dma_unmap_page,
> -	ipath_map_sg,
> -	ipath_unmap_sg,
> -	ipath_sg_dma_address,
> -	ipath_sg_dma_len,
> -	ipath_sync_single_for_cpu,
> -	ipath_sync_single_for_device,
> -	ipath_dma_alloc_coherent,
> -	ipath_dma_free_coherent
> +	.mapping_error = ipath_mapping_error,
> +	.map_single = ipath_dma_map_single,
> +	.unmap_single = ipath_dma_unmap_single,
> +	.map_page = ipath_dma_map_page,
> +	.unmap_page = ipath_dma_unmap_page,
> +	.map_sg = ipath_map_sg,
> +	.unmap_sg = ipath_unmap_sg,
> +	.sync_single_for_cpu = ipath_sync_single_for_cpu,
> +	.sync_single_for_device = ipath_sync_single_for_device,
> +	.alloc_coherent = ipath_dma_alloc_coherent,
> +	.free_coherent = ipath_dma_free_coherent
>  };
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

RE: [PATCH v2 2/4] IB/ipath: remove ib_sg_dma_address() and ib_sg_dma_len() overloads

[Marciniszyn, Mike]

> And struct ipath_dma_mapping_ops was converted to C99 initializer.
> 

This is now mentioned in the ipath patch.

[PATCH v2 3/4] IB/ehca: remove ib_sg_dma_address() and ib_sg_dma_len() overloads

[Mike Marciniszyn]

These methods appear to only mimic the sg_dma_address()
and sg_dma_len() behavior.

They can be safely removed.

Suggested-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Hoang-Nam Nguyen <hnguyen-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
Cc: Christoph Raisch <raisch-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/hw/ehca/ehca_mrmw.c |   12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/infiniband/hw/ehca/ehca_mrmw.c b/drivers/infiniband/hw/ehca/ehca_mrmw.c
index bcfb0c1..65873ee 100644
--- a/drivers/infiniband/hw/ehca/ehca_mrmw.c
+++ b/drivers/infiniband/hw/ehca/ehca_mrmw.c
@@ -2591,16 +2591,6 @@ static void ehca_dma_unmap_sg(struct ib_device *dev, struct scatterlist *sg,
 	/* This is only a stub; nothing to be done here */
 }
 
-static u64 ehca_dma_address(struct ib_device *dev, struct scatterlist *sg)
-{
-	return sg->dma_address;
-}
-
-static unsigned int ehca_dma_len(struct ib_device *dev, struct scatterlist *sg)
-{
-	return sg->length;
-}
-
 static void ehca_dma_sync_single_for_cpu(struct ib_device *dev, u64 addr,
 					 size_t size,
 					 enum dma_data_direction dir)
@@ -2653,8 +2643,6 @@ struct ib_dma_mapping_ops ehca_dma_mapping_ops = {
 	.unmap_page             = ehca_dma_unmap_page,
 	.map_sg                 = ehca_dma_map_sg,
 	.unmap_sg               = ehca_dma_unmap_sg,
-	.dma_address            = ehca_dma_address,
-	.dma_len                = ehca_dma_len,
 	.sync_single_for_cpu    = ehca_dma_sync_single_for_cpu,
 	.sync_single_for_device = ehca_dma_sync_single_for_device,
 	.alloc_coherent         = ehca_dma_alloc_coherent,

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

[PATCH v2 4/4] IB/core: Remove overload in ib_sg_dma*

[Mike Marciniszyn]

The code is replaced by driver specific
changes and avoids the pointer NULL test
for drivers that don't overload these
operations.

Suggested-by: <Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Tested-by: Vinod Kumar <vinod.kumar-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/rdma/ib_verbs.h |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 6793f32..5777716 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1266,10 +1266,6 @@ struct ib_dma_mapping_ops {
 	void		(*unmap_sg)(struct ib_device *dev,
 				    struct scatterlist *sg, int nents,
 				    enum dma_data_direction direction);
-	u64		(*dma_address)(struct ib_device *dev,
-				       struct scatterlist *sg);
-	unsigned int	(*dma_len)(struct ib_device *dev,
-				   struct scatterlist *sg);
 	void		(*sync_single_for_cpu)(struct ib_device *dev,
 					       u64 dma_handle,
 					       size_t size,
@@ -2089,12 +2085,13 @@ static inline void ib_dma_unmap_sg_attrs(struct ib_device *dev,
  * ib_sg_dma_address - Return the DMA address from a scatter/gather entry
  * @dev: The device for which the DMA addresses were created
  * @sg: The scatter/gather entry
+ *
+ * Note: this function is obsolete. To do: change all occurrences of
+ * ib_sg_dma_address() into sg_dma_address().
  */
 static inline u64 ib_sg_dma_address(struct ib_device *dev,
 				    struct scatterlist *sg)
 {
-	if (dev->dma_ops)
-		return dev->dma_ops->dma_address(dev, sg);
 	return sg_dma_address(sg);
 }
 
@@ -2102,12 +2099,13 @@ static inline u64 ib_sg_dma_address(struct ib_device *dev,
  * ib_sg_dma_len - Return the DMA length from a scatter/gather entry
  * @dev: The device for which the DMA addresses were created
  * @sg: The scatter/gather entry
+ *
+ * Note: this function is obsolete. To do: change all occurrences of
+ * ib_sg_dma_len() into sg_dma_len().
  */
 static inline unsigned int ib_sg_dma_len(struct ib_device *dev,
 					 struct scatterlist *sg)
 {
-	if (dev->dma_ops)
-		return dev->dma_ops->dma_len(dev, sg);
 	return sg_dma_len(sg);
 }
 

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

Re: [PATCH v2 0/4] Series short description

[Yann Droneaud]

Le vendredi 28 mars 2014 à 13:26 -0400, Mike Marciniszyn a écrit :
> This patch series modified the ib_sg_dma API to
> eliminate the .dma_len and .dma_address methods.
> 
> In all present cases that overload these methods
> (ipath, qib, ehca), the lack of these methods are
> compensated for by code changes to the driver .map_sg
> to insure that the vanilla sg_dma_address() and
> sg_dma_len() will do the same thing as the equivalent
> former ib_sg_dma_address() and ib_sg_dma_len() calls
> into the drivers.
> 
> This patch series is a followup to this recent submission
> http://marc.info/?l=linux-rdma&m=139602422108727&w=2
> and Bart's similar comment in
> http://marc.info/?l=linux-netdev&m=135643746610259&w=2.
> 
> Roland, I'm not sure of the history of these methods and
> what rationale caused them to be added?
> 
> There is obviously an interim step that could be done
> here to preseve the overload by not changing verbs, yet
> change qib/ipath/echa to not overload them.  This would
> allow the drivers to work even when the "correct" API's
> had not been used by the ULP.
> 
> This version reorders the patch series order based
> on comments from Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> so that the overload removal is last.
> 
> The commit messages are enhanced a bit as well.
> 

Thanks, that makes more sense now.

Reviewed-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

Regards

-- 
Yann Droneaud
OPTEYA


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