All of lore.kernel.org
 help / color / mirror / Atom feed
From: robin.murphy@arm.com (Robin Murphy)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] scatterlist: use sg_dma_len() in sg_set_page()
Date: Mon, 11 Apr 2016 11:31:04 +0100	[thread overview]
Message-ID: <570B7CE8.3020307@arm.com> (raw)
In-Reply-To: <1460342864-27891-1-git-send-email-shawnguo@kernel.org>

Hi Shawn,

On 11/04/16 03:47, Shawn Guo wrote:
> The macro sg_dma_len(sg) is commonly used to retrieve length of sg,
> which could be 'dma_length' or 'length' field, depending on whether
> NEED_SG_DMA_LENGTH is enabled or not.  On the other hand, many driver
> code use helper function sg_set_page() to set an sg entry pointing at
> a page, with offset and length set up in one call.  But sg_set_page()
> does not consider NEED_SG_DMA_LENGTH case and only set up 'length'
> field.  This causes problem on platforms like ARM64, where
> NEED_SG_DMA_LENGTH is enabled by default, i.e. sg_set_page() sets up
> 'length' while sg_dma_len(sg) returns 'dma_length' field.
>
> The patch changes sg_set_page() to use sg_dma_len() for sg length setup
> as well, so that NEED_SG_DMA_LENGTH case can be handled.
>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> ---
>   include/linux/scatterlist.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 556ec1ea2574..b0e32ea594c3 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -114,7 +114,7 @@ static inline void sg_set_page(struct scatterlist *sg, struct page *page,
>   {
>   	sg_assign_page(sg, page);
>   	sg->offset = offset;
> -	sg->length = len;
> +	sg_dma_len(sg) = len;

This looks wrong. If a driver is building a scatterlist, then it needs 
to fill in the page, offset and length fields to describe the physical 
layout - leaving sg->length uninitialised would be a recipe for disaster 
- then pass it to the DMA API. Only the DMA API implementation should be 
setting dma_addr and dma_len, and they may not correspond to the 
physical layout at all (e.g. an IOMMU could concatenate the entire list 
into a single much longer segment at whatever arbitrary DMA address it 
chooses).

Is there some particular driver hiccup behind this?

Robin.

>   }
>
>   static inline struct page *sg_page(struct scatterlist *sg)
>

WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com>
To: Shawn Guo <shawnguo@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH] scatterlist: use sg_dma_len() in sg_set_page()
Date: Mon, 11 Apr 2016 11:31:04 +0100	[thread overview]
Message-ID: <570B7CE8.3020307@arm.com> (raw)
In-Reply-To: <1460342864-27891-1-git-send-email-shawnguo@kernel.org>

Hi Shawn,

On 11/04/16 03:47, Shawn Guo wrote:
> The macro sg_dma_len(sg) is commonly used to retrieve length of sg,
> which could be 'dma_length' or 'length' field, depending on whether
> NEED_SG_DMA_LENGTH is enabled or not.  On the other hand, many driver
> code use helper function sg_set_page() to set an sg entry pointing at
> a page, with offset and length set up in one call.  But sg_set_page()
> does not consider NEED_SG_DMA_LENGTH case and only set up 'length'
> field.  This causes problem on platforms like ARM64, where
> NEED_SG_DMA_LENGTH is enabled by default, i.e. sg_set_page() sets up
> 'length' while sg_dma_len(sg) returns 'dma_length' field.
>
> The patch changes sg_set_page() to use sg_dma_len() for sg length setup
> as well, so that NEED_SG_DMA_LENGTH case can be handled.
>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> ---
>   include/linux/scatterlist.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 556ec1ea2574..b0e32ea594c3 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -114,7 +114,7 @@ static inline void sg_set_page(struct scatterlist *sg, struct page *page,
>   {
>   	sg_assign_page(sg, page);
>   	sg->offset = offset;
> -	sg->length = len;
> +	sg_dma_len(sg) = len;

This looks wrong. If a driver is building a scatterlist, then it needs 
to fill in the page, offset and length fields to describe the physical 
layout - leaving sg->length uninitialised would be a recipe for disaster 
- then pass it to the DMA API. Only the DMA API implementation should be 
setting dma_addr and dma_len, and they may not correspond to the 
physical layout at all (e.g. an IOMMU could concatenate the entire list 
into a single much longer segment at whatever arbitrary DMA address it 
chooses).

Is there some particular driver hiccup behind this?

Robin.

>   }
>
>   static inline struct page *sg_page(struct scatterlist *sg)
>

  reply	other threads:[~2016-04-11 10:31 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-11  2:47 [PATCH] scatterlist: use sg_dma_len() in sg_set_page() Shawn Guo
2016-04-11  2:47 ` Shawn Guo
2016-04-11 10:31 ` Robin Murphy [this message]
2016-04-11 10:31   ` Robin Murphy
2016-04-11 12:31   ` Shawn Guo
2016-04-11 12:31     ` Shawn Guo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=570B7CE8.3020307@arm.com \
    --to=robin.murphy@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.