[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[Russell King]

We must use a 64-bit for this, otherwise overflowed bits get lost, and
that can result in a lower than intended value set.

Fixes: 8e0cb8a1f6ac ("ARM: 7797/1: mmc: Use dma_max_pfn(dev) helper for bounce_limit calculations")
Fixes: 7d35496dd982 ("ARM: 7796/1: scsi: Use dma_max_pfn(dev) helper for bounce_limit calculations")
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
While poking about with the Cubox-i4 and investigating why my UHS-1
SD card wasn't achieving its full potential, I came across a slight
problem... the SDHCI host sets a mask of 0xffffffff, but with the
start of memory at pfn 0x10000, the blk code sees this when setting
the bounce limit:

	max addr 0x0ffff000 bounce limit 0xffff dma 1

and this results in the bounce functions appearing in the profile:

00000000c00f8b70 copy_to_high_bio_irq                  1139 2.5886
00000000c00f8d28 bounce_end_io                           12 0.0714
00000000c00f8dd0 bounce_end_io_read_isa                   8 0.1053

which, compared to the cost of copying the data to userland and
request handling, this is quite significant:

00000000c04b1794 sdhci_request                          268 0.5447
00000000c02d0740 __copy_to_user_std                     398 0.4252

With this calculation fixed, we avoid the bouncing code entirely.

	max addr 0x10ffff000 bounce limit 0x10ffff dma 0

 drivers/mmc/card/queue.c | 2 +-
 drivers/scsi/scsi_lib.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 357bbc54fe4b..3e049c13429c 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -197,7 +197,7 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
 	struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
 
 	if (mmc_dev(host)->dma_mask && *mmc_dev(host)->dma_mask)
-		limit = dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
+		limit = (u64)dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
 
 	mq->card = card;
 	mq->queue = blk_init_queue(mmc_request_fn, lock);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d5f050..62ec84b42e31 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1684,7 +1684,7 @@ u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
 
 	host_dev = scsi_get_device(shost);
 	if (host_dev && host_dev->dma_mask)
-		bounce_limit = dma_max_pfn(host_dev) << PAGE_SHIFT;
+		bounce_limit = (u64)dma_max_pfn(host_dev) << PAGE_SHIFT;
 
 	return bounce_limit;
 }
-- 
1.8.3.1

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[Santosh Shilimkar]

On Tuesday 11 February 2014 12:28 PM, Russell King wrote:
> We must use a 64-bit for this, otherwise overflowed bits get lost, and
> that can result in a lower than intended value set.
> 
> Fixes: 8e0cb8a1f6ac ("ARM: 7797/1: mmc: Use dma_max_pfn(dev) helper for bounce_limit calculations")
> Fixes: 7d35496dd982 ("ARM: 7796/1: scsi: Use dma_max_pfn(dev) helper for bounce_limit calculations")
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> While poking about with the Cubox-i4 and investigating why my UHS-1
> SD card wasn't achieving its full potential, I came across a slight
> problem... the SDHCI host sets a mask of 0xffffffff, but with the
> start of memory at pfn 0x10000, the blk code sees this when setting
> the bounce limit:
> 
> 	max addr 0x0ffff000 bounce limit 0xffff dma 1
> 
> and this results in the bounce functions appearing in the profile:
> 
> 00000000c00f8b70 copy_to_high_bio_irq                  1139 2.5886
> 00000000c00f8d28 bounce_end_io                           12 0.0714
> 00000000c00f8dd0 bounce_end_io_read_isa                   8 0.1053
> 
> which, compared to the cost of copying the data to userland and
> request handling, this is quite significant:
> 
> 00000000c04b1794 sdhci_request                          268 0.5447
> 00000000c02d0740 __copy_to_user_std                     398 0.4252
> 
> With this calculation fixed, we avoid the bouncing code entirely.
> 
> 	max addr 0x10ffff000 bounce limit 0x10ffff dma 0
>
You are right. We were seeing an issue with SCSI HDD over PCI and
the patch fixes that issue as well.

I think we should send this patch to stable as well. At least 3.13
should get this patch.

Thanks RMK for the bug fix. Feel free to add, in case the patch is
not committed already

Tested-Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
 
>  drivers/mmc/card/queue.c | 2 +-
>  drivers/scsi/scsi_lib.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index 357bbc54fe4b..3e049c13429c 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -197,7 +197,7 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>  	struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
>  
>  	if (mmc_dev(host)->dma_mask && *mmc_dev(host)->dma_mask)
> -		limit = dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
> +		limit = (u64)dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
>  
>  	mq->card = card;
>  	mq->queue = blk_init_queue(mmc_request_fn, lock);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7bd7f0d5f050..62ec84b42e31 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1684,7 +1684,7 @@ u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
>  
>  	host_dev = scsi_get_device(shost);
>  	if (host_dev && host_dev->dma_mask)
> -		bounce_limit = dma_max_pfn(host_dev) << PAGE_SHIFT;
> +		bounce_limit = (u64)dma_max_pfn(host_dev) << PAGE_SHIFT;
>  
>  	return bounce_limit;
>  }
> 

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[James Bottomley]

On Tue, 2014-02-11 at 17:28 +0000, Russell King wrote:
> We must use a 64-bit for this, otherwise overflowed bits get lost, and
> that can result in a lower than intended value set.
> 
> Fixes: 8e0cb8a1f6ac ("ARM: 7797/1: mmc: Use dma_max_pfn(dev) helper for bounce_limit calculations")
> Fixes: 7d35496dd982 ("ARM: 7796/1: scsi: Use dma_max_pfn(dev) helper for bounce_limit calculations")
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> While poking about with the Cubox-i4 and investigating why my UHS-1
> SD card wasn't achieving its full potential, I came across a slight
> problem... the SDHCI host sets a mask of 0xffffffff, but with the
> start of memory at pfn 0x10000, the blk code sees this when setting
> the bounce limit:
> 
> 	max addr 0x0ffff000 bounce limit 0xffff dma 1
> 
> and this results in the bounce functions appearing in the profile:
> 
> 00000000c00f8b70 copy_to_high_bio_irq                  1139 2.5886
> 00000000c00f8d28 bounce_end_io                           12 0.0714
> 00000000c00f8dd0 bounce_end_io_read_isa                   8 0.1053
> 
> which, compared to the cost of copying the data to userland and
> request handling, this is quite significant:
> 
> 00000000c04b1794 sdhci_request                          268 0.5447
> 00000000c02d0740 __copy_to_user_std                     398 0.4252
> 
> With this calculation fixed, we avoid the bouncing code entirely.
> 
> 	max addr 0x10ffff000 bounce limit 0x10ffff dma 0
> 
>  drivers/mmc/card/queue.c | 2 +-
>  drivers/scsi/scsi_lib.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index 357bbc54fe4b..3e049c13429c 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -197,7 +197,7 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>  	struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
>  
>  	if (mmc_dev(host)->dma_mask && *mmc_dev(host)->dma_mask)
> -		limit = dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
> +		limit = (u64)dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
>  
>  	mq->card = card;
>  	mq->queue = blk_init_queue(mmc_request_fn, lock);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7bd7f0d5f050..62ec84b42e31 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1684,7 +1684,7 @@ u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
>  
>  	host_dev = scsi_get_device(shost);
>  	if (host_dev && host_dev->dma_mask)
> -		bounce_limit = dma_max_pfn(host_dev) << PAGE_SHIFT;
> +		bounce_limit = (u64)dma_max_pfn(host_dev) << PAGE_SHIFT;

This doesn't really look like the right fix.  You replaced dev->dma_mask
with a calculation on dev_max_pfn().  Since dev->dma_mask is always u64
and dev_max_pfn is supposed to be returning the pfn of the dma_mask, it
should unconditionally be 64 bits as well.  Either that or it should
return dma_addr_t.

James

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[Russell King - ARM Linux]

On Thu, Feb 13, 2014 at 08:58:10AM -0800, James Bottomley wrote:
> This doesn't really look like the right fix.  You replaced dev->dma_mask
> with a calculation on dev_max_pfn().  Since dev->dma_mask is always u64
> and dev_max_pfn is supposed to be returning the pfn of the dma_mask, it
> should unconditionally be 64 bits as well.  Either that or it should
> return dma_addr_t.

My reasoning is that PFNs in the system are always of type "unsigned long"
and therefore a function returning a pfn should have that type.  If we
overflow a PFN fitting in an unsigned long, we have lots of places which
need fixing.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[James Bottomley]

On Thu, 2014-02-13 at 17:11 +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 08:58:10AM -0800, James Bottomley wrote:
> > This doesn't really look like the right fix.  You replaced dev->dma_mask
> > with a calculation on dev_max_pfn().  Since dev->dma_mask is always u64
> > and dev_max_pfn is supposed to be returning the pfn of the dma_mask, it
> > should unconditionally be 64 bits as well.  Either that or it should
> > return dma_addr_t.
> 
> My reasoning is that PFNs in the system are always of type "unsigned long"
> and therefore a function returning a pfn should have that type.  If we
> overflow a PFN fitting in an unsigned long, we have lots of places which
> need fixing.

It's not intuitive to people who need the dma mask that they're supposed
to use dma_max_pfn() << PAGE_SHIFT but now they have to worry about the
casting and, if they don't get it right, nothing will warn or tell them.
what about a new macro, say dma_max_mask(dev) that just returns
(u64)dma_max_pfn() << PAGE_SHIFT?

James

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[Russell King - ARM Linux]

On Thu, Feb 13, 2014 at 10:07:01AM -0800, James Bottomley wrote:
> On Thu, 2014-02-13 at 17:11 +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 13, 2014 at 08:58:10AM -0800, James Bottomley wrote:
> > > This doesn't really look like the right fix.  You replaced dev->dma_mask
> > > with a calculation on dev_max_pfn().  Since dev->dma_mask is always u64
> > > and dev_max_pfn is supposed to be returning the pfn of the dma_mask, it
> > > should unconditionally be 64 bits as well.  Either that or it should
> > > return dma_addr_t.
> > 
> > My reasoning is that PFNs in the system are always of type "unsigned long"
> > and therefore a function returning a pfn should have that type.  If we
> > overflow a PFN fitting in an unsigned long, we have lots of places which
> > need fixing.
> 
> It's not intuitive to people who need the dma mask that they're supposed
> to use dma_max_pfn() << PAGE_SHIFT but now they have to worry about the
> casting and, if they don't get it right, nothing will warn or tell them.
> what about a new macro, say dma_max_mask(dev) that just returns
> (u64)dma_max_pfn() << PAGE_SHIFT?

This sounds like a good idea.

I've just been looking@places which do this << PAGE_SHIFT, and we
have other places which suffer from this same bug all over the kernel,
so maybe we actually need a pfn_to_addr() macro or similar so that
people get this right in these other places too?  It appears to be
quite a widespread problem.

I'm surprised none of the below haven't already caused a problem.

Thoughts?

int __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
                 unsigned long nr_pages)
{
        resource_size_t start, size;

        start = phys_start_pfn << PAGE_SHIFT;

void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
                unsigned long start_pfn, enum memmap_context context)
{
        unsigned long end_pfn = start_pfn + size;
        unsigned long pfn;

                /* The shift won't overflow because ZONE_NORMAL is below 4G. */
                if (!is_highmem_idx(zone))
                        set_page_address(page, __va(pfn << PAGE_SHIFT));

void __init free_area_init_nodes(unsigned long *max_zone_pfn)
{
        unsigned long start_pfn, end_pfn;

        for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid)
                printk("  node %3d: [mem %#010lx-%#010lx]\n", nid,
                       start_pfn << PAGE_SHIFT, (end_pfn << PAGE_SHIFT) - 1);
(thankfully, this one is just a printk).

int vb2_get_contig_userptr(unsigned long vaddr, unsigned long size,
                           struct vm_area_struct **res_vma, dma_addr_t *res_pa)
{
        unsigned long this_pfn, prev_pfn;
        dma_addr_t pa = 0;

                if (prev_pfn == 0)
                        pa = this_pfn << PAGE_SHIFT;

static pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
                                     unsigned long size, pgprot_t vma_prot)
{
#ifdef pgprot_noncached
        phys_addr_t offset = pfn << PAGE_SHIFT;

        if (uncached_access(file, offset))
                return pgprot_noncached(vma_prot);
#endif
        return vma_prot;

static int i810_insert_dcache_entries(struct agp_memory *mem, off_t pg_start,
                                      int type)
{
        int i;

        for (i = pg_start; i < (pg_start + mem->page_count); i++) {
                dma_addr_t addr = i << PAGE_SHIFT;


-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[Russell King - ARM Linux]

On Thu, Feb 13, 2014 at 08:01:12PM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 10:07:01AM -0800, James Bottomley wrote:
> > On Thu, 2014-02-13 at 17:11 +0000, Russell King - ARM Linux wrote:
> > > On Thu, Feb 13, 2014 at 08:58:10AM -0800, James Bottomley wrote:
> > > > This doesn't really look like the right fix.  You replaced dev->dma_mask
> > > > with a calculation on dev_max_pfn().  Since dev->dma_mask is always u64
> > > > and dev_max_pfn is supposed to be returning the pfn of the dma_mask, it
> > > > should unconditionally be 64 bits as well.  Either that or it should
> > > > return dma_addr_t.
> > > 
> > > My reasoning is that PFNs in the system are always of type "unsigned long"
> > > and therefore a function returning a pfn should have that type.  If we
> > > overflow a PFN fitting in an unsigned long, we have lots of places which
> > > need fixing.
> > 
> > It's not intuitive to people who need the dma mask that they're supposed
> > to use dma_max_pfn() << PAGE_SHIFT but now they have to worry about the
> > casting and, if they don't get it right, nothing will warn or tell them.
> > what about a new macro, say dma_max_mask(dev) that just returns
> > (u64)dma_max_pfn() << PAGE_SHIFT?
> 
> This sounds like a good idea.
> 
> I've just been looking at places which do this << PAGE_SHIFT, and we
> have other places which suffer from this same bug all over the kernel,
> so maybe we actually need a pfn_to_addr() macro or similar so that
> people get this right in these other places too?  It appears to be
> quite a widespread problem.
> 
> I'm surprised none of the below haven't already caused a problem.
> 
> Thoughts?

Okay, as there's been no response to this, I'm going to push the patch
as-is to Linus this evening.

Nevertheless, your point is valid, but it is more of a general problem in
the kernel than with this specific change - and it's more important to fix
the existing problem here than to try and work out a new way of sorting
this out to fix the general case in the middle of the -rc series.

It is arguable that people _should_ know that shifting an unsigned long
left and assigning it to a u64 doesn't automatically get any of the high
bits set - the promotion happens at assignment, not while the expression
is being evaluated.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH] Fix uses of dma_max_pfn() when converting to a limiting address

[Ulf Hansson]

On 11 February 2014 18:28, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> We must use a 64-bit for this, otherwise overflowed bits get lost, and
> that can result in a lower than intended value set.
>
> Fixes: 8e0cb8a1f6ac ("ARM: 7797/1: mmc: Use dma_max_pfn(dev) helper for bounce_limit calculations")
> Fixes: 7d35496dd982 ("ARM: 7796/1: scsi: Use dma_max_pfn(dev) helper for bounce_limit calculations")
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
> While poking about with the Cubox-i4 and investigating why my UHS-1
> SD card wasn't achieving its full potential, I came across a slight
> problem... the SDHCI host sets a mask of 0xffffffff, but with the
> start of memory at pfn 0x10000, the blk code sees this when setting
> the bounce limit:
>
>         max addr 0x0ffff000 bounce limit 0xffff dma 1
>
> and this results in the bounce functions appearing in the profile:
>
> 00000000c00f8b70 copy_to_high_bio_irq                  1139 2.5886
> 00000000c00f8d28 bounce_end_io                           12 0.0714
> 00000000c00f8dd0 bounce_end_io_read_isa                   8 0.1053
>
> which, compared to the cost of copying the data to userland and
> request handling, this is quite significant:
>
> 00000000c04b1794 sdhci_request                          268 0.5447
> 00000000c02d0740 __copy_to_user_std                     398 0.4252
>
> With this calculation fixed, we avoid the bouncing code entirely.
>
>         max addr 0x10ffff000 bounce limit 0x10ffff dma 0
>
>  drivers/mmc/card/queue.c | 2 +-
>  drivers/scsi/scsi_lib.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index 357bbc54fe4b..3e049c13429c 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -197,7 +197,7 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>         struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
>
>         if (mmc_dev(host)->dma_mask && *mmc_dev(host)->dma_mask)
> -               limit = dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
> +               limit = (u64)dma_max_pfn(mmc_dev(host)) << PAGE_SHIFT;
>
>         mq->card = card;
>         mq->queue = blk_init_queue(mmc_request_fn, lock);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7bd7f0d5f050..62ec84b42e31 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1684,7 +1684,7 @@ u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
>
>         host_dev = scsi_get_device(shost);
>         if (host_dev && host_dev->dma_mask)
> -               bounce_limit = dma_max_pfn(host_dev) << PAGE_SHIFT;
> +               bounce_limit = (u64)dma_max_pfn(host_dev) << PAGE_SHIFT;
>
>         return bounce_limit;
>  }
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html