* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-25 18:31 ` [PATCH v2] " Robin Murphy
@ 2017-01-25 19:14 ` Robin Murphy
2017-01-25 19:31 ` Michael Zoran
2017-01-25 21:49 ` Aaro Koskinen
2017-01-26 12:52 ` Will Deacon
2 siblings, 1 reply; 15+ messages in thread
From: Robin Murphy @ 2017-01-25 19:14 UTC (permalink / raw)
To: will.deacon, catalin.marinas
Cc: Jisheng Zhang, arnd, konrad.wilk, aaro.koskinen, stable,
linux-rpi-kernel, linux-arm-kernel, Michael Zoran
[ +Michael - FYI this is straight on top of 4.10-rc5 ]
On 25/01/17 18:31, Robin Murphy wrote:
> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
> being initialised.
>
> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
> will only ever return the DMA-offset-adjusted physical address of the
> page passed in, thus we can report success unconditionally.
>
> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
> CC: stable@vger.kernel.org
> CC: Jisheng Zhang <jszhang@marvell.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>
> v2: Get the return value the right way round this time... After some
> careful reasoning it really is that simple.
>
> arch/arm64/mm/dma-mapping.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e04082700bb1..1ffb7d5d299a 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
> return 1;
> }
>
> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
> +{
> + if (swiotlb)
> + return swiotlb_dma_mapping_error(hwdev, addr);
> + return 0;
> +}
> +
> static struct dma_map_ops swiotlb_dma_ops = {
> .alloc = __dma_alloc,
> .free = __dma_free,
> @@ -366,7 +373,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
> .sync_sg_for_cpu = __swiotlb_sync_sg_for_cpu,
> .sync_sg_for_device = __swiotlb_sync_sg_for_device,
> .dma_supported = __swiotlb_dma_supported,
> - .mapping_error = swiotlb_dma_mapping_error,
> + .mapping_error = __swiotlb_dma_mapping_error,
> };
>
> static int __init atomic_pool_init(void)
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-25 19:14 ` Robin Murphy
@ 2017-01-25 19:31 ` Michael Zoran
0 siblings, 0 replies; 15+ messages in thread
From: Michael Zoran @ 2017-01-25 19:31 UTC (permalink / raw)
To: Robin Murphy, will.deacon, catalin.marinas
Cc: Jisheng Zhang, arnd, konrad.wilk, aaro.koskinen, stable,
linux-rpi-kernel, linux-arm-kernel
On Wed, 2017-01-25 at 19:14 +0000, Robin Murphy wrote:
> [ +Michael - FYI this is straight on top of 4.10-rc5 ]
>
> On 25/01/17 18:31, Robin Murphy wrote:
> > When bypassing SWIOTLB on small-memory systems, we need to avoid
> > calling
> > into swiotlb_dma_mapping_error() in exactly the same way as we
> > avoid
> > swiotlb_dma_supported(), because the former also relies on SWIOTLB
> > state
> > being initialised.
> >
> > Under the assumptions for which we skip SWIOTLB,
> > dma_map_{single,page}()
> > will only ever return the DMA-offset-adjusted physical address of
> > the
> > page passed in, thus we can report success unconditionally.
> >
> > Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when
> > necessary")
> > CC: stable@vger.kernel.org
> > CC: Jisheng Zhang <jszhang@marvell.com>
> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >
> > v2: Get the return value the right way round this time... After
> > some
> > careful reasoning it really is that simple.
> >
> > arch/arm64/mm/dma-mapping.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
I was able to build it and it works. Cool.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-25 18:31 ` [PATCH v2] " Robin Murphy
2017-01-25 19:14 ` Robin Murphy
@ 2017-01-25 21:49 ` Aaro Koskinen
2017-01-26 12:52 ` Will Deacon
2 siblings, 0 replies; 15+ messages in thread
From: Aaro Koskinen @ 2017-01-25 21:49 UTC (permalink / raw)
To: Robin Murphy
Cc: will.deacon, catalin.marinas, linux-arm-kernel, arnd,
linux-rpi-kernel, konrad.wilk, stable, Jisheng Zhang
Hi,
On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
> being initialised.
>
> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
> will only ever return the DMA-offset-adjusted physical address of the
> page passed in, thus we can report success unconditionally.
>
> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
> CC: stable@vger.kernel.org
> CC: Jisheng Zhang <jszhang@marvell.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Thanks,
A.
> ---
>
> v2: Get the return value the right way round this time... After some
> careful reasoning it really is that simple.
>
> arch/arm64/mm/dma-mapping.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e04082700bb1..1ffb7d5d299a 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
> return 1;
> }
>
> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
> +{
> + if (swiotlb)
> + return swiotlb_dma_mapping_error(hwdev, addr);
> + return 0;
> +}
> +
> static struct dma_map_ops swiotlb_dma_ops = {
> .alloc = __dma_alloc,
> .free = __dma_free,
> @@ -366,7 +373,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
> .sync_sg_for_cpu = __swiotlb_sync_sg_for_cpu,
> .sync_sg_for_device = __swiotlb_sync_sg_for_device,
> .dma_supported = __swiotlb_dma_supported,
> - .mapping_error = swiotlb_dma_mapping_error,
> + .mapping_error = __swiotlb_dma_mapping_error,
> };
>
> static int __init atomic_pool_init(void)
> --
> 2.11.0.dirty
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-25 18:31 ` [PATCH v2] " Robin Murphy
2017-01-25 19:14 ` Robin Murphy
2017-01-25 21:49 ` Aaro Koskinen
@ 2017-01-26 12:52 ` Will Deacon
2017-01-26 13:04 ` Michael Zoran
2017-01-26 15:20 ` Robin Murphy
2 siblings, 2 replies; 15+ messages in thread
From: Will Deacon @ 2017-01-26 12:52 UTC (permalink / raw)
To: Robin Murphy
Cc: catalin.marinas, linux-arm-kernel, arnd, aaro.koskinen,
linux-rpi-kernel, konrad.wilk, stable, Jisheng Zhang
On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
> being initialised.
>
> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
> will only ever return the DMA-offset-adjusted physical address of the
> page passed in, thus we can report success unconditionally.
>
> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
> CC: stable@vger.kernel.org
> CC: Jisheng Zhang <jszhang@marvell.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>
> v2: Get the return value the right way round this time... After some
> careful reasoning it really is that simple.
>
> arch/arm64/mm/dma-mapping.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e04082700bb1..1ffb7d5d299a 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
> return 1;
> }
>
> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
> +{
> + if (swiotlb)
> + return swiotlb_dma_mapping_error(hwdev, addr);
> + return 0;
> +}
I was about to apply this, but I'm really uncomfortable with the way that
we call into swiotlb without initialising it. For example, if somebody
passes swiotlb=noforce on the command line and all of our memory is
DMA-able, then we don't call swiotlb_init but we will leave the DMA ops
intact. On a dma_map_page, we then end up in swiotlb_map_page. If, for
some reason or another, dma_capable fails (perhaps the address is out of
range), then we call map_single which will return SWIOTLB_MAP_ERROR
and subsequently phys_to_dma(dev, io_tlb_overflow_buffer);, which is
exactly what swiotlb_dma_mapping_error checks for. Except it won't get the
chance, because our swiotlb variable is false.
I can see three ways to resolve this:
1. Revert the hack that skips SWIOTLB initialisation and pay the 64m price
(but this is configurable on the cmdline).
2. Keep the hack, but instead of skipping initialisation altogether,
automatically adjust the bounce buffer size to a single entry. This
shouldn't ever get used, but will allow the error paths to work.
3. We bite the bullet and implement some non-swiotlb DMA ops for the case
when SWIOTLB is not used.
Thoughts?
Will
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-26 12:52 ` Will Deacon
@ 2017-01-26 13:04 ` Michael Zoran
2017-01-26 15:20 ` Robin Murphy
1 sibling, 0 replies; 15+ messages in thread
From: Michael Zoran @ 2017-01-26 13:04 UTC (permalink / raw)
To: Will Deacon, Robin Murphy
Cc: Jisheng Zhang, arnd, konrad.wilk, catalin.marinas, aaro.koskinen,
stable, linux-rpi-kernel, linux-arm-kernel
On Thu, 2017-01-26 at 12:52 +0000, Will Deacon wrote:
> On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
> > When bypassing SWIOTLB on small-memory systems, we need to avoid
> > calling
> > into swiotlb_dma_mapping_error() in exactly the same way as we
> > avoid
> > swiotlb_dma_supported(), because the former also relies on SWIOTLB
> > state
> > being initialised.
> >
> > Under the assumptions for which we skip SWIOTLB,
> > dma_map_{single,page}()
> > will only ever return the DMA-offset-adjusted physical address of
> > the
> > page passed in, thus we can report success unconditionally.
> >
> > Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when
> > necessary")
> > CC: stable@vger.kernel.org
> > CC: Jisheng Zhang <jszhang@marvell.com>
> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >
> > v2: Get the return value the right way round this time... After
> > some
> > careful reasoning it really is that simple.
> >
> > arch/arm64/mm/dma-mapping.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-
> > mapping.c
> > index e04082700bb1..1ffb7d5d299a 100644
> > --- a/arch/arm64/mm/dma-mapping.c
> > +++ b/arch/arm64/mm/dma-mapping.c
> > @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct
> > device *hwdev, u64 mask)
> > return 1;
> > }
> >
> > +static int __swiotlb_dma_mapping_error(struct device *hwdev,
> > dma_addr_t addr)
> > +{
> > + if (swiotlb)
> > + return swiotlb_dma_mapping_error(hwdev, addr);
> > + return 0;
> > +}
>
> I was about to apply this, but I'm really uncomfortable with the way
> that
> we call into swiotlb without initialising it. For example, if
> somebody
> passes swiotlb=noforce on the command line and all of our memory is
> DMA-able, then we don't call swiotlb_init but we will leave the DMA
> ops
> intact. On a dma_map_page, we then end up in swiotlb_map_page. If,
> for
> some reason or another, dma_capable fails (perhaps the address is out
> of
> range), then we call map_single which will return SWIOTLB_MAP_ERROR
> and subsequently phys_to_dma(dev, io_tlb_overflow_buffer);, which is
> exactly what swiotlb_dma_mapping_error checks for. Except it won't
> get the
> chance, because our swiotlb variable is false.
>
> I can see three ways to resolve this:
>
> 1. Revert the hack that skips SWIOTLB initialisation and pay the 64m
> price
> (but this is configurable on the cmdline).
>
> 2. Keep the hack, but instead of skipping initialisation altogether,
> automatically adjust the bounce buffer size to a single entry.
> This
> shouldn't ever get used, but will allow the error paths to work.
>
> 3. We bite the bullet and implement some non-swiotlb DMA ops for the
> case
> when SWIOTLB is not used.
>
> Thoughts?
>
> Will
>
I'm learning about the DMA APIs since I'm new here and just trying to
understand...
On the RPI 3, all the memory is DMA able if I understand. All the DMA
APIs needs to do is just flush the various caches.
To keep things as simple as possible, why not just have a seperate dma-
ops table for the simple case where all the functions are no-ops except
for the needed cache flushing?
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-26 12:52 ` Will Deacon
2017-01-26 13:04 ` Michael Zoran
@ 2017-01-26 15:20 ` Robin Murphy
2017-01-26 20:35 ` Aaro Koskinen
2017-01-27 9:53 ` Will Deacon
1 sibling, 2 replies; 15+ messages in thread
From: Robin Murphy @ 2017-01-26 15:20 UTC (permalink / raw)
To: Will Deacon
Cc: catalin.marinas, linux-arm-kernel, arnd, aaro.koskinen,
linux-rpi-kernel, konrad.wilk, stable, Jisheng Zhang
On 26/01/17 12:52, Will Deacon wrote:
> On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
>> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
>> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
>> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
>> being initialised.
>>
>> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
>> will only ever return the DMA-offset-adjusted physical address of the
>> page passed in, thus we can report success unconditionally.
>>
>> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
>> CC: stable@vger.kernel.org
>> CC: Jisheng Zhang <jszhang@marvell.com>
>> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v2: Get the return value the right way round this time... After some
>> careful reasoning it really is that simple.
>>
>> arch/arm64/mm/dma-mapping.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index e04082700bb1..1ffb7d5d299a 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>> return 1;
>> }
>>
>> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
>> +{
>> + if (swiotlb)
>> + return swiotlb_dma_mapping_error(hwdev, addr);
>> + return 0;
>> +}
>
> I was about to apply this, but I'm really uncomfortable with the way that
> we call into swiotlb without initialising it. For example, if somebody
> passes swiotlb=noforce on the command line and all of our memory is
> DMA-able, then we don't call swiotlb_init but we will leave the DMA ops
> intact. On a dma_map_page, we then end up in swiotlb_map_page. If, for
> some reason or another, dma_capable fails (perhaps the address is out of
> range), then we call map_single which will return SWIOTLB_MAP_ERROR
> and subsequently phys_to_dma(dev, io_tlb_overflow_buffer);, which is
> exactly what swiotlb_dma_mapping_error checks for. Except it won't get the
> chance, because our swiotlb variable is false.
Right. Or to look at it another way (in isolation), this patch as-is at
least moves us from a real observed problem to a theoretical problem
involving a theoretical device :P
However, I do agree that skirting danger by calling into uninitialised
SWIOTLB state on the assumption that it 'should' be OK is grotty as
hell, and having had a closer look I found another sweet nugget - if
someone calls dma_alloc_coherent() in non-blocking context, for a
sufficiently large order that the initial __get_free_pages() call from
swiotlb_alloc_coherent() fails (hey, small-memory systems *are* going to
suffer fragmentation), then we'll end up poking around in yet more
uninitialised internals trying to allocate out of the non-existent
bounce buffer.
> I can see three ways to resolve this:
>
> 1. Revert the hack that skips SWIOTLB initialisation and pay the 64m price
> (but this is configurable on the cmdline).
>
> 2. Keep the hack, but instead of skipping initialisation altogether,
> automatically adjust the bounce buffer size to a single entry. This
> shouldn't ever get used, but will allow the error paths to work.
Since the hack's been present in two kernel releases already, one of
them long-term stable, I suspect 2 is the only viable option of those,
although it does look to require juggling two different SWIOTLB init
functions, which appears fiddly at a glance.
> 3. We bite the bullet and implement some non-swiotlb DMA ops for the case
> when SWIOTLB is not used.
I'd already started thinking along those lines in the process of writing
this patch - I'm happy to take that on, although it might be a wee bit
tight for 4.11 now.
Robin.
>
> Thoughts?
>
> Will
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-26 15:20 ` Robin Murphy
@ 2017-01-26 20:35 ` Aaro Koskinen
2017-01-27 9:53 ` Will Deacon
1 sibling, 0 replies; 15+ messages in thread
From: Aaro Koskinen @ 2017-01-26 20:35 UTC (permalink / raw)
To: Robin Murphy
Cc: Will Deacon, Jisheng Zhang, arnd, konrad.wilk, catalin.marinas,
stable, linux-rpi-kernel, linux-arm-kernel
Hi,
On Thu, Jan 26, 2017 at 03:20:47PM +0000, Robin Murphy wrote:
> hell, and having had a closer look I found another sweet nugget - if
> someone calls dma_alloc_coherent() in non-blocking context, for a
> sufficiently large order that the initial __get_free_pages() call from
> swiotlb_alloc_coherent() fails (hey, small-memory systems *are* going to
> suffer fragmentation), then we'll end up poking around in yet more
> uninitialised internals trying to allocate out of the non-existent
> bounce buffer.
I think this shouldn't happen anymore after 524dabe1c68e ("arm64: Fix
swiotlb fallback allocation").
A.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB
2017-01-26 15:20 ` Robin Murphy
2017-01-26 20:35 ` Aaro Koskinen
@ 2017-01-27 9:53 ` Will Deacon
1 sibling, 0 replies; 15+ messages in thread
From: Will Deacon @ 2017-01-27 9:53 UTC (permalink / raw)
To: Robin Murphy
Cc: catalin.marinas, linux-arm-kernel, arnd, aaro.koskinen,
linux-rpi-kernel, konrad.wilk, stable, Jisheng Zhang
On Thu, Jan 26, 2017 at 03:20:47PM +0000, Robin Murphy wrote:
> On 26/01/17 12:52, Will Deacon wrote:
> > 3. We bite the bullet and implement some non-swiotlb DMA ops for the case
> > when SWIOTLB is not used.
>
> I'd already started thinking along those lines in the process of writing
> this patch - I'm happy to take that on, although it might be a wee bit
> tight for 4.11 now.
Ok, so I've merged this patch as-is for 4.11 because it is an improvement
over what we had before. Let's look at using separate ops in the long run.
Will
^ permalink raw reply [flat|nested] 15+ messages in thread