[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Subhash Jadavani]

blk_rq_map_sg() function merges the physically contiguous pages to use same
scatter-gather node without checking if their page descriptors are
contiguous or not.

Now when dma_map_sg() is called on the scatter gather list, it would
take the base page pointer from each node (one by one) and iterates
through all of the pages in same sg node by keep incrementing the base
page pointer with the assumption that physically contiguous pages will
have their page descriptor address contiguous which may not be true
if SPARSEMEM config is enabled. So here we may end referring to invalid
page descriptor.

Following table shows the example of physically contiguous pages but
their page descriptor addresses non-contiguous.
-------------------------------------------
| Page Descriptor    |   Physical Address |
------------------------------------------
| 0xc1e43fdc         |   0xdffff000       |
| 0xc2052000         |   0xe0000000       |
-------------------------------------------

With this patch, relevant blk-merge functions will also check if the
physically contiguous pages are having page descriptors address contiguous
or not? If not then, these pages are separated to be in different
scatter-gather nodes.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
---
 block/blk-merge.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 936a110..6eaef3d4 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -42,6 +42,9 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 					goto new_segment;
 				if (!BIOVEC_SEG_BOUNDARY(q, bvprv, bv))
 					goto new_segment;
+				if ((bvprv->bv_page != bv->bv_page) &&
+				    (bvprv->bv_page + 1) != bv->bv_page)
+					goto new_segment;
 
 				seg_size += bv->bv_len;
 				bvprv = bv;
@@ -126,6 +129,9 @@ __blk_segment_map_sg(struct request_queue *q, struct bio_vec *bvec,
 			goto new_segment;
 		if (!BIOVEC_SEG_BOUNDARY(q, *bvprv, bvec))
 			goto new_segment;
+		if (((*bvprv)->bv_page != bvec->bv_page) &&
+		    (((*bvprv)->bv_page + 1) != bvec->bv_page))
+			goto new_segment;
 
 		(*sg)->length += nbytes;
 	} else {
-- 
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Tue, 2013-01-15 at 21:31 +0530, Subhash Jadavani wrote:
> blk_rq_map_sg() function merges the physically contiguous pages to use same
> scatter-gather node without checking if their page descriptors are
> contiguous or not.
> 
> Now when dma_map_sg() is called on the scatter gather list, it would
> take the base page pointer from each node (one by one) and iterates
> through all of the pages in same sg node by keep incrementing the base
> page pointer with the assumption that physically contiguous pages will
> have their page descriptor address contiguous which may not be true
> if SPARSEMEM config is enabled. So here we may end referring to invalid
> page descriptor.
> 
> Following table shows the example of physically contiguous pages but
> their page descriptor addresses non-contiguous.
> -------------------------------------------
> | Page Descriptor    |   Physical Address |
> ------------------------------------------
> | 0xc1e43fdc         |   0xdffff000       |
> | 0xc2052000         |   0xe0000000       |
> -------------------------------------------
> 
> With this patch, relevant blk-merge functions will also check if the
> physically contiguous pages are having page descriptors address contiguous
> or not? If not then, these pages are separated to be in different
> scatter-gather nodes.

How does this manifest as a bug?

The discontinuity is in struct page arrays, which hardware doesn't care
about.  All we need is to get from struct page to the physical address
for programming the hardware, for which we use the sg_phys() inline
function.

Even given we have a two page physical contiguity at 0xdffff000 in your
example, the sg list entry contains a length of 8192 and a page_link of
0xc1e43fdc, which we transform to the correct physical address and
length.

James

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Subhash Jadavani]

On 1/15/2013 9:49 PM, James Bottomley wrote:
> On Tue, 2013-01-15 at 21:31 +0530, Subhash Jadavani wrote:
>> blk_rq_map_sg() function merges the physically contiguous pages to use same
>> scatter-gather node without checking if their page descriptors are
>> contiguous or not.
>>
>> Now when dma_map_sg() is called on the scatter gather list, it would
>> take the base page pointer from each node (one by one) and iterates
>> through all of the pages in same sg node by keep incrementing the base
>> page pointer with the assumption that physically contiguous pages will
>> have their page descriptor address contiguous which may not be true
>> if SPARSEMEM config is enabled. So here we may end referring to invalid
>> page descriptor.
>>
>> Following table shows the example of physically contiguous pages but
>> their page descriptor addresses non-contiguous.
>> -------------------------------------------
>> | Page Descriptor    |   Physical Address |
>> ------------------------------------------
>> | 0xc1e43fdc         |   0xdffff000       |
>> | 0xc2052000         |   0xe0000000       |
>> -------------------------------------------
>>
>> With this patch, relevant blk-merge functions will also check if the
>> physically contiguous pages are having page descriptors address contiguous
>> or not? If not then, these pages are separated to be in different
>> scatter-gather nodes.
> How does this manifest as a bug?
>
> The discontinuity is in struct page arrays, which hardware doesn't care
> about.  All we need is to get from struct page to the physical address
> for programming the hardware, for which we use the sg_phys() inline
> function.
>
> Even given we have a two page physical contiguity at 0xdffff000 in your
> example, the sg list entry contains a length of 8192 and a page_link of
> 0xc1e43fdc, which we transform to the correct physical address and
> length.
Thanks James for looking at this patch.

Let's assume this scenario.
PAGE_SIZE = 4096 (4K),
2 page descriptors (as mentioned commit text) whose own addresses are 
discontingous but they point to physically contiguous memory space, 
sizeof(struct page) is 36 bytes.
Only one SG node created in Scatter Gather list (by blk_rq_map_sg) with 
this node's page_link=0xc1e43fdc, length=8192, offset=0

Now consider this call stack from MMC block driver (this is on the ARmv7 
based board):
     [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
[<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
     [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
[<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
     [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
[<c0017ff8>] (dma_map_sg+0x3c/0x114)
     [   98.947169] [<c0017ff8>] (dma_map_sg+0x3c/0x114) from 
[<c0532c40>] (msmsdcc_prep_xfer+0x50/0x10c)
     [   98.956020] [<c0532c40>] (msmsdcc_prep_xfer+0x50/0x10c) from 
[<c0539664>] (msmsdcc_pre_req+0x78/0x98)
     [   98.965237] [<c0539664>] (msmsdcc_pre_req+0x78/0x98) from 
[<c0521d98>] (mmc_start_req+0x4c/0x1c4)
     [   98.974088] [<c0521d98>] (mmc_start_req+0x4c/0x1c4) from 
[<c052faa0>] (mmc_blk_issue_rw_rq+0x3a0/0x84c)
     [   98.983457] [<c052faa0>] (mmc_blk_issue_rw_rq+0x3a0/0x84c) from 
[<c05304b4>] (mmc_blk_issue_rq+0x568/0x5c4)
     [   98.993193] [<c05304b4>] (mmc_blk_issue_rq+0x568/0x5c4) from 
[<c0530718>] (mmc_queue_thread+0xb4/0x120)
     [   99.002563] [<c0530718>] (mmc_queue_thread+0xb4/0x120) from 
[<c0096400>] (kthread+0x80/0x8c)
     [   99.010987] [<c0096400>] (kthread+0x80/0x8c) from [<c000f028>] 
(kernel_thread_exit+0x0/0x8)

dma_cache_maint_page() function iterates through each page in single sg 
node, maps it to virtual space and then call the cache maintainance 
operation on that page.

With above scenario, first page descriptor would be 0xc1e43fdc, which 
would be mapped virtual address by either kmap() (if it's higmem page) 
or by page_address() (if it's low mem page).

Now as the size of the sg node is 8192 bytes, dma_cache_maint_page() 
function increments the page pointer (pointed by page_link of sg node) 
to get to the next descriptor. page++ would yield page descriptor 
address = 0xc1e44000 (0xc1e43fdc + sizeof (struct page)). 0xc1e44000 is 
not pointing any real page descriptor so when calls 
page_address(0xc1e44000) to get virtual address of the page, it returns 
invalid virtual address. Now when we try to dereference that invalid 
virtual address in cache maintainance functions, it would result into 
"Unable to handle kernel paging request at virtual address xxxxxxxx" 
error and kernel crashes.

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

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
> On 1/15/2013 9:49 PM, James Bottomley wrote:
> > On Tue, 2013-01-15 at 21:31 +0530, Subhash Jadavani wrote:
> >> blk_rq_map_sg() function merges the physically contiguous pages to use same
> >> scatter-gather node without checking if their page descriptors are
> >> contiguous or not.
> >>
> >> Now when dma_map_sg() is called on the scatter gather list, it would
> >> take the base page pointer from each node (one by one) and iterates
> >> through all of the pages in same sg node by keep incrementing the base
> >> page pointer with the assumption that physically contiguous pages will
> >> have their page descriptor address contiguous which may not be true
> >> if SPARSEMEM config is enabled. So here we may end referring to invalid
> >> page descriptor.
> >>
> >> Following table shows the example of physically contiguous pages but
> >> their page descriptor addresses non-contiguous.
> >> -------------------------------------------
> >> | Page Descriptor    |   Physical Address |
> >> ------------------------------------------
> >> | 0xc1e43fdc         |   0xdffff000       |
> >> | 0xc2052000         |   0xe0000000       |
> >> -------------------------------------------
> >>
> >> With this patch, relevant blk-merge functions will also check if the
> >> physically contiguous pages are having page descriptors address contiguous
> >> or not? If not then, these pages are separated to be in different
> >> scatter-gather nodes.
> > How does this manifest as a bug?
> >
> > The discontinuity is in struct page arrays, which hardware doesn't care
> > about.  All we need is to get from struct page to the physical address
> > for programming the hardware, for which we use the sg_phys() inline
> > function.
> >
> > Even given we have a two page physical contiguity at 0xdffff000 in your
> > example, the sg list entry contains a length of 8192 and a page_link of
> > 0xc1e43fdc, which we transform to the correct physical address and
> > length.
> Thanks James for looking at this patch.
> 
> Let's assume this scenario.
> PAGE_SIZE = 4096 (4K),
> 2 page descriptors (as mentioned commit text) whose own addresses are 
> discontingous but they point to physically contiguous memory space, 
> sizeof(struct page) is 36 bytes.
> Only one SG node created in Scatter Gather list (by blk_rq_map_sg) with 
> this node's page_link=0xc1e43fdc, length=8192, offset=0
> 
> Now consider this call stack from MMC block driver (this is on the ARmv7 
> based board):
>      [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
> [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
>      [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
> [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
>      [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
> [<c0017ff8>] (dma_map_sg+0x3c/0x114)
>      [   98.947169] [<c0017ff8>] (dma_map_sg+0x3c/0x114) from 
> [<c0532c40>] (msmsdcc_prep_xfer+0x50/0x10c)
>      [   98.956020] [<c0532c40>] (msmsdcc_prep_xfer+0x50/0x10c) from 
> [<c0539664>] (msmsdcc_pre_req+0x78/0x98)
>      [   98.965237] [<c0539664>] (msmsdcc_pre_req+0x78/0x98) from 
> [<c0521d98>] (mmc_start_req+0x4c/0x1c4)
>      [   98.974088] [<c0521d98>] (mmc_start_req+0x4c/0x1c4) from 
> [<c052faa0>] (mmc_blk_issue_rw_rq+0x3a0/0x84c)
>      [   98.983457] [<c052faa0>] (mmc_blk_issue_rw_rq+0x3a0/0x84c) from 
> [<c05304b4>] (mmc_blk_issue_rq+0x568/0x5c4)
>      [   98.993193] [<c05304b4>] (mmc_blk_issue_rq+0x568/0x5c4) from 
> [<c0530718>] (mmc_queue_thread+0xb4/0x120)
>      [   99.002563] [<c0530718>] (mmc_queue_thread+0xb4/0x120) from 
> [<c0096400>] (kthread+0x80/0x8c)
>      [   99.010987] [<c0096400>] (kthread+0x80/0x8c) from [<c000f028>] 
> (kernel_thread_exit+0x0/0x8)
> 
> dma_cache_maint_page() function iterates through each page in single sg 
> node, maps it to virtual space and then call the cache maintainance 
> operation on that page.
> 
> With above scenario, first page descriptor would be 0xc1e43fdc, which 
> would be mapped virtual address by either kmap() (if it's higmem page) 
> or by page_address() (if it's low mem page).
> 
> Now as the size of the sg node is 8192 bytes, dma_cache_maint_page() 
> function increments the page pointer (pointed by page_link of sg node) 
> to get to the next descriptor. page++ would yield page descriptor 
> address = 0xc1e44000 (0xc1e43fdc + sizeof (struct page)). 0xc1e44000 is 
> not pointing any real page descriptor so when calls 
> page_address(0xc1e44000) to get virtual address of the page, it returns 
> invalid virtual address. Now when we try to dereference that invalid 
> virtual address in cache maintainance functions, it would result into 
> "Unable to handle kernel paging request at virtual address xxxxxxxx" 
> error and kernel crashes.

Well, that means the fault is in the iterator, doesn't it?  It's
assuming two pages which are physically contiguous have adjacent entries
in the page arrays.  If you want to fix that fault, fix it in the
iterators ... probably they should use the correct page to pfn
transforms which will move across the discontiguous arrays.

More simply, though, if the MMC drivers want to iterate through all the
pages in the array, then surely you don't want any physical merging
anyway, so your quick fix is to turn off clustering:

		q->limits.cluster = 0;

in the mmc layer?

James

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:

> Now consider this call stack from MMC block driver (this is on the ARmv7 
> based board):
>      [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
> [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
>      [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
> [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
>      [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
> [<c0017ff8>] (dma_map_sg+0x3c/0x114)

OK, so this is showing that ARM itself is making the assumption that the
pages are contiguous in the page offset map.

Fix this by doing the increment via the pfn, which will do the right
thing whatever the memory model.

Signed-off-by: James Bottomley <JBottomley@Parallels.com>

---

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 6b2fb87..ab88c5b 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
 			op(vaddr, len, dir);
 		}
 		offset = 0;
-		page++;
+		page = pfn_to_page(page_to_pfn(page) + 1);
 		left -= len;
 	} while (left);
 }

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:

> Now consider this call stack from MMC block driver (this is on the ARmv7 
> based board):
>      [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
> [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
>      [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
> [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
>      [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
> [<c0017ff8>] (dma_map_sg+0x3c/0x114)

OK, so this is showing that ARM itself is making the assumption that the
pages are contiguous in the page offset map.

Fix this by doing the increment via the pfn, which will do the right
thing whatever the memory model.

Signed-off-by: James Bottomley <JBottomley@Parallels.com>

---

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 6b2fb87..ab88c5b 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
 			op(vaddr, len, dir);
 		}
 		offset = 0;
-		page++;
+		page = pfn_to_page(page_to_pfn(page) + 1);
 		left -= len;
 	} while (left);
 }

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Subhash Jadavani]

On 1/16/2013 4:02 PM, James Bottomley wrote:
> On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
>
>> Now consider this call stack from MMC block driver (this is on the ARmv7
>> based board):
>>       [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from
>> [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
>>       [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from
>> [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
>>       [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from
>> [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> OK, so this is showing that ARM itself is making the assumption that the
> pages are contiguous in the page offset map.
>
> Fix this by doing the increment via the pfn, which will do the right
> thing whatever the memory model.
>
> Signed-off-by: James Bottomley <JBottomley@Parallels.com>

Thanks James. Yes, it make sense to fix the ARM code itself if it is the 
only one giving this trouble.
I have tried your change below and it also fixes this issue (without 
having my blk-merge patch). I will forward your change to Russel King to 
see what he thinks about it.

Regards,
Subhash
>
> ---
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   			op(vaddr, len, dir);
>   		}
>   		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);
>   		left -= len;
>   	} while (left);
>   }
>
>
> --
> 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

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Subhash Jadavani]

On 1/16/2013 4:02 PM, James Bottomley wrote:
> On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
>
>> Now consider this call stack from MMC block driver (this is on the ARmv7
>> based board):
>>       [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from
>> [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
>>       [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from
>> [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
>>       [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from
>> [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> OK, so this is showing that ARM itself is making the assumption that the
> pages are contiguous in the page offset map.
>
> Fix this by doing the increment via the pfn, which will do the right
> thing whatever the memory model.
>
> Signed-off-by: James Bottomley <JBottomley@Parallels.com>

Thanks James. Yes, it make sense to fix the ARM code itself if it is the 
only one giving this trouble.
I have tried your change below and it also fixes this issue (without 
having my blk-merge patch). I will forward your change to Russel King to 
see what he thinks about it.

Regards,
Subhash
>
> ---
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   			op(vaddr, len, dir);
>   		}
>   		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);
>   		left -= len;
>   	} while (left);
>   }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

ARM DMA: Fix in dma_cache_maint_page

[Subhash Jadavani]

[-- Attachment #1.1: Type: text/plain, Size: 2745 bytes --]


Hi Russell,

Is it possible to pick up James patch below? Thread here: 
http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
on the motivation behind this fix.

Regards,
Subhash

-------- Original Message --------
Subject: 	Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages 
with non-contiguous descriptors
Date: 	Wed, 16 Jan 2013 18:09:14 +0530
From: 	Subhash Jadavani <subhashj@codeaurora.org>
To: 	James Bottomley <James.Bottomley@HansenPartnership.com>
CC: 	linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, 
linux-mmc@vger.kernel.org, linux-arm-msm@vger.kernel.org, 
martin.petersen@oracle.com, asias@redhat.com, tj@kernel.org, 
linux-arm-kernel@lists.infradead.org, Russell King <linux@arm.linux.org.uk>



On 1/16/2013 4:02 PM, James Bottomley wrote:
> On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
>
>> Now consider this call stack from MMC block driver (this is on the ARmv7
>> based board):
>>       [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from
>> [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
>>       [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from
>> [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
>>       [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from
>> [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> OK, so this is showing that ARM itself is making the assumption that the
> pages are contiguous in the page offset map.
>
> Fix this by doing the increment via the pfn, which will do the right
> thing whatever the memory model.
>
> Signed-off-by: James Bottomley <JBottomley@Parallels.com>

Thanks James. Yes, it make sense to fix the ARM code itself if it is the
only one giving this trouble.
I have tried your change below and it also fixes this issue (without
having my blk-merge patch). I will forward your change to Russel King to
see what he thinks about it.

Regards,
Subhash
>
> ---
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   			op(vaddr, len, dir);
>   		}
>   		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);
>   		left -= len;
>   	} while (left);
>   }
>
>
> --
> 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

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




[-- Attachment #1.2: Type: text/html, Size: 5532 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

ARM DMA: Fix in dma_cache_maint_page

[Subhash Jadavani]

Hi Russell,

Is it possible to pick up James patch below? Thread here: 
http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
on the motivation behind this fix.

Regards,
Subhash

-------- Original Message --------
Subject: 	Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages 
with non-contiguous descriptors
Date: 	Wed, 16 Jan 2013 18:09:14 +0530
From: 	Subhash Jadavani <subhashj@codeaurora.org>
To: 	James Bottomley <James.Bottomley@HansenPartnership.com>
CC: 	linux-kernel at vger.kernel.org, linux-scsi at vger.kernel.org, 
linux-mmc at vger.kernel.org, linux-arm-msm at vger.kernel.org, 
martin.petersen at oracle.com, asias at redhat.com, tj at kernel.org, 
linux-arm-kernel at lists.infradead.org, Russell King <linux@arm.linux.org.uk>



On 1/16/2013 4:02 PM, James Bottomley wrote:
> On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
>
>> Now consider this call stack from MMC block driver (this is on the ARmv7
>> based board):
>>       [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from
>> [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
>>       [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from
>> [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
>>       [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from
>> [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> OK, so this is showing that ARM itself is making the assumption that the
> pages are contiguous in the page offset map.
>
> Fix this by doing the increment via the pfn, which will do the right
> thing whatever the memory model.
>
> Signed-off-by: James Bottomley <JBottomley@Parallels.com>

Thanks James. Yes, it make sense to fix the ARM code itself if it is the
only one giving this trouble.
I have tried your change below and it also fixes this issue (without
having my blk-merge patch). I will forward your change to Russel King to
see what he thinks about it.

Regards,
Subhash
>
> ---
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   			op(vaddr, len, dir);
>   		}
>   		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);
>   		left -= len;
>   	} while (left);
>   }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130116/bcc1625a/attachment-0001.html>

Re: ARM DMA: Fix in dma_cache_maint_page

[James Bottomley]

On Wed, 2013-01-16 at 18:17 +0530, Subhash Jadavani wrote:
> Is it possible to pick up James patch below? Thread here: 
> http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
> on the motivation behind this fix.

Someone should also audit the arm kernel code for more of these linear
page array assumptions.  I'm guessing that when sparsemem was added to
arm over a year ago, whoever did it either didn't audit or missed a few.

James

ARM DMA: Fix in dma_cache_maint_page

[James Bottomley]

On Wed, 2013-01-16 at 18:17 +0530, Subhash Jadavani wrote:
> Is it possible to pick up James patch below? Thread here: 
> http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
> on the motivation behind this fix.

Someone should also audit the arm kernel code for more of these linear
page array assumptions.  I'm guessing that when sparsemem was added to
arm over a year ago, whoever did it either didn't audit or missed a few.

James

Re: ARM DMA: Fix in dma_cache_maint_page

[Russell King - ARM Linux]

On Wed, Jan 16, 2013 at 12:51:55PM +0000, James Bottomley wrote:
> On Wed, 2013-01-16 at 18:17 +0530, Subhash Jadavani wrote:
> > Is it possible to pick up James patch below? Thread here: 
> > http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
> > on the motivation behind this fix.
> 
> Someone should also audit the arm kernel code for more of these linear
> page array assumptions.  I'm guessing that when sparsemem was added to
> arm over a year ago, whoever did it either didn't audit or missed a few.

No, that's a bad assumption.  We've had discontigmem for years - maybe
something like 12 years.  I switched everything over to sparsemem, and
sparsemem has been used on ARM for years too:

	commit 05944d74bc28fffbcce159cb915d0acff82f30a1
	Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
	Date:   Thu Nov 30 20:43:51 2006 +0000

	    [ARM] Add initial sparsemem support

	    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

However, there's a big problem with this: very few of the lead people
have machines which suffer from this disability, so there's very little
testing of it - and there's very little testing of new code with it.

The patch which originally introduced this code which your patch
touches was part of adding highmem support to ARM:

	commit 43377453af83b8ff8c1c731da1508bd6b84ebfea
	Author: Nicolas Pitre <nico@cam.org>
	Date:   Thu Mar 12 22:52:09 2009 -0400

	    [ARM] introduce dma_cache_maint_page()

	    This is a helper to be used by the DMA mapping API to handle cache
	    maintenance for memory identified by a page structure instead of a
	    virtual address.  Those pages may or may not be highmem pages, and
	    when they're highmem pages, they may or may not be virtually mapped.
	    When they're not mapped then there is no L1 cache to worry about. But
	    even in that case the L2 cache must be processed since unmapped highmem
	    pages can still be L2 cached.

	    Signed-off-by: Nicolas Pitre <nico@marvell.com>

some three years later, and has been through a number of revisions since.

I'd really like to get rid of sparsemem so it's one less failure case, but
alas there's a relatively small bunch of folk who rely upon it.  That
means it's always going to be more buggy.

ARM DMA: Fix in dma_cache_maint_page

[Russell King - ARM Linux]

On Wed, Jan 16, 2013 at 12:51:55PM +0000, James Bottomley wrote:
> On Wed, 2013-01-16 at 18:17 +0530, Subhash Jadavani wrote:
> > Is it possible to pick up James patch below? Thread here: 
> > http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
> > on the motivation behind this fix.
> 
> Someone should also audit the arm kernel code for more of these linear
> page array assumptions.  I'm guessing that when sparsemem was added to
> arm over a year ago, whoever did it either didn't audit or missed a few.

No, that's a bad assumption.  We've had discontigmem for years - maybe
something like 12 years.  I switched everything over to sparsemem, and
sparsemem has been used on ARM for years too:

	commit 05944d74bc28fffbcce159cb915d0acff82f30a1
	Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
	Date:   Thu Nov 30 20:43:51 2006 +0000

	    [ARM] Add initial sparsemem support

	    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

However, there's a big problem with this: very few of the lead people
have machines which suffer from this disability, so there's very little
testing of it - and there's very little testing of new code with it.

The patch which originally introduced this code which your patch
touches was part of adding highmem support to ARM:

	commit 43377453af83b8ff8c1c731da1508bd6b84ebfea
	Author: Nicolas Pitre <nico@cam.org>
	Date:   Thu Mar 12 22:52:09 2009 -0400

	    [ARM] introduce dma_cache_maint_page()

	    This is a helper to be used by the DMA mapping API to handle cache
	    maintenance for memory identified by a page structure instead of a
	    virtual address.  Those pages may or may not be highmem pages, and
	    when they're highmem pages, they may or may not be virtually mapped.
	    When they're not mapped then there is no L1 cache to worry about. But
	    even in that case the L2 cache must be processed since unmapped highmem
	    pages can still be L2 cached.

	    Signed-off-by: Nicolas Pitre <nico@marvell.com>

some three years later, and has been through a number of revisions since.

I'd really like to get rid of sparsemem so it's one less failure case, but
alas there's a relatively small bunch of folk who rely upon it.  That
means it's always going to be more buggy.

Re: ARM DMA: Fix in dma_cache_maint_page

[James Bottomley]

On Wed, 2013-01-16 at 13:08 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 16, 2013 at 12:51:55PM +0000, James Bottomley wrote:
> > On Wed, 2013-01-16 at 18:17 +0530, Subhash Jadavani wrote:
> > > Is it possible to pick up James patch below? Thread here: 
> > > http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
> > > on the motivation behind this fix.
> > 
> > Someone should also audit the arm kernel code for more of these linear
> > page array assumptions.  I'm guessing that when sparsemem was added to
> > arm over a year ago, whoever did it either didn't audit or missed a few.
> 
> No, that's a bad assumption.  We've had discontigmem for years - maybe
> something like 12 years.

Discontigmem doesn't suffer from this particular problem: The breaks in
the page arrays in a discontigmem environment represent real breaks in
the physical memory map, so there can never arise a situation where
page++ wouldn't get you the next valid page, provided there's still a
real contiguous page of physical memory present in the system, of
course.

Sparsemem tried to be cleverer and it broke that assumption.  I'm
starting to wonder if there are other places in the kernel which make it
and get broken by sparsemem.

I have patches for parisc to switch it over to sparsemem from
discontigmem ... I haven't pushed them yet, fortunately, and I certainly
didn't check our underlying assumptions for this problem.

>   I switched everything over to sparsemem, and
> sparsemem has been used on ARM for years too:
> 
> 	commit 05944d74bc28fffbcce159cb915d0acff82f30a1
> 	Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
> 	Date:   Thu Nov 30 20:43:51 2006 +0000
> 
> 	    [ARM] Add initial sparsemem support
> 
> 	    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> However, there's a big problem with this: very few of the lead people
> have machines which suffer from this disability, so there's very little
> testing of it - and there's very little testing of new code with it.

Anyone can test this just by enabling sparsemem with an artificially
small SECTION_SIZE_BITS (that makes it much more likely you'll hit a
boundary).

> The patch which originally introduced this code which your patch
> touches was part of adding highmem support to ARM:
> 
> 	commit 43377453af83b8ff8c1c731da1508bd6b84ebfea
> 	Author: Nicolas Pitre <nico@cam.org>
> 	Date:   Thu Mar 12 22:52:09 2009 -0400
> 
> 	    [ARM] introduce dma_cache_maint_page()
> 
> 	    This is a helper to be used by the DMA mapping API to handle cache
> 	    maintenance for memory identified by a page structure instead of a
> 	    virtual address.  Those pages may or may not be highmem pages, and
> 	    when they're highmem pages, they may or may not be virtually mapped.
> 	    When they're not mapped then there is no L1 cache to worry about. But
> 	    even in that case the L2 cache must be processed since unmapped highmem
> 	    pages can still be L2 cached.
> 
> 	    Signed-off-by: Nicolas Pitre <nico@marvell.com>
> 
> some three years later, and has been through a number of revisions since.
> 
> I'd really like to get rid of sparsemem so it's one less failure case, but
> alas there's a relatively small bunch of folk who rely upon it.  That
> means it's always going to be more buggy.

Heh, I'd like to do that too on parisc; the problem is that the memory
holes in our platform are just to big to use a linear array on.  And now
that the slub people want DISCONTIGMEM deprecated, there's not much
choice.

James

ARM DMA: Fix in dma_cache_maint_page

[James Bottomley]

On Wed, 2013-01-16 at 13:08 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 16, 2013 at 12:51:55PM +0000, James Bottomley wrote:
> > On Wed, 2013-01-16 at 18:17 +0530, Subhash Jadavani wrote:
> > > Is it possible to pick up James patch below? Thread here: 
> > > http://comments.gmane.org/gmane.linux.kernel.mmc/18670, have the details 
> > > on the motivation behind this fix.
> > 
> > Someone should also audit the arm kernel code for more of these linear
> > page array assumptions.  I'm guessing that when sparsemem was added to
> > arm over a year ago, whoever did it either didn't audit or missed a few.
> 
> No, that's a bad assumption.  We've had discontigmem for years - maybe
> something like 12 years.

Discontigmem doesn't suffer from this particular problem: The breaks in
the page arrays in a discontigmem environment represent real breaks in
the physical memory map, so there can never arise a situation where
page++ wouldn't get you the next valid page, provided there's still a
real contiguous page of physical memory present in the system, of
course.

Sparsemem tried to be cleverer and it broke that assumption.  I'm
starting to wonder if there are other places in the kernel which make it
and get broken by sparsemem.

I have patches for parisc to switch it over to sparsemem from
discontigmem ... I haven't pushed them yet, fortunately, and I certainly
didn't check our underlying assumptions for this problem.

>   I switched everything over to sparsemem, and
> sparsemem has been used on ARM for years too:
> 
> 	commit 05944d74bc28fffbcce159cb915d0acff82f30a1
> 	Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
> 	Date:   Thu Nov 30 20:43:51 2006 +0000
> 
> 	    [ARM] Add initial sparsemem support
> 
> 	    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> However, there's a big problem with this: very few of the lead people
> have machines which suffer from this disability, so there's very little
> testing of it - and there's very little testing of new code with it.

Anyone can test this just by enabling sparsemem with an artificially
small SECTION_SIZE_BITS (that makes it much more likely you'll hit a
boundary).

> The patch which originally introduced this code which your patch
> touches was part of adding highmem support to ARM:
> 
> 	commit 43377453af83b8ff8c1c731da1508bd6b84ebfea
> 	Author: Nicolas Pitre <nico@cam.org>
> 	Date:   Thu Mar 12 22:52:09 2009 -0400
> 
> 	    [ARM] introduce dma_cache_maint_page()
> 
> 	    This is a helper to be used by the DMA mapping API to handle cache
> 	    maintenance for memory identified by a page structure instead of a
> 	    virtual address.  Those pages may or may not be highmem pages, and
> 	    when they're highmem pages, they may or may not be virtually mapped.
> 	    When they're not mapped then there is no L1 cache to worry about. But
> 	    even in that case the L2 cache must be processed since unmapped highmem
> 	    pages can still be L2 cached.
> 
> 	    Signed-off-by: Nicolas Pitre <nico@marvell.com>
> 
> some three years later, and has been through a number of revisions since.
> 
> I'd really like to get rid of sparsemem so it's one less failure case, but
> alas there's a relatively small bunch of folk who rely upon it.  That
> means it's always going to be more buggy.

Heh, I'd like to do that too on parisc; the problem is that the memory
holes in our platform are just to big to use a linear array on.  And now
that the slub people want DISCONTIGMEM deprecated, there's not much
choice.

James

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
> 
> > Now consider this call stack from MMC block driver (this is on the ARmv7 
> > based board):
> >      [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
> > [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
> >      [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
> > [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
> >      [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
> > [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> 
> OK, so this is showing that ARM itself is making the assumption that the
> pages are contiguous in the page offset map.
> 
> Fix this by doing the increment via the pfn, which will do the right
> thing whatever the memory model.
> 
> Signed-off-by: James Bottomley <JBottomley@Parallels.com>

Ok.  What would you like the patch summary line for this to be -
the existing one seems to be a little wrong given the content of
this patch...

> 
> ---
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  			op(vaddr, len, dir);
>  		}
>  		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);
>  		left -= len;
>  	} while (left);
>  }
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
> 
> > Now consider this call stack from MMC block driver (this is on the ARmv7 
> > based board):
> >      [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
> > [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
> >      [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
> > [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
> >      [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
> > [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> 
> OK, so this is showing that ARM itself is making the assumption that the
> pages are contiguous in the page offset map.
> 
> Fix this by doing the increment via the pfn, which will do the right
> thing whatever the memory model.
> 
> Signed-off-by: James Bottomley <JBottomley@Parallels.com>

Ok.  What would you like the patch summary line for this to be -
the existing one seems to be a little wrong given the content of
this patch...

> 
> ---
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  			op(vaddr, len, dir);
>  		}
>  		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);
>  		left -= len;
>  	} while (left);
>  }
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Tejun Heo]

On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  			op(vaddr, len, dir);
>  		}
>  		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);

Probably page = nth_page(page, 1) is the better form.

Thanks.

-- 
tejun

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Tejun Heo]

On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..ab88c5b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  			op(vaddr, len, dir);
>  		}
>  		offset = 0;
> -		page++;
> +		page = pfn_to_page(page_to_pfn(page) + 1);

Probably page = nth_page(page, 1) is the better form.

Thanks.

-- 
tejun

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Wed, 2013-01-16 at 23:14 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> > On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
> > 
> > > Now consider this call stack from MMC block driver (this is on the ARmv7 
> > > based board):
> > >      [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
> > > [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
> > >      [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
> > > [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
> > >      [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
> > > [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> > 
> > OK, so this is showing that ARM itself is making the assumption that the
> > pages are contiguous in the page offset map.
> > 
> > Fix this by doing the increment via the pfn, which will do the right
> > thing whatever the memory model.
> > 
> > Signed-off-by: James Bottomley <JBottomley@Parallels.com>
> 
> Ok.  What would you like the patch summary line for this to be -
> the existing one seems to be a little wrong given the content of
> this patch...

how about

arm: fix struct page iterator in dma_cache_maint() to work with
sparsemem

?

James

> > 
> > ---
> > 
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 6b2fb87..ab88c5b 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
> >  			op(vaddr, len, dir);
> >  		}
> >  		offset = 0;
> > -		page++;
> > +		page = pfn_to_page(page_to_pfn(page) + 1);
> >  		left -= len;
> >  	} while (left);
> >  }
> > 
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> --
> 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

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Wed, 2013-01-16 at 23:14 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> > On Wed, 2013-01-16 at 12:07 +0530, Subhash Jadavani wrote:
> > 
> > > Now consider this call stack from MMC block driver (this is on the ARmv7 
> > > based board):
> > >      [   98.918174] [<c001b50c>] (v7_dma_inv_range+0x30/0x48) from 
> > > [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c)
> > >      [   98.927819] [<c0017b8c>] (dma_cache_maint_page+0x1c4/0x24c) from 
> > > [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c)
> > >      [   98.937982] [<c0017c28>] (___dma_page_cpu_to_dev+0x14/0x1c) from 
> > > [<c0017ff8>] (dma_map_sg+0x3c/0x114)
> > 
> > OK, so this is showing that ARM itself is making the assumption that the
> > pages are contiguous in the page offset map.
> > 
> > Fix this by doing the increment via the pfn, which will do the right
> > thing whatever the memory model.
> > 
> > Signed-off-by: James Bottomley <JBottomley@Parallels.com>
> 
> Ok.  What would you like the patch summary line for this to be -
> the existing one seems to be a little wrong given the content of
> this patch...

how about

arm: fix struct page iterator in dma_cache_maint() to work with
sparsemem

?

James

> > 
> > ---
> > 
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 6b2fb87..ab88c5b 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
> >  			op(vaddr, len, dir);
> >  		}
> >  		offset = 0;
> > -		page++;
> > +		page = pfn_to_page(page_to_pfn(page) + 1);
> >  		left -= len;
> >  	} while (left);
> >  }
> > 
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Wed, 2013-01-16 at 15:18 -0800, Tejun Heo wrote:
> On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 6b2fb87..ab88c5b 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
> >  			op(vaddr, len, dir);
> >  		}
> >  		offset = 0;
> > -		page++;
> > +		page = pfn_to_page(page_to_pfn(page) + 1);
> 
> Probably page = nth_page(page, 1) is the better form.

It's the same thing.

I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
it makes the code look like the hack it is.  The preferred form for all
iterators like this should be to iterate over the pfn instead of a
pointer into the page arrays, because that will always work correctly no
matter how many weird and wonderful memory schemes we come up with.

James

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Wed, 2013-01-16 at 15:18 -0800, Tejun Heo wrote:
> On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 6b2fb87..ab88c5b 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
> >  			op(vaddr, len, dir);
> >  		}
> >  		offset = 0;
> > -		page++;
> > +		page = pfn_to_page(page_to_pfn(page) + 1);
> 
> Probably page = nth_page(page, 1) is the better form.

It's the same thing.

I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
it makes the code look like the hack it is.  The preferred form for all
iterators like this should be to iterate over the pfn instead of a
pointer into the page arrays, because that will always work correctly no
matter how many weird and wonderful memory schemes we come up with.

James

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
> On Wed, 2013-01-16 at 15:18 -0800, Tejun Heo wrote:
> > On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > > index 6b2fb87..ab88c5b 100644
> > > --- a/arch/arm/mm/dma-mapping.c
> > > +++ b/arch/arm/mm/dma-mapping.c
> > > @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
> > >  			op(vaddr, len, dir);
> > >  		}
> > >  		offset = 0;
> > > -		page++;
> > > +		page = pfn_to_page(page_to_pfn(page) + 1);
> > 
> > Probably page = nth_page(page, 1) is the better form.
> 
> It's the same thing.
> 
> I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
> it makes the code look like the hack it is.  The preferred form for all
> iterators like this should be to iterate over the pfn instead of a
> pointer into the page arrays, because that will always work correctly no
> matter how many weird and wonderful memory schemes we come up with.

So, why don't we update the code to do that then?

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
> On Wed, 2013-01-16 at 15:18 -0800, Tejun Heo wrote:
> > On Wed, Jan 16, 2013 at 10:32:35AM +0000, James Bottomley wrote:
> > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > > index 6b2fb87..ab88c5b 100644
> > > --- a/arch/arm/mm/dma-mapping.c
> > > +++ b/arch/arm/mm/dma-mapping.c
> > > @@ -809,7 +809,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
> > >  			op(vaddr, len, dir);
> > >  		}
> > >  		offset = 0;
> > > -		page++;
> > > +		page = pfn_to_page(page_to_pfn(page) + 1);
> > 
> > Probably page = nth_page(page, 1) is the better form.
> 
> It's the same thing.
> 
> I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
> it makes the code look like the hack it is.  The preferred form for all
> iterators like this should be to iterate over the pfn instead of a
> pointer into the page arrays, because that will always work correctly no
> matter how many weird and wonderful memory schemes we come up with.

So, why don't we update the code to do that then?

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 10:37:42AM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
> > I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
> > it makes the code look like the hack it is.  The preferred form for all
> > iterators like this should be to iterate over the pfn instead of a
> > pointer into the page arrays, because that will always work correctly no
> > matter how many weird and wonderful memory schemes we come up with.
> 
> So, why don't we update the code to do that then?

Also, couldn't the addition of the scatterlist offset to the page also
be buggy too?

So, what about this patch which addresses both additions by keeping our
iterator as a pfn as you suggest.  It also simplifies some of the code
in the loop too.

Can the original folk with the problem test this patch?

 arch/arm/mm/dma-mapping.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 6b2fb87..076c26d 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -774,25 +774,27 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
 	size_t size, enum dma_data_direction dir,
 	void (*op)(const void *, size_t, int))
 {
+	unsigned long pfn;
+	size_t left = size;
+
+	pfn = page_to_pfn(page) + offset / PAGE_SIZE;
+	offset %= PAGE_SIZE;
+
 	/*
 	 * A single sg entry may refer to multiple physically contiguous
 	 * pages.  But we still need to process highmem pages individually.
 	 * If highmem is not configured then the bulk of this loop gets
 	 * optimized out.
 	 */
-	size_t left = size;
 	do {
 		size_t len = left;
 		void *vaddr;
 
+		page = pfn_to_page(pfn);
+
 		if (PageHighMem(page)) {
-			if (len + offset > PAGE_SIZE) {
-				if (offset >= PAGE_SIZE) {
-					page += offset / PAGE_SIZE;
-					offset %= PAGE_SIZE;
-				}
+			if (len + offset > PAGE_SIZE)
 				len = PAGE_SIZE - offset;
-			}
 			vaddr = kmap_high_get(page);
 			if (vaddr) {
 				vaddr += offset;
@@ -809,7 +811,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
 			op(vaddr, len, dir);
 		}
 		offset = 0;
-		page++;
+		pfn++;
 		left -= len;
 	} while (left);
 }

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 10:37:42AM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
> > I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
> > it makes the code look like the hack it is.  The preferred form for all
> > iterators like this should be to iterate over the pfn instead of a
> > pointer into the page arrays, because that will always work correctly no
> > matter how many weird and wonderful memory schemes we come up with.
> 
> So, why don't we update the code to do that then?

Also, couldn't the addition of the scatterlist offset to the page also
be buggy too?

So, what about this patch which addresses both additions by keeping our
iterator as a pfn as you suggest.  It also simplifies some of the code
in the loop too.

Can the original folk with the problem test this patch?

 arch/arm/mm/dma-mapping.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 6b2fb87..076c26d 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -774,25 +774,27 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
 	size_t size, enum dma_data_direction dir,
 	void (*op)(const void *, size_t, int))
 {
+	unsigned long pfn;
+	size_t left = size;
+
+	pfn = page_to_pfn(page) + offset / PAGE_SIZE;
+	offset %= PAGE_SIZE;
+
 	/*
 	 * A single sg entry may refer to multiple physically contiguous
 	 * pages.  But we still need to process highmem pages individually.
 	 * If highmem is not configured then the bulk of this loop gets
 	 * optimized out.
 	 */
-	size_t left = size;
 	do {
 		size_t len = left;
 		void *vaddr;
 
+		page = pfn_to_page(pfn);
+
 		if (PageHighMem(page)) {
-			if (len + offset > PAGE_SIZE) {
-				if (offset >= PAGE_SIZE) {
-					page += offset / PAGE_SIZE;
-					offset %= PAGE_SIZE;
-				}
+			if (len + offset > PAGE_SIZE)
 				len = PAGE_SIZE - offset;
-			}
 			vaddr = kmap_high_get(page);
 			if (vaddr) {
 				vaddr += offset;
@@ -809,7 +811,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
 			op(vaddr, len, dir);
 		}
 		offset = 0;
-		page++;
+		pfn++;
 		left -= len;
 	} while (left);
 }

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 10:37:42AM +0000, Russell King - ARM Linux wrote:
> > On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
> > > I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
> > > it makes the code look like the hack it is.  The preferred form for all
> > > iterators like this should be to iterate over the pfn instead of a
> > > pointer into the page arrays, because that will always work correctly no
> > > matter how many weird and wonderful memory schemes we come up with.
> > 
> > So, why don't we update the code to do that then?

We can, but it involves quite a rewrite within the arm dma-mapping code
to use pfn instead of page.  It looks like it would make the code
cleaner because there are a lot of page_to_pfn transformations in there.
However, the current patch is the simplest one for stable and I don't
actually have any arm build and test environments.

> Also, couldn't the addition of the scatterlist offset to the page also
> be buggy too?

No, fortunately, offset must be within the first page from the point of
view of block generated sg lists.  As long as nothing within arm
violates this, it should be a safe assumption ... although the code
seems to assume otherwise.

James

> So, what about this patch which addresses both additions by keeping our
> iterator as a pfn as you suggest.  It also simplifies some of the code
> in the loop too.
> 
> Can the original folk with the problem test this patch?
> 
>  arch/arm/mm/dma-mapping.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..076c26d 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -774,25 +774,27 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  	size_t size, enum dma_data_direction dir,
>  	void (*op)(const void *, size_t, int))
>  {
> +	unsigned long pfn;
> +	size_t left = size;
> +
> +	pfn = page_to_pfn(page) + offset / PAGE_SIZE;
> +	offset %= PAGE_SIZE;
> +
>  	/*
>  	 * A single sg entry may refer to multiple physically contiguous
>  	 * pages.  But we still need to process highmem pages individually.
>  	 * If highmem is not configured then the bulk of this loop gets
>  	 * optimized out.
>  	 */
> -	size_t left = size;
>  	do {
>  		size_t len = left;
>  		void *vaddr;
>  
> +		page = pfn_to_page(pfn);
> +
>  		if (PageHighMem(page)) {
> -			if (len + offset > PAGE_SIZE) {
> -				if (offset >= PAGE_SIZE) {
> -					page += offset / PAGE_SIZE;
> -					offset %= PAGE_SIZE;
> -				}
> +			if (len + offset > PAGE_SIZE)
>  				len = PAGE_SIZE - offset;
> -			}
>  			vaddr = kmap_high_get(page);
>  			if (vaddr) {
>  				vaddr += offset;
> @@ -809,7 +811,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  			op(vaddr, len, dir);
>  		}
>  		offset = 0;
> -		page++;
> +		pfn++;
>  		left -= len;
>  	} while (left);
>  }

Looks reasonable modulo all the simplification we could do if we can
assume offset < PAGE_SIZE

James

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 10:37:42AM +0000, Russell King - ARM Linux wrote:
> > On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
> > > I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
> > > it makes the code look like the hack it is.  The preferred form for all
> > > iterators like this should be to iterate over the pfn instead of a
> > > pointer into the page arrays, because that will always work correctly no
> > > matter how many weird and wonderful memory schemes we come up with.
> > 
> > So, why don't we update the code to do that then?

We can, but it involves quite a rewrite within the arm dma-mapping code
to use pfn instead of page.  It looks like it would make the code
cleaner because there are a lot of page_to_pfn transformations in there.
However, the current patch is the simplest one for stable and I don't
actually have any arm build and test environments.

> Also, couldn't the addition of the scatterlist offset to the page also
> be buggy too?

No, fortunately, offset must be within the first page from the point of
view of block generated sg lists.  As long as nothing within arm
violates this, it should be a safe assumption ... although the code
seems to assume otherwise.

James

> So, what about this patch which addresses both additions by keeping our
> iterator as a pfn as you suggest.  It also simplifies some of the code
> in the loop too.
> 
> Can the original folk with the problem test this patch?
> 
>  arch/arm/mm/dma-mapping.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..076c26d 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -774,25 +774,27 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  	size_t size, enum dma_data_direction dir,
>  	void (*op)(const void *, size_t, int))
>  {
> +	unsigned long pfn;
> +	size_t left = size;
> +
> +	pfn = page_to_pfn(page) + offset / PAGE_SIZE;
> +	offset %= PAGE_SIZE;
> +
>  	/*
>  	 * A single sg entry may refer to multiple physically contiguous
>  	 * pages.  But we still need to process highmem pages individually.
>  	 * If highmem is not configured then the bulk of this loop gets
>  	 * optimized out.
>  	 */
> -	size_t left = size;
>  	do {
>  		size_t len = left;
>  		void *vaddr;
>  
> +		page = pfn_to_page(pfn);
> +
>  		if (PageHighMem(page)) {
> -			if (len + offset > PAGE_SIZE) {
> -				if (offset >= PAGE_SIZE) {
> -					page += offset / PAGE_SIZE;
> -					offset %= PAGE_SIZE;
> -				}
> +			if (len + offset > PAGE_SIZE)
>  				len = PAGE_SIZE - offset;
> -			}
>  			vaddr = kmap_high_get(page);
>  			if (vaddr) {
>  				vaddr += offset;
> @@ -809,7 +811,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>  			op(vaddr, len, dir);
>  		}
>  		offset = 0;
> -		page++;
> +		pfn++;
>  		left -= len;
>  	} while (left);
>  }

Looks reasonable modulo all the simplification we could do if we can
assume offset < PAGE_SIZE

James

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 11:01:47AM +0000, James Bottomley wrote:
> On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> > Also, couldn't the addition of the scatterlist offset to the page also
> > be buggy too?
> 
> No, fortunately, offset must be within the first page from the point of
> view of block generated sg lists.  As long as nothing within arm
> violates this, it should be a safe assumption ... although the code
> seems to assume otherwise.

Are you absolutely sure about that?  I believe I have seen cases where
that has been violated in the past, though it was many years ago.

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 11:01:47AM +0000, James Bottomley wrote:
> On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> > Also, couldn't the addition of the scatterlist offset to the page also
> > be buggy too?
> 
> No, fortunately, offset must be within the first page from the point of
> view of block generated sg lists.  As long as nothing within arm
> violates this, it should be a safe assumption ... although the code
> seems to assume otherwise.

Are you absolutely sure about that?  I believe I have seen cases where
that has been violated in the past, though it was many years ago.

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Thu, 2013-01-17 at 11:04 +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 11:01:47AM +0000, James Bottomley wrote:
> > On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> > > Also, couldn't the addition of the scatterlist offset to the page also
> > > be buggy too?
> > 
> > No, fortunately, offset must be within the first page from the point of
> > view of block generated sg lists.  As long as nothing within arm
> > violates this, it should be a safe assumption ... although the code
> > seems to assume otherwise.
> 
> Are you absolutely sure about that?  I believe I have seen cases where
> that has been violated in the past, though it was many years ago.

>From the point of view of the block layer, absolutely: the scatterlist
is generated from an array of bio_vecs.  Each bio_vec is a page, offset
and length element and obeys the rule that offset must be within the
page and offset + length cannot stray over the page.

>From the point of view of other arm stuff, I don't know.

James

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[James Bottomley]

On Thu, 2013-01-17 at 11:04 +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 11:01:47AM +0000, James Bottomley wrote:
> > On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> > > Also, couldn't the addition of the scatterlist offset to the page also
> > > be buggy too?
> > 
> > No, fortunately, offset must be within the first page from the point of
> > view of block generated sg lists.  As long as nothing within arm
> > violates this, it should be a safe assumption ... although the code
> > seems to assume otherwise.
> 
> Are you absolutely sure about that?  I believe I have seen cases where
> that has been violated in the past, though it was many years ago.

>From the point of view of the block layer, absolutely: the scatterlist
is generated from an array of bio_vecs.  Each bio_vec is a page, offset
and length element and obeys the rule that offset must be within the
page and offset + length cannot stray over the page.

>From the point of view of other arm stuff, I don't know.

James

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 11:19:21AM +0000, James Bottomley wrote:
> On Thu, 2013-01-17 at 11:04 +0000, Russell King - ARM Linux wrote:
> > On Thu, Jan 17, 2013 at 11:01:47AM +0000, James Bottomley wrote:
> > > On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> > > > Also, couldn't the addition of the scatterlist offset to the page also
> > > > be buggy too?
> > > 
> > > No, fortunately, offset must be within the first page from the point of
> > > view of block generated sg lists.  As long as nothing within arm
> > > violates this, it should be a safe assumption ... although the code
> > > seems to assume otherwise.
> > 
> > Are you absolutely sure about that?  I believe I have seen cases where
> > that has been violated in the past, though it was many years ago.
> 
> >From the point of view of the block layer, absolutely: the scatterlist
> is generated from an array of bio_vecs.  Each bio_vec is a page, offset
> and length element and obeys the rule that offset must be within the
> page and offset + length cannot stray over the page.

Well, I found it when working on the mmc stuff initially, long before
it got complex.  The scatterlists were unmodified from the block layer,
and I'm positive I saw occasions where the offset in the scatter lists
were larger than PAGE_SIZE.

> >From the point of view of other arm stuff, I don't know.

I'm not talking about anything ARM specific here.

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Russell King - ARM Linux]

On Thu, Jan 17, 2013 at 11:19:21AM +0000, James Bottomley wrote:
> On Thu, 2013-01-17 at 11:04 +0000, Russell King - ARM Linux wrote:
> > On Thu, Jan 17, 2013 at 11:01:47AM +0000, James Bottomley wrote:
> > > On Thu, 2013-01-17 at 10:47 +0000, Russell King - ARM Linux wrote:
> > > > Also, couldn't the addition of the scatterlist offset to the page also
> > > > be buggy too?
> > > 
> > > No, fortunately, offset must be within the first page from the point of
> > > view of block generated sg lists.  As long as nothing within arm
> > > violates this, it should be a safe assumption ... although the code
> > > seems to assume otherwise.
> > 
> > Are you absolutely sure about that?  I believe I have seen cases where
> > that has been violated in the past, though it was many years ago.
> 
> >From the point of view of the block layer, absolutely: the scatterlist
> is generated from an array of bio_vecs.  Each bio_vec is a page, offset
> and length element and obeys the rule that offset must be within the
> page and offset + length cannot stray over the page.

Well, I found it when working on the mmc stuff initially, long before
it got complex.  The scatterlists were unmodified from the block layer,
and I'm positive I saw occasions where the offset in the scatter lists
were larger than PAGE_SIZE.

> >From the point of view of other arm stuff, I don't know.

I'm not talking about anything ARM specific here.

Re: [PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Subhash Jadavani]

On 1/17/2013 4:17 PM, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 10:37:42AM +0000, Russell King - ARM Linux wrote:
>> On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
>>> I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
>>> it makes the code look like the hack it is.  The preferred form for all
>>> iterators like this should be to iterate over the pfn instead of a
>>> pointer into the page arrays, because that will always work correctly no
>>> matter how many weird and wonderful memory schemes we come up with.
>> So, why don't we update the code to do that then?
> Also, couldn't the addition of the scatterlist offset to the page also
> be buggy too?
>
> So, what about this patch which addresses both additions by keeping our
> iterator as a pfn as you suggest.  It also simplifies some of the code
> in the loop too.
>
> Can the original folk with the problem test this patch?

Yes, this patch also fixes the issue.  You may add: Tested-by: Subhash 
Jadavani <subhashj@codeaurora.org> .

Regards,
Subhash

>
>   arch/arm/mm/dma-mapping.c |   18 ++++++++++--------
>   1 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..076c26d 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -774,25 +774,27 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   	size_t size, enum dma_data_direction dir,
>   	void (*op)(const void *, size_t, int))
>   {
> +	unsigned long pfn;
> +	size_t left = size;
> +
> +	pfn = page_to_pfn(page) + offset / PAGE_SIZE;
> +	offset %= PAGE_SIZE;
> +
>   	/*
>   	 * A single sg entry may refer to multiple physically contiguous
>   	 * pages.  But we still need to process highmem pages individually.
>   	 * If highmem is not configured then the bulk of this loop gets
>   	 * optimized out.
>   	 */
> -	size_t left = size;
>   	do {
>   		size_t len = left;
>   		void *vaddr;
>   
> +		page = pfn_to_page(pfn);
> +
>   		if (PageHighMem(page)) {
> -			if (len + offset > PAGE_SIZE) {
> -				if (offset >= PAGE_SIZE) {
> -					page += offset / PAGE_SIZE;
> -					offset %= PAGE_SIZE;
> -				}
> +			if (len + offset > PAGE_SIZE)
>   				len = PAGE_SIZE - offset;
> -			}
>   			vaddr = kmap_high_get(page);
>   			if (vaddr) {
>   				vaddr += offset;
> @@ -809,7 +811,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   			op(vaddr, len, dir);
>   		}
>   		offset = 0;
> -		page++;
> +		pfn++;
>   		left -= len;
>   	} while (left);
>   }
>
> --
> 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

[PATCH v2 1/1] block: blk-merge: don't merge the pages with non-contiguous descriptors

[Subhash Jadavani]

On 1/17/2013 4:17 PM, Russell King - ARM Linux wrote:
> On Thu, Jan 17, 2013 at 10:37:42AM +0000, Russell King - ARM Linux wrote:
>> On Thu, Jan 17, 2013 at 09:11:20AM +0000, James Bottomley wrote:
>>> I'd actually prefer page = pfn_to_page(page_to_pfn(page) + 1); because
>>> it makes the code look like the hack it is.  The preferred form for all
>>> iterators like this should be to iterate over the pfn instead of a
>>> pointer into the page arrays, because that will always work correctly no
>>> matter how many weird and wonderful memory schemes we come up with.
>> So, why don't we update the code to do that then?
> Also, couldn't the addition of the scatterlist offset to the page also
> be buggy too?
>
> So, what about this patch which addresses both additions by keeping our
> iterator as a pfn as you suggest.  It also simplifies some of the code
> in the loop too.
>
> Can the original folk with the problem test this patch?

Yes, this patch also fixes the issue.  You may add: Tested-by: Subhash 
Jadavani <subhashj@codeaurora.org> .

Regards,
Subhash

>
>   arch/arm/mm/dma-mapping.c |   18 ++++++++++--------
>   1 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 6b2fb87..076c26d 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -774,25 +774,27 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   	size_t size, enum dma_data_direction dir,
>   	void (*op)(const void *, size_t, int))
>   {
> +	unsigned long pfn;
> +	size_t left = size;
> +
> +	pfn = page_to_pfn(page) + offset / PAGE_SIZE;
> +	offset %= PAGE_SIZE;
> +
>   	/*
>   	 * A single sg entry may refer to multiple physically contiguous
>   	 * pages.  But we still need to process highmem pages individually.
>   	 * If highmem is not configured then the bulk of this loop gets
>   	 * optimized out.
>   	 */
> -	size_t left = size;
>   	do {
>   		size_t len = left;
>   		void *vaddr;
>   
> +		page = pfn_to_page(pfn);
> +
>   		if (PageHighMem(page)) {
> -			if (len + offset > PAGE_SIZE) {
> -				if (offset >= PAGE_SIZE) {
> -					page += offset / PAGE_SIZE;
> -					offset %= PAGE_SIZE;
> -				}
> +			if (len + offset > PAGE_SIZE)
>   				len = PAGE_SIZE - offset;
> -			}
>   			vaddr = kmap_high_get(page);
>   			if (vaddr) {
>   				vaddr += offset;
> @@ -809,7 +811,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
>   			op(vaddr, len, dir);
>   		}
>   		offset = 0;
> -		page++;
> +		pfn++;
>   		left -= len;
>   	} while (left);
>   }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html