Re: [PATCH 1/2] mmc: renesas_sdhi: fix swiotlb buffer is full

[Konrad Rzeszutek Wilk]

On Fri, Nov 03, 2017 at 03:01:12PM +0100, Geert Uytterhoeven wrote:
> Hi Konrad,
> 
> On Fri, Nov 3, 2017 at 2:23 PM, Konrad Rzeszutek Wilk <kon...@kernel.org> 
> wrote:
> > ..snip..
> >> > >
> >> > > Anyway, I made v2 patches by using swiotlb related definitions. Would 
> >> > > you check it?
> >> >
> >> > Did I miss that email? As in was I cc-ed?
> >>
> >> This was my fault. When I submitted v2 patches, I didn't include your 
> >> email and iommu mailing list...
> >
> > No problem.
> >>
> >> > > https://patchwork.kernel.org/patch/10018879/
> >> >
> >> > Why not use IO_TLB_SEGSIZE << IO_TLB_SHIFT or alternatively
> >> > swiotlb_max_segment?  See 5584f1b1d73e9
> >>
> >> I already made such a patch as v2 and it was merged into mmc.git / fixes 
> >> branch.
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git/commit/?h=fixes=e90e8da72ad694a16a4ffa6e5adae3610208f73b
> >
> > What happens if the user has swiotlb=4096 on the command line (meaning
> > less than the default value)? Your max value will be incorrect. Could you 
> > use
> > swiotlb_max_segment?
> 
> No, as that's the total size of the swiotlb buffer, not the maximum size of
> a single segment.


Aaah, then you are all fine! Thanks for the quote!
> 
> Quoting an earlier part of the thread:
> 
> On Thu, Oct 19, 2017 at 1:39 PM, Yoshihiro Shimoda
> <yoshihiro.shimoda...@renesas.com> wrote:
> >> >> iommu: Is there a better (generic) way to handle this?
> >> >
> >> > Yes. See 7453c549f5f6485c0d79cad7844870dcc7d1b34d, aka 
> >> > swiotlb_max_segment
> >>
> >> Thanks for the pointer!
> >>
> >> While I agree this can be used to avoid the swiotlb buffer full issue,
> >> I believe it is a suboptimal solution if the device actually uses an IOMMU.
> >> It limits the mapping size if CONFIG_SWIOTLB=y, which is always the
> >> case for arm/arm64 these days.
> >
> > I'm afraid but I misunderstood this API's spec when I read it at first.
> > After I tried to use it, I found the API cannot be used for a workaround 
> > because
> > this API returns total size of swiotlb.
> >
> > For example:
> >  - The swiotlb_max_segment() returns 64M bytes from the API when a default 
> > setting.
> >   - In this case, the maximum size per a map is 256k bytes.
> >  - The swiotlb_max_segment() returns 128M bytes from the API when I added 
> > swiotlb=65536
> >into the kernel parameter on arm64.
> >   - In this case, the maximum size per a map is still 256k bytes because
> > the swiotlb has hardcoded the size by the following code:
> >  
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/swiotlb.c?h=v4.14-rc5#n254
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH 1/2] mmc: renesas_sdhi: fix swiotlb buffer is full

[Konrad Rzeszutek Wilk]

..snip..
> > >
> > > Anyway, I made v2 patches by using swiotlb related definitions. Would you 
> > > check it?
> > 
> > Did I miss that email? As in was I cc-ed?
> 
> This was my fault. When I submitted v2 patches, I didn't include your email 
> and iommu mailing list...

No problem.
> 
> > > https://patchwork.kernel.org/patch/10018879/
> > 
> > Why not use IO_TLB_SEGSIZE << IO_TLB_SHIFT or alternatively
> > swiotlb_max_segment?  See 5584f1b1d73e9
> 
> I already made such a patch as v2 and it was merged into mmc.git / fixes 
> branch.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git/commit/?h=fixes=e90e8da72ad694a16a4ffa6e5adae3610208f73b

What happens if the user has swiotlb=4096 on the command line (meaning
less than the default value)? Your max value will be incorrect. Could you use
swiotlb_max_segment?

Re: [PATCH 1/2] mmc: renesas_sdhi: fix swiotlb buffer is full

[Konrad Rzeszutek Wilk]

On Fri, Oct 20, 2017 at 03:18:55AM +, Yoshihiro Shimoda wrote:
> Hi again!
> 
> > From: Yoshihiro Shimoda, Sent: Thursday, October 19, 2017 8:39 PM
> > 
> > Hi Geert-san, Konrad-san,
> > 
> > > From: Geert Uytterhoeven, Sent: Thursday, October 19, 2017 5:34 PM
> > >
> > > Hi Konrad,
> > >
> > > On Thu, Oct 19, 2017 at 2:24 AM, Konrad Rzeszutek Wilk
> > > <kon...@darnok.org> wrote:
> < snip >
> > > >> > diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c 
> > > >> > b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > > >> > index f905f23..6c9b4b2 100644
> > > >> > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > > >> > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > > >> > @@ -80,8 +80,9 @@
> > > >> > .scc_offset = 0x1000,
> > > >> > .taps   = rcar_gen3_scc_taps,
> > > >> > .taps_num   = ARRAY_SIZE(rcar_gen3_scc_taps),
> > > >> > -   /* Gen3 SDHI DMAC can handle 0x blk count, but seg = 
> > > >> > 1 */
> > > >> > -   .max_blk_count  = 0x,
> > > >> > +   /* The swiotlb can handle memory size up to 256 kbytes for 
> > > >> > now. */
> > > >> > +   .max_blk_count  = 512,
> > > >>
> > > >> Fixing this in the individual drivers feels like the wrong solution to 
> > > >> me.
> > > >>
> > > >> iommu: Is there a better (generic) way to handle this?
> > > >
> > > > Yes. See 7453c549f5f6485c0d79cad7844870dcc7d1b34d, aka 
> > > > swiotlb_max_segment
> > >
> > > Thanks for the pointer!
> > >
> > > While I agree this can be used to avoid the swiotlb buffer full issue,
> > > I believe it is a suboptimal solution if the device actually uses an 
> > > IOMMU.
> > > It limits the mapping size if CONFIG_SWIOTLB=y, which is always the
> > > case for arm/arm64 these days.
> > 
> > I'm afraid but I misunderstood this API's spec when I read it at first.
> > After I tried to use it, I found the API cannot be used for a workaround 
> > because
> > this API returns total size of swiotlb.
> > 
> > For example:
> >  - The swiotlb_max_segment() returns 64M bytes from the API when a default 
> > setting.
> >   - In this case, the maximum size per a map is 256k bytes.
> >  - The swiotlb_max_segment() returns 128M bytes from the API when I added 
> > swiotlb=65536
> >into the kernel parameter on arm64.
> >   - In this case, the maximum size per a map is still 256k bytes because
> > the swiotlb has hardcoded the size by the following code:
> >  
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/swiotlb.c?h=v4.14-rc5#n254
> > 
> > So, how do we handle to resolve (or avoid) the issue?
> 
> Anyway, I made v2 patches by using swiotlb related definitions. Would you 
> check it?

Did I miss that email? As in was I cc-ed?

> https://patchwork.kernel.org/patch/10018879/

Why not use IO_TLB_SEGSIZE << IO_TLB_SHIFT or alternatively
swiotlb_max_segment?  See 5584f1b1d73e9

Re: [PATCH 1/2] mmc: renesas_sdhi: fix swiotlb buffer is full

[Konrad Rzeszutek Wilk]

On Tue, Oct 17, 2017 at 10:02:50AM +0200, Geert Uytterhoeven wrote:
> Hi Shimoda-san,
> 
> CC iommu
> 
> On Tue, Oct 17, 2017 at 9:30 AM, Yoshihiro Shimoda
>  wrote:
> > Since the commit de3ee99b097d ("mmc: Delete bounce buffer handling")
> > deletes the bounce buffer handling, a request data size will be referred
> > to max_{req,seg}_size instead of MMC_QUEUE_BOUNCESZ (64k bytes).
> >
> > In other hand, renesas_sdhi_internal_dmac.c will set very big value of
> > max_{req,seg}_size because the max_blk_count is set to 0x.
> > And then, "swiotlb buffer is full" happens because swiotlb can handle
> > a memory size up to 256k bytes only (IO_TLB_SEGSIZE = 128 and
> > IO_TLB_SHIFT = 11).
> >
> > So, this patch fixes the issue to set max_blk_count to 512. Then,
> > the max_{req,seg}_size will be set to 256k bytes.
> >
> > Reported-by: Dirk Behme 
> > Signed-off-by: Yoshihiro Shimoda 
> 
> Thanks for your patch!
> 
> > ---
> >  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c 
> > b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > index f905f23..6c9b4b2 100644
> > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > @@ -80,8 +80,9 @@
> > .scc_offset = 0x1000,
> > .taps   = rcar_gen3_scc_taps,
> > .taps_num   = ARRAY_SIZE(rcar_gen3_scc_taps),
> > -   /* Gen3 SDHI DMAC can handle 0x blk count, but seg = 1 */
> > -   .max_blk_count  = 0x,
> > +   /* The swiotlb can handle memory size up to 256 kbytes for now. */
> > +   .max_blk_count  = 512,
> 
> Fixing this in the individual drivers feels like the wrong solution to me.
> 
> iommu: Is there a better (generic) way to handle this?

Yes. See 7453c549f5f6485c0d79cad7844870dcc7d1b34d, aka swiotlb_max_segment

Re: [PATCH v2 2/3] swiotlb: Convert swiotlb_force from int to enum

[Konrad Rzeszutek Wilk]

On Wed, Jan 04, 2017 at 05:57:17PM +, Catalin Marinas wrote:
> On Fri, Dec 16, 2016 at 02:28:41PM +0100, Geert Uytterhoeven wrote:
> > Convert the flag swiotlb_force from an int to an enum, to prepare for
> > the advent of more possible values.
> > 
> > Suggested-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> > Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> > ---
> > v2:
> >   - New.
> > ---
> >  arch/arm64/mm/dma-mapping.c|  3 ++-
> >  arch/arm64/mm/init.c   |  3 ++-
> >  arch/x86/kernel/pci-swiotlb.c  |  2 +-
> >  arch/x86/xen/pci-swiotlb-xen.c |  2 +-
> >  drivers/xen/swiotlb-xen.c  |  4 ++--
> >  include/linux/swiotlb.h|  7 ++-
> >  include/trace/events/swiotlb.h | 16 +---
> >  lib/swiotlb.c  |  8 
> >  8 files changed, 27 insertions(+), 18 deletions(-)
> 
> If this hasn't been queued somewhere yet, for arm64:

It is on swiotlb git tree.
> 
> Acked-by: Catalin Marinas <catalin.mari...@arm.com>

Re: [PATCH v2 3/3] swiotlb: Add swiotlb=noforce debug option

[Konrad Rzeszutek Wilk]

On Mon, Dec 19, 2016 at 02:57:00PM +0100, Geert Uytterhoeven wrote:
> Hi Konrad,
> 
> On Mon, Dec 19, 2016 at 2:38 PM, Konrad Rzeszutek Wilk
> <kon...@darnok.org> wrote:
> > On Fri, Dec 16, 2016 at 02:28:42PM +0100, Geert Uytterhoeven wrote:
> >> On architectures like arm64, swiotlb is tied intimately to the core
> >> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
> >>
> >> To aid debugging and catch devices not supporting DMA to memory outside
> >> the 32-bit address space, add a kernel command line option
> >> "swiotlb=noforce", which disables the use of bounce buffers.
> >> If specified, trying to map memory that cannot be used with DMA will
> >> fail, and a rate-limited warning will be printed.
> >>
> >> Note that io_tlb_nslabs is set to 1, which is the minimal supported
> >> value.
> >>
> >> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> >
> > What is this based on? I can't apply it on my latest that I had
> > sent to Linus?
> 
> I rebased it on last Friday's linux-next, due to the recent move of
> kernel-parameters.txt.
> All the rest should be identical to your linux-next branch.
> 
> > Could you rebase this one please on:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git 
> > stable/for-linus-4.9
> 
> That one indeed doesn't have the move of kernel-parameters.txt.
> 
> > Or if alternatively that does not - then please rebase it on
> > b5cab0da75c292ffa0fbd68dd2c820066b2842de
> 
> However, my series already applies cleanly with git am on top of
> that commit?

Ah that does indeed work.

Which means I can't push it to Linus until rc2 at least.

Linus is sad when he sees git pulls that hadn't "soaked"
for two weeks in linux-next - and especially patches on top of
merges during the merge window.

> 
> Please let me know what to do. Thanks!

Building and testing them now.

Wish there was some travis script to do this.

Re: [PATCH v2 3/3] swiotlb: Add swiotlb=noforce debug option

[Konrad Rzeszutek Wilk]

On Fri, Dec 16, 2016 at 02:28:42PM +0100, Geert Uytterhoeven wrote:
> On architectures like arm64, swiotlb is tied intimately to the core
> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
> 
> To aid debugging and catch devices not supporting DMA to memory outside
> the 32-bit address space, add a kernel command line option
> "swiotlb=noforce", which disables the use of bounce buffers.
> If specified, trying to map memory that cannot be used with DMA will
> fail, and a rate-limited warning will be printed.
> 
> Note that io_tlb_nslabs is set to 1, which is the minimal supported
> value.
> 
> Signed-off-by: Geert Uytterhoeven 

What is this based on? I can't apply it on my latest that I had
sent to Linus?

Could you rebase this one please on:

git://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git 
stable/for-linus-4.9

Thanks?

Or if alternatively that does not - then please rebase it on
b5cab0da75c292ffa0fbd68dd2c820066b2842de

Thanks!

Re: [PATCH v2 0/3] swiotlb: Add swiotlb=noforce debug option

[Konrad Rzeszutek Wilk]

On Fri, Dec 16, 2016 at 02:28:39PM +0100, Geert Uytterhoeven wrote:
>   Hi Konrad,

Heya!
> 
> This patch series adds a kernel command line option to aid debugging
> when developing support for DMA to memory outside the 32-bit address
> space.  If specified, trying to map memory that cannot be used with DMA
> will fail, and a warning will be printed.  This has been used
> succesfully to find drivers and DMA engines that do not support 64-bit
> memory.
> 
> Changes compared to v1:
>   - Dropp patch "swiotlb: Rate-limit printing when running out of
> SW-IOMMU space", which has been picked up,
>   - Add cleanup patch 1/3,
>   - Add patch 2/3, to convert the flag swiotlb_force from an int to an
> enum,
>   - Change the kernel parameter "swiotlb=nobounce" to "swiotlb=noforce"
> (requested by Konrad Wilk),
>   - Extend swiotlb_force enum instead of adding an swiotlb_nobounce
> variable (requested by Konrad Wilk).
> 
> This has been tested on ra7796/salvator-x, and compile-tested on x86.

Let me run it on various combinations of x86 and if all is good
will ask Linus to pick it up for rc1.

Thanks!
> 
> Thanks!
> 
> Geert Uytterhoeven (3):
>   x86, swiotlb: Simplify pci_swiotlb_detect_override()
>   swiotlb: Convert swiotlb_force from int to enum
>   swiotlb: Add swiotlb=noforce debug option
> 
>  Documentation/admin-guide/kernel-parameters.txt |  3 ++-
>  arch/arm64/mm/dma-mapping.c |  3 ++-
>  arch/arm64/mm/init.c|  3 ++-
>  arch/x86/kernel/pci-swiotlb.c   |  6 ++
>  arch/x86/xen/pci-swiotlb-xen.c  |  2 +-
>  drivers/xen/swiotlb-xen.c   |  4 ++--
>  include/linux/swiotlb.h |  8 +++-
>  include/trace/events/swiotlb.h  | 17 +---
>  lib/swiotlb.c   | 26 
> +++--
>  9 files changed, 48 insertions(+), 24 deletions(-)
> 
> -- 
> 1.9.1
> 
> Gr{oetje,eeting}s,
> 
>   Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
>   -- Linus Torvalds

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Konrad Rzeszutek Wilk]

On Mon, Nov 07, 2016 at 07:57:11PM +0100, Geert Uytterhoeven wrote:
> Hi Konrad,
> 
> On Mon, Oct 31, 2016 at 6:52 PM, Konrad Rzeszutek Wilk
> <konrad.w...@oracle.com> wrote:
> > On Mon, Oct 31, 2016 at 04:45:04PM +0100, Geert Uytterhoeven wrote:
> >> On architectures like arm64, swiotlb is tied intimately to the core
> >> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
> >>
> >> To aid debugging and catch devices not supporting DMA to memory outside
> >> the 32-bit address space, add a kernel command line option
> >> "swiotlb=nobounce", which disables the use of bounce buffers.
> >> If specified, trying to map memory that cannot be used with DMA will
> >> fail, and a warning will be printed (rate-limited).
> >
> > I would make the 'swiotlb_force' an enum. And then instead of this
> > being 'nobounce' just do the inverse of 'force', that is the
> > 'noforce' would trigger this no bounce effect.
> >
> > So:
> >
> > enum {
> > NORMAL, /* Default - depending on the hardware DMA mask and 
> > such. */
> > FORCE,  /* swiotlb=force */
> > NO_FORCE,   /* swiotlb=noforce */
> 
> Fine for me, but swiotlb_force is exported to platform code. Hence all users
> should be updated?

Yeah it would have to be moved to the swiotlb.h (the enum).

Thanks!

Re: [PATCH 1/2] swiotlb: Rate-limit printing when running out of SW-IOMMU space

[Konrad Rzeszutek Wilk]

On Mon, Oct 31, 2016 at 04:45:03PM +0100, Geert Uytterhoeven wrote:
> If the system runs out of SW-IOMMU space, changes are high successive
> requests will fail, too, flooding the kernel log.  This is true
> especially for streaming DMA, which is typically used repeatedly outside
> the driver's initialization routine.  Add rate-limiting to fix this.
> 
> While at it, get rid of the open-coded dev_name() handling by using the
> appropriate dev_err_*() variant.
> 
> Signed-off-by: Geert Uytterhoeven 

applied.
> ---
>  lib/swiotlb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 22e13a0e19d76a2b..6ce764410ae475cc 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -714,8 +714,8 @@ void swiotlb_tbl_sync_single(struct device *hwdev, 
> phys_addr_t tlb_addr,
>* When the mapping is small enough return a static buffer to limit
>* the damage, or panic when the transfer is too big.
>*/
> - printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
> -"device %s\n", size, dev ? dev_name(dev) : "?");
> + dev_err_ratelimited(dev, "DMA: Out of SW-IOMMU space for %zu bytes\n",
> + size);
>  
>   if (size <= io_tlb_overflow || !do_panic)
>   return;
> -- 
> 1.9.1
> 

Re: [PATCHv6 3/8] dma-mapping: add dma_{map,unmap}_resource

[Konrad Rzeszutek Wilk]

>  
> -In some circumstances dma_map_single() and dma_map_page() will fail to create
> -a mapping. A driver can check for these errors by testing the returned
> -DMA address with dma_mapping_error(). A non-zero return value means the 
> mapping
> -could not be created and the driver should take appropriate action (e.g.
> -reduce current DMA mapping usage or delay and try again later).
> +In some circumstances dma_map_single(), dma_map_page() and dma_map_resource()
> +will fail to create a mapping. A driver can check for these errors by testing
> +the returned DMA address with dma_mapping_error(). A non-zero return value
> +means the mapping could not be created and the driver should take appropriate
> +action (e.g. reduce current DMA mapping usage or delay and try again later).

This looks like it belongs to another patch?

Re: [PATCHv6 2/8] dma-debug: add support for resource mappings

[Konrad Rzeszutek Wilk]

> +void debug_dma_map_resource(struct device *dev, phys_addr_t addr, size_t 
> size,
> + int direction, dma_addr_t dma_addr)
> +{
> + struct dma_debug_entry *entry;
> +
> + if (unlikely(dma_debug_disabled()))
> + return;
> +
> + entry = dma_entry_alloc();
> + if (!entry)
> + return;
> +
> + entry->type = dma_debug_resource;
> + entry->dev  = dev;
> + entry->pfn  = __phys_to_pfn(addr);
> + entry->offset   = addr - PHYS_PFN(entry->pfn);

Is that right? 

__phys_to_pfn(addr) is PHYS_PFN(addr), so what you get here is

addr - PHYS_PFN(PHYS_PFN(addr)) ?


> + entry->size = size;
> + entry->dev_addr = dma_addr;
> + entry->direction= direction;
> + entry->map_err_type = MAP_ERR_NOT_CHECKED;
> +
> + add_dma_entry(entry);
> +}
> +EXPORT_SYMBOL(debug_dma_map_resource);
> +