diff mbox series

[v4,5/5] dma-buf: Clarify that dma-buf sg lists are page aligned

Message ID 1602692161-107096-1-git-send-email-jianxin.xiong@intel.com (mailing list archive)
State New, archived
Headers show
Series RDMA: Add dma-buf support | expand

Commit Message

Xiong, Jianxin Oct. 14, 2020, 4:16 p.m. UTC
The dma-buf API have been used under the assumption that the sg lists
returned from dma_buf_map_attachment() are fully page aligned. Lots of
stuff can break otherwise all over the place. Clarify this in the
documentation and add a check when DMA API debug is enabled.

Signed-off-by: Jianxin Xiong <jianxin.xiong@intel.com>
---
 drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++++++
 include/linux/dma-buf.h   |  3 ++-
 2 files changed, 23 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Oct. 15, 2020, 4:03 p.m. UTC | #1
On Wed, Oct 14, 2020 at 09:16:01AM -0700, Jianxin Xiong wrote:
> The dma-buf API have been used under the assumption that the sg lists
> returned from dma_buf_map_attachment() are fully page aligned. Lots of
> stuff can break otherwise all over the place. Clarify this in the
> documentation and add a check when DMA API debug is enabled.
> 
> Signed-off-by: Jianxin Xiong <jianxin.xiong@intel.com>

lgtm, thanks for creating this and giving it a spin.

I'll queue this up in drm-misc-next for 5.11, should show up in linux-next
after the merge window is closed.

Cheers, Daniel

> ---
>  drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++++++
>  include/linux/dma-buf.h   |  3 ++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 844967f..7309c83 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -851,6 +851,9 @@ void dma_buf_unpin(struct dma_buf_attachment *attach)
>   * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
>   * on error. May return -EINTR if it is interrupted by a signal.
>   *
> + * On success, the DMA addresses and lengths in the returned scatterlist are
> + * PAGE_SIZE aligned.
> + *
>   * A mapping must be unmapped by using dma_buf_unmap_attachment(). Note that
>   * the underlying backing storage is pinned for as long as a mapping exists,
>   * therefore users/importers should not hold onto a mapping for undue amounts of
> @@ -904,6 +907,24 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  		attach->dir = direction;
>  	}
>  
> +#ifdef CONFIG_DMA_API_DEBUG
> +	{
> +		struct scatterlist *sg;
> +		u64 addr;
> +		int len;
> +		int i;
> +
> +		for_each_sgtable_dma_sg(sg_table, sg, i) {
> +			addr = sg_dma_address(sg);
> +			len = sg_dma_len(sg);
> +			if (!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(len)) {
> +				pr_debug("%s: addr %llx or len %x is not page aligned!\n",
> +					 __func__, addr, len);
> +			}
> +		}
> +	}
> +#endif /* CONFIG_DMA_API_DEBUG */
> +
>  	return sg_table;
>  }
>  EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index a2ca294e..4a5fa70 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -145,7 +145,8 @@ struct dma_buf_ops {
>  	 *
>  	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
>  	 * already mapped into the device address space of the &device attached
> -	 * with the provided &dma_buf_attachment.
> +	 * with the provided &dma_buf_attachment. The addresses and lengths in
> +	 * the scatter list are PAGE_SIZE aligned.
>  	 *
>  	 * On failure, returns a negative error value wrapped into a pointer.
>  	 * May also return -EINTR when a signal was received while being
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Christian König Oct. 15, 2020, 4:31 p.m. UTC | #2
Am 15.10.20 um 18:03 schrieb Daniel Vetter:
> On Wed, Oct 14, 2020 at 09:16:01AM -0700, Jianxin Xiong wrote:
>> The dma-buf API have been used under the assumption that the sg lists
>> returned from dma_buf_map_attachment() are fully page aligned. Lots of
>> stuff can break otherwise all over the place. Clarify this in the
>> documentation and add a check when DMA API debug is enabled.
>>
>> Signed-off-by: Jianxin Xiong <jianxin.xiong@intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> lgtm, thanks for creating this and giving it a spin.
>
> I'll queue this up in drm-misc-next for 5.11, should show up in linux-next
> after the merge window is closed.

Thanks, I'm currently without landline internet and need to rely on my 
mobile.

Christian.

>
> Cheers, Daniel
>
>> ---
>>   drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++++++
>>   include/linux/dma-buf.h   |  3 ++-
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 844967f..7309c83 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -851,6 +851,9 @@ void dma_buf_unpin(struct dma_buf_attachment *attach)
>>    * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
>>    * on error. May return -EINTR if it is interrupted by a signal.
>>    *
>> + * On success, the DMA addresses and lengths in the returned scatterlist are
>> + * PAGE_SIZE aligned.
>> + *
>>    * A mapping must be unmapped by using dma_buf_unmap_attachment(). Note that
>>    * the underlying backing storage is pinned for as long as a mapping exists,
>>    * therefore users/importers should not hold onto a mapping for undue amounts of
>> @@ -904,6 +907,24 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>>   		attach->dir = direction;
>>   	}
>>   
>> +#ifdef CONFIG_DMA_API_DEBUG
>> +	{
>> +		struct scatterlist *sg;
>> +		u64 addr;
>> +		int len;
>> +		int i;
>> +
>> +		for_each_sgtable_dma_sg(sg_table, sg, i) {
>> +			addr = sg_dma_address(sg);
>> +			len = sg_dma_len(sg);
>> +			if (!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(len)) {
>> +				pr_debug("%s: addr %llx or len %x is not page aligned!\n",
>> +					 __func__, addr, len);
>> +			}
>> +		}
>> +	}
>> +#endif /* CONFIG_DMA_API_DEBUG */
>> +
>>   	return sg_table;
>>   }
>>   EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index a2ca294e..4a5fa70 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -145,7 +145,8 @@ struct dma_buf_ops {
>>   	 *
>>   	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
>>   	 * already mapped into the device address space of the &device attached
>> -	 * with the provided &dma_buf_attachment.
>> +	 * with the provided &dma_buf_attachment. The addresses and lengths in
>> +	 * the scatter list are PAGE_SIZE aligned.
>>   	 *
>>   	 * On failure, returns a negative error value wrapped into a pointer.
>>   	 * May also return -EINTR when a signal was received while being
>> -- 
>> 1.8.3.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C58ee8712041e4e742fe408d87123e970%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637383746351346603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=EwAEVcJa6gboHMEQ6XUymC%2BtjFoWd0wl8YUyzdnV5N8%3D&amp;reserved=0
diff mbox series

Patch

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 844967f..7309c83 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -851,6 +851,9 @@  void dma_buf_unpin(struct dma_buf_attachment *attach)
  * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
  * on error. May return -EINTR if it is interrupted by a signal.
  *
+ * On success, the DMA addresses and lengths in the returned scatterlist are
+ * PAGE_SIZE aligned.
+ *
  * A mapping must be unmapped by using dma_buf_unmap_attachment(). Note that
  * the underlying backing storage is pinned for as long as a mapping exists,
  * therefore users/importers should not hold onto a mapping for undue amounts of
@@ -904,6 +907,24 @@  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 		attach->dir = direction;
 	}
 
+#ifdef CONFIG_DMA_API_DEBUG
+	{
+		struct scatterlist *sg;
+		u64 addr;
+		int len;
+		int i;
+
+		for_each_sgtable_dma_sg(sg_table, sg, i) {
+			addr = sg_dma_address(sg);
+			len = sg_dma_len(sg);
+			if (!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(len)) {
+				pr_debug("%s: addr %llx or len %x is not page aligned!\n",
+					 __func__, addr, len);
+			}
+		}
+	}
+#endif /* CONFIG_DMA_API_DEBUG */
+
 	return sg_table;
 }
 EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index a2ca294e..4a5fa70 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -145,7 +145,8 @@  struct dma_buf_ops {
 	 *
 	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
 	 * already mapped into the device address space of the &device attached
-	 * with the provided &dma_buf_attachment.
+	 * with the provided &dma_buf_attachment. The addresses and lengths in
+	 * the scatter list are PAGE_SIZE aligned.
 	 *
 	 * On failure, returns a negative error value wrapped into a pointer.
 	 * May also return -EINTR when a signal was received while being