* [PATCH v6] soc: qcom: add l2 cache perf events driver
From: Mark Rutland @ 2016-10-04 15:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474492374-12140-1-git-send-email-nleeder@codeaurora.org>
Hi Neil,
On Wed, Sep 21, 2016 at 05:12:54PM -0400, Neil Leeder wrote:
> Adds perf events support for L2 cache PMU.
>
> The L2 cache PMU driver is named 'l2cache_0' and can be used
> with perf events to profile L2 events such as cache hits
> and misses.
>
> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> ---
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/perf_event_l2.c | 948 +++++++++++++++++++++++++++++++++++++++
> include/linux/cpuhotplug.h | 1 +
> 4 files changed, 959 insertions(+)
> create mode 100644 drivers/soc/qcom/perf_event_l2.c
Apologies for the delay; this has been on my todo list, but I've been a
little distracted and haven't had the time necessary to devote to this.
It's somewhat unusual given the constraint logic and the percpu uncore
component, so there's more to consider than usual.
At a high level, this will need to be moved to drivers/perf/, per [1].
Can you move the driver there, and post the result atop of v4.8-rc1 at
the end of the merge window? Until then, I can't guarantee that I'll
have the time to look at this.
Can you also give Vince's perf fuzzer [2] a spin against the driver? I
can't recall if we covered that previously, and in practice it's found a
number of issues in drivers that have otherwise looked fine. If you've
done so, it'd be worth noting in the cover.
Thanks,
Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-September/457188.html
[2] https://github.com/deater/perf_event_tests
^ permalink raw reply
* [PATCH] dmaengine: coh901318: fix integer overflow when shifting more than 32 places
From: Joe Perches @ 2016-10-04 16:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161004153645.GG2467@localhost>
On Tue, 2016-10-04 at 21:06 +0530, Vinod Koul wrote:
> On Tue, Oct 04, 2016 at 02:23:51PM +0200, Linus Walleij wrote:
> > On Thu, Sep 29, 2016 at 8:06 PM, Joe Perches <joe@perches.com> wrote:
> > > On Thu, 2016-09-29 at 18:57 +0100, Colin King wrote:
> > > > Currently U300_DMA_CHANNELS is set to 40, meaning that the shift of 1 can
> > > > be more than 32 places, which leads to a 32 bit integer overflow. Fix this
> > > > by casting 1 to a u64 (the same type as started_channels) before shifting
> > > > it.
> > > trivia:
> > > > diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
> > > []
> > > > @@ -1353,7 +1353,7 @@ static ssize_t coh901318_debugfs_read(struct file *file, char __user *buf,
> > > > ? tmp += sprintf(tmp, "DMA -- enabled dma channels\n");
> > > >
> > > > ? for (i = 0; i < U300_DMA_CHANNELS; i++)
> > > > - if (started_channels & (1 << i))
> > > > + if (started_channels & ((u64)1 << i))
> > > Using
> > > ? if (started_channels & (1ULL << i))
> > > would be more common.
> > Even better (IMO):
> > #include <linux/bitops.h>
> > if (started_channels & BIT(i))
> > Apparently code is there to avoid the bit 31 problem, mea culpa.
> I have already applied this one, so feel free to send this as an update :)
BIT_ULL as it still needs to be u64 not unsigned long.
But if a change is really desired, please use it
consistently in the entire file and not just this
instance.
^ permalink raw reply
* [PATCH 06/14] ASoC: Add sun8i digital audio codec
From: Maxime Ripard @ 2016-10-04 16:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161004144008.0d07d18c@free-electrons.com>
Hi,
On Tue, Oct 04, 2016 at 02:40:08PM +0200, Thomas Petazzoni wrote:
> > + scodec->clk_apb = devm_clk_get(&pdev->dev, "apb");
> > + if (IS_ERR(scodec->clk_apb)) {
> > + dev_err(&pdev->dev, "Failed to get the apb clock\n");
> > + return PTR_ERR(scodec->clk_apb);
> > + }
> > + if (clk_prepare_enable(scodec->clk_apb))
> > + pr_err("err:open failed;\n");
>
> Ditto. + unprepare/disable the previous clock.
Ideally, that would be even not be part of the runtime_pm
hooks. Ideally, that would be great if that driver supports it.
We'll have to go through all the drivers to support it, that would be
one less to do (and ASoC makes it very easy, you can have a look at
the sun4i-i2s driver).
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161004/3a347edd/attachment.sig>
^ permalink raw reply
* [PATCH 06/14] ASoC: Add sun8i digital audio codec
From: Maxime Ripard @ 2016-10-04 16:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <85cbd9926e52d0aa03f6bbfd8794373d8db491e0.1475571575.git.mylene.josserand@free-electrons.com>
Hi,
> +static const struct of_device_id sun8i_codec_of_match[] = {
> + { .compatible = "allwinner,sun8i-a33-codec" },
> + { .compatible = "allwinner,sun8i-a23-codec" },
I thought that the A23 and A33 had different codecs? In that case, it
wouldn't be a good assumption to make
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
> +
> +static struct platform_driver sun8i_codec_driver = {
> + .driver = {
> + .name = "sun8i-codec",
> + .owner = THIS_MODULE,
> + .of_match_table = sun8i_codec_of_match,
> + },
> + .probe = sun8i_codec_probe,
> + .remove = sun8i_codec_remove,
> +};
> +module_platform_driver(sun8i_codec_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A33 (sun8i) codec driver");
> +MODULE_AUTHOR("huanxin<huanxin@reuuimllatech.com>");
Those obfuscated email adresses are not really helpful :)
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161004/b93afd78/attachment.sig>
^ permalink raw reply
* [PATCH 09/14] dt-bindings: sound: Add sun8i codec documentation
From: Maxime Ripard @ 2016-10-04 16:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a5b0bdc6b50dfcc2dc0145d00edd08fee3ce7764.1475571575.git.mylene.josserand@free-electrons.com>
On Tue, Oct 04, 2016 at 11:46:22AM +0200, Myl?ne Josserand wrote:
> Add the documentation for dt-binding of the digital audio codec driver
> for sun8i SoC.
>
> Signed-off-by: Myl?ne Josserand <mylene.josserand@free-electrons.com>
> ---
> .../devicetree/bindings/sound/sun8i-codec.txt | 24 ++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/sun8i-codec.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/sun8i-codec.txt b/Documentation/devicetree/bindings/sound/sun8i-codec.txt
> new file mode 100644
> index 0000000..1808869
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/sun8i-codec.txt
> @@ -0,0 +1,24 @@
> +* Allwinner A23/A33 Codec
> +
> +Required properties:
> +- compatible: must be either "allwinner,sun4i-a23-codec" or
> + "allwinner,sun7i-a33-codec"
Copy and paste issue ? :)
One compatible by line is usually favored, since when you'll add new
compatibles, you don't have to modify the context.xs
> +- reg: must contain the registers location and length
> +- interrupts: must contain the codec interrupt
> +- clocks: a list of phandle + clock-specifer pairs, one for each entry
> + in clock-names.
> +- clock-names: should contain followings:
> + - "apb": the parent APB clock for this controller
> + - "codec": the parent module clock
We're usually calling them "bus" and "mod".
> +
> +Example:
> +codec: codec at 01c22e00 {
> + #sound-dai-cells = <0>;
> + compatible = "allwinner,sun8i-a33-codec";
> + reg = <0x01c22e00 0x400>; /* SUNXI_AUDIO_PBASE + 0x200 */
> + reg-names = "audio";
You don't define reg-names in your bindings, while your code relies on
it. It isn't really needed, since you have only one couple of base +
size, so it should just go away.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161004/dc85a491/attachment.sig>
^ permalink raw reply
* [PATCH 08/14] dt-bindings: sound: Add sun8i analog codec documentation
From: Maxime Ripard @ 2016-10-04 16:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <06ea14939405c3eb0fb9be655d26ee564a04a53d.1475571575.git.mylene.josserand@free-electrons.com>
Hi,
On Tue, Oct 04, 2016 at 11:46:21AM +0200, Myl?ne Josserand wrote:
> Add the documentation for dt-binding of the analog audiocodec
> driver for SUN8I SoC.
>
> Signed-off-by: Myl?ne Josserand <mylene.josserand@free-electrons.com>
> ---
> .../devicetree/bindings/sound/sun8i-codec-analog.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt b/Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt
> new file mode 100644
> index 0000000..a03ec20
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt
> @@ -0,0 +1,20 @@
> +* Allwinner A23/A33 Analog Codec
> +
> +This codec must be handled as a PRCM subnode.
Like Mark was saying, you should probably reference the sun6i-prcm.txt
binding here
> +Required properties:
> +- compatible: must be either "allwinner,sun8i-codec-analog"
Our compatible prefix is <family>-<soc>, and using the older SoC that
introduced that block.
In this case, that would be sun6i-a31, I think?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161004/efe56436/attachment.sig>
^ permalink raw reply
* [PATCH v6] soc: qcom: add l2 cache perf events driver
From: Neil Leeder @ 2016-10-04 16:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161004155304.GB29008@leverpostej>
On 10/4/2016 11:53 AM, Mark Rutland wrote:
> Hi Neil,
>
> On Wed, Sep 21, 2016 at 05:12:54PM -0400, Neil Leeder wrote:
>> Adds perf events support for L2 cache PMU.
>>
>> The L2 cache PMU driver is named 'l2cache_0' and can be used
>> with perf events to profile L2 events such as cache hits
>> and misses.
>>
>> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
>> ---
>
>> drivers/soc/qcom/Kconfig | 9 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/perf_event_l2.c | 948 +++++++++++++++++++++++++++++++++++++++
>> include/linux/cpuhotplug.h | 1 +
>> 4 files changed, 959 insertions(+)
>> create mode 100644 drivers/soc/qcom/perf_event_l2.c
>
> Apologies for the delay; this has been on my todo list, but I've been a
> little distracted and haven't had the time necessary to devote to this.
> It's somewhat unusual given the constraint logic and the percpu uncore
> component, so there's more to consider than usual.
>
> At a high level, this will need to be moved to drivers/perf/, per [1].
>
> Can you move the driver there, and post the result atop of v4.8-rc1 at
> the end of the merge window? Until then, I can't guarantee that I'll
> have the time to look at this.
>
> Can you also give Vince's perf fuzzer [2] a spin against the driver? I
> can't recall if we covered that previously, and in practice it's found a
> number of issues in drivers that have otherwise looked fine. If you've
> done so, it'd be worth noting in the cover.
>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-September/457188.html
> [2] https://github.com/deater/perf_event_tests
>
Thanks Mark. I'll move it, rebase on 4.9-rc1 and run perf fuzzer.
Neil
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] Adding missing features of Coresight PTM components
From: Mathieu Poirier @ 2016-10-04 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <583ccfa4-ee4e-561a-5480-d0f243194ee3@supelec.fr>
On 29 September 2016 at 04:16, Muhammad Abdul WAHAB
<muhammadabdul.wahab@supelec.fr> wrote:
> In the current driver for Coresight components, two features of PTM
> components are missing:
>
> 1. Branch Broadcasting (present also in ETM but called Branch Output)
> 2. Return Stack (only present in PTM v1.0 and PTMv1.1)
>
> These features can be added simply to the code using `mode` field of
> `etm_config` struct.
The rule of thumb when writing a patch description is to specify "why"
rather than "what" you are doing. The above describe the why alright
and should be kept. Everything below until your SOB describe what you
are doing and should be removed.
>
> ## Modifications in coresight-etm.h
> Two defines are added in register definition part of `coresight_etm.h` file
> that corresponds to the bitfield of these options. Two defines for mode
> field are added as well in the same file. The `ETM_MODE_ALL` field is
> modified accordingly.
>
> ## Modifs in coresight-etm3x-sysfs.c
> As the return stack feature is only available in PTM components, a test is
> made to make sure that for ETM components, this part is never executed.
> In addition, these two options (Branch Broadcasting and Return Stack) must
> not be enabled at the same time because the obtained trace is unpredictable
> in this case (as described in
> [PFT architecture v1.1](https://goo.gl/lZ72R1)). For now, a warning is
> shown to alert user that the behavior is unpredictable. However, only one
> option could be allowed to trace. The user need to change configuration
> of PTM.
>
> To enable these features, the correct value should be written in `mode`
> file. The values are :
>
> 1. Branch Broadcasting (1 << 5)
> 2. Return Stack (1 << 6)
>
> Signed-off-by: Muhammad Abdul Wahab <muhammadabdul.wahab@centralesupelec.fr>
> ---
> ### Purpose
>
> 1. **Branch Broadcast** : The branch broadcast feature is present in ETM
> components as well and is called Branch output. It allows to retrieve
> addresses for direct branch addresses alongside the indirect branch
> addresses. For example, it could be useful in cases when tracing without
> source code.
> 2. **Return Stack** : The return stack option allows to retrieve the return
> addresses of function calls. It can be useful to avoid CRA
> (Code Reuse Attacks) by keeping a shadowstack.
The above should go in the patch description.
>
> ### Testing
>
> The trace sink components need to be enabled by accessing through sys file
> system.
>
> echo 1 > /sys/bus/coresight/devices/@addr.etb/enable\_sink
>
> Then enable the CS source component:
>
> echo 1 > /sys/bus/coresight/devices/@addr.ptm/enable\_source
>
> By default, CS Source components are configured to trace the kernel.
>
> Then the trace can be read by dumping ETB.
>
> dd if=/dev/@addr.etb of=trace_kernel.bin
>
> The trace can be visualized by
>
> hexdump -C trace_kernel.bin
>
> Or stored using
>
> hexdump -C trace_kernel.bin > trace_kernel.txt
>
> The trace need to be decoded to be readable.All these above steps can now
> be performed with Perf Library which was not available at the time I was
> playing with DT entries.
All of the above is not needed.
>
> diff -uprN -X linux-4.7-vanilla/Documentation/dontdiff
> linux-4.7-vanilla/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> linux-4.7/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> --- linux-4.7-vanilla/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> 2016-07-24 21:23:50.000000000 +0200
> +++ linux-4.7/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> 2016-09-28 15:36:39.886542702 +0200
> @@ -145,7 +145,7 @@ static ssize_t mode_store(struct device
> goto err_unlock;
> }
> config->ctrl |= ETMCR_STALL_MODE;
> - } else
> + } else
There is indeed an indentation problem here but it can't be fixed in
this patch. Please do another patch for this.
> config->ctrl &= ~ETMCR_STALL_MODE;
>
> if (config->mode & ETM_MODE_TIMESTAMP) {
> @@ -163,6 +163,20 @@ static ssize_t mode_store(struct device
> else
> config->ctrl &= ~ETMCR_CTXID_SIZE;
>
> + if (config->mode & ETM_MODE_BBROAD)
> + config->ctrl |= ETMCR_BRANCH_BROADCAST;
> + else
> + config->ctrl &= ~ETMCR_BRANCH_BROADCAST;
> +
> + if (drvdata->arch == (PFT_ARCH_V1_0 | PFT_ARCH_V1_1)) {
The sysFS mode users can do what they want, including configurations
not supported by the HW. There are so many rules and exception that
adding a check for every one of they doesn't scale up. I suggest to
remove the above check.
> + if (config->mode & ETM_MODE_RET_STACK) {
> + if (config->mode & ETM_MODE_BBROAD)
> + dev_warn(drvdata->dev, "behavior is unpredictable\n");
> + config->ctrl |= ETMCR_RETURN_STACK_EN;
> + } else
> + config->ctrl &= ~ETMCR_RETURN_STACK_EN;
> + }
> +
> if (config->mode & (ETM_MODE_EXCL_KERN | ETM_MODE_EXCL_USER))
> etm_config_trace_mode(config);
>
> diff -uprN -X linux-4.7-vanilla/Documentation/dontdiff
> linux-4.7-vanilla/drivers/hwtracing/coresight/coresight-etm.h
> linux-4.7/drivers/hwtracing/coresight/coresight-etm.h
> --- linux-4.7-vanilla/drivers/hwtracing/coresight/coresight-etm.h
> 2016-07-24 21:23:50.000000000 +0200
> +++ linux-4.7/drivers/hwtracing/coresight/coresight-etm.h 2016-09-28
> 15:35:59.862544418 +0200
> @@ -89,11 +89,13 @@
> /* ETMCR - 0x00 */
> #define ETMCR_PWD_DWN BIT(0)
> #define ETMCR_STALL_MODE BIT(7)
> +#define ETMCR_BRANCH_BROADCAST BIT(8)
> #define ETMCR_ETM_PRG BIT(10)
> #define ETMCR_ETM_EN BIT(11)
> #define ETMCR_CYC_ACC BIT(12)
> #define ETMCR_CTXID_SIZE (BIT(14)|BIT(15))
> #define ETMCR_TIMESTAMP_EN BIT(28)
> +#define ETMCR_RETURN_STACK_EN BIT(29) /* PTM v1.0 & PTM v1.1 */
> /* ETMCCR - 0x04 */
> #define ETMCCR_FIFOFULL BIT(23)
> /* ETMPDCR - 0x310 */
> @@ -110,8 +112,11 @@
> #define ETM_MODE_STALL BIT(2)
> #define ETM_MODE_TIMESTAMP BIT(3)
> #define ETM_MODE_CTXID BIT(4)
> +#define ETM_MODE_BBROAD BIT(5)
> +#define ETM_MODE_RET_STACK BIT(6)
> #define ETM_MODE_ALL (ETM_MODE_EXCLUDE | ETM_MODE_CYCACC | \
> ETM_MODE_STALL | ETM_MODE_TIMESTAMP | \
> + ETM_MODE_BBROAD | ETM_MODE_RET_STACK | \
> ETM_MODE_CTXID | ETM_MODE_EXCL_KERN | \
> ETM_MODE_EXCL_USER)
You patch doesn't apply on my tree. Please use "git format-patch" for
your next submission. Last but not least the email address in the
"from:" part of the submission doesn't match the one in the SOB - they
have to be similar.
Thanks,
Mathieu
^ permalink raw reply
* [PATCH 10/14] dt-bindings: sound: Add sun8i audio card documentation
From: Maxime Ripard @ 2016-10-04 16:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c317ce9f180a24611e79086944feca00d416b7b4.1475571575.git.mylene.josserand@free-electrons.com>
On Tue, Oct 04, 2016 at 11:46:23AM +0200, Myl?ne Josserand wrote:
> Add the documentation for dt-binding of the audio card driver
> for sun8i SoC.
>
> Signed-off-by: Myl?ne Josserand <mylene.josserand@free-electrons.com>
> ---
> Documentation/devicetree/bindings/sound/sun8i-audio.txt | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/sun8i-audio.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/sun8i-audio.txt b/Documentation/devicetree/bindings/sound/sun8i-audio.txt
> new file mode 100644
> index 0000000..2403983
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/sun8i-audio.txt
> @@ -0,0 +1,17 @@
> +* Allwinner A23/A33 audio card
> +
> +This binding implements the A33 audio card.
> +
> +Required properties:
> +- compatible: must be "allwinner,sun8i-audio"
> +- allwinner,audio-codec: must have the phandle of the audio codec
> + ("sun8i-a33-codec", for example).
> +- allwinner,i2s-controller: must have the phandle of the DAI
> + ("allwinner,sun4i-a10-i2s", for example)
You should probably have a link to the PRCM too, instead of relying on
the name of the device in your card, which is quite fragile.
Also, I'm wondering, shouldn't all these nodes be part of a single
MFD? They share the same address space (even though it's split
nicely), the same clocks, and really are just one big device. Chen-Yu,
Mark, any opinion?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161004/998b2002/attachment.sig>
^ permalink raw reply
* [PATCH 01/14] dma: sun6i-dma: Add burst case of 4
From: Maxime Ripard @ 2016-10-04 16:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161004124011.d7f5754a082d5f17d5185fc4@free.fr>
On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> On Tue, 4 Oct 2016 11:46:14 +0200
> Myl?ne Josserand <mylene.josserand@free-electrons.com> wrote:
>
> > Add the case of a burst of 4 which is handled by the SoC.
> >
> > Signed-off-by: Myl?ne Josserand <mylene.josserand@free-electrons.com>
> > ---
> > drivers/dma/sun6i-dma.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 8346199..0485204 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > switch (maxburst) {
> > case 1:
> > return 0;
> > + case 4:
> > + return 1;
> > case 8:
> > return 2;
> > default:
> > --
> > 2.9.3
>
> This patch has already been rejected by Maxime in the threads
> http://www.spinics.net/lists/dmaengine/msg08610.html
> and
> http://www.spinics.net/lists/dmaengine/msg08719.html
>
> I hope you will find the way he wants for this maxburst to be added.
I was talking about something along these lines (not tested):
-------8<---------
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 83461994e418..573ac4608293 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
switch (maxburst) {
case 1:
return 0;
+ case 4:
+ return 1;
case 8:
return 2;
default:
@@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device *pdev)
sdc->slave.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+ sdc->slave.dst_bursts = BIT(1) | BIT(8);
+ sdc->slave.src_bursts = BIT(1) | BIT(8);
sdc->slave.directions = BIT(DMA_DEV_TO_MEM) |
BIT(DMA_MEM_TO_DEV);
sdc->slave.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
sdc->slave.dev = &pdev->dev;
+ if (of_device_is_compatible(pdev->dev.of_node,
+ "allwinner,sun8i-h3-dma")) {
+ sdc->slave.dst_bursts |= BIT(4);
+ sdc->slave.src_bursts |= BIT(4);
+ }
+
sdc->pchans = devm_kcalloc(&pdev->dev, sdc->cfg->nr_max_channels,
sizeof(struct sun6i_pchan), GFP_KERNEL);
if (!sdc->pchans)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cc535a478bae..f7bbec24bb58 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -673,6 +673,8 @@ struct dma_filter {
* each type of direction, the dma controller should fill (1 <<
* <TYPE>) and same should be checked by controller as well
* @max_burst: max burst capability per-transfer
+ * @dst_bursts: bitfield of the available burst sizes for the destination
+ * @src_bursts: bitfield of the available burst sizes for the source
* @residue_granularity: granularity of the transfer residue reported
* by tx_status
* @device_alloc_chan_resources: allocate resources and return the
@@ -800,6 +802,14 @@ struct dma_device {
static inline int dmaengine_slave_config(struct dma_chan *chan,
struct dma_slave_config *config)
{
+ if (config->src_maxburst && config->device->src_bursts &&
+ !(BIT(config->src_maxburst) & config->device->src_bursts))
+ return -EINVAL;
+
+ if (config->dst_maxburst && config->device->dst_bursts &&
+ !(BIT(config->dst_maxburst) & config->device->dst_bursts))
+ return -EINVAL;
+
if (chan->device->device_config)
return chan->device->device_config(chan, config);
-------8<------------
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161004/925068da/attachment.sig>
^ permalink raw reply related
* [PATCH V3 0/8] IOMMU probe deferral support
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
Initial post from Laurent Pinchart[1]. This is
series calls the dma ops configuration for the devices
at a generic place so that it works for all busses.
The dma_configure_ops for a device is now called during
the device_attach callback just before the probe of the
bus/driver is called. Similarly dma_deconfigure is called during
device/driver_detach path.
pci_bus_add_devices (platform/amba)(_device_create/driver_register)
| |
pci_bus_add_device (device_add/driver_register)
| |
device_attach device_initial_probe
| |
__device_attach_driver __device_attach_driver
|
driver_probe_device
|
really_probe
|
dma_configure
Similarly on the device/driver_unregister path __device_release_driver is
called which inturn calls dma_deconfigure.
If the ACPI bus code follows the same, we can add acpi_dma_configure
at the same place as of_dma_configure.
This series is based on the recently merged Generic DT bindings for
PCI IOMMUs and ARM SMMU from Robin Murphy robin.murphy at arm.com [2]
This time tested this with platform and pci device for probe deferral
and reprobe on arm64 based platform. There is an issue on the cleanup
path for arm64 though, where there is WARN_ON if the dma_ops is reset while
device is attached to an domain in arch_teardown_dma_ops.
But with iommu_groups created from the iommu driver, the device is always
attached to a domain/default_domain. So so the WARN has to be removed/handled
probably.
Previous post of this series [3].
[V3]
* Removed the patch to split dma_masks/dma_ops configuration separately
based on review comments that both masks and ops are required only
during the device probe time.
* Reworked the series based on Generic DT bindings series [2].
* Added call to iommu's remove_device in the cleanup path for arm and arm64.
* Removed the notifier trick in arm64 to handle early device registration.
* Added reset of dma_ops in cleanup path for arm based on comments.
* Fixed the pci_iommu_configure path and tested with PCI device as well.
* Fixed a bug to return the correct iommu_ops from patch 7 [4] in last post.
* Fixed few other cosmetic comments.
[V2]
* Updated the Initial post to call dma_configure/deconfigure from generic code
* Added iommu add_device callback from of_iommu_configure path
[V1]
* Initial post
[1] http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013016.html
[2] http://www.spinics.net/lists/devicetree/msg142943.html
[3] https://www.mail-archive.com/iommu at lists.linux-foundation.org/msg13941.html
[4] https://www.mail-archive.com/iommu at lists.linux-foundation.org/msg13940.html
Laurent Pinchart (4):
arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
of: dma: Move range size workaround to of_dma_get_range()
of: dma: Make of_dma_deconfigure() public
iommu: of: Handle IOMMU lookup failure with deferred probing or error
Sricharan R (4):
drivers: platform: Configure dma operations at probe time
arm: dma-mapping: Reset the device's dma_ops
arm/arm64: dma-mapping: Call iommu's remove_device callback during
device detach
arm64: dma-mapping: Remove the notifier trick to handle early setting
of dma_ops
arch/arm/mm/dma-mapping.c | 18 ++++++++
arch/arm64/mm/dma-mapping.c | 107 +++++---------------------------------------
drivers/base/dd.c | 10 +++++
drivers/base/dma-mapping.c | 11 +++++
drivers/iommu/of_iommu.c | 47 +++++++++++++++++--
drivers/of/address.c | 20 ++++++++-
drivers/of/device.c | 34 +++++++-------
drivers/of/platform.c | 9 ----
drivers/pci/probe.c | 5 +--
include/linux/dma-mapping.h | 3 ++
include/linux/of_device.h | 7 ++-
11 files changed, 138 insertions(+), 133 deletions(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* [PATCH V3 1/8] arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
The arch_setup_dma_ops() function is in charge of setting dma_ops with a
call to set_dma_ops(). set_dma_ops() is also called from
- highbank and mvebu bus notifiers
- dmabounce (to be replaced with swiotlb)
- arm_iommu_attach_device
(arm_iommu_attach_device is itself called from IOMMU and bus master
device drivers)
To allow the arch_setup_dma_ops() call to be moved from device add time
to device probe time we must ensure that dma_ops already setup by any of
the above callers will not be overridden.
Aftering replacing dmabounce with swiotlb, converting IOMMU drivers to
of_xlate and taking care of highbank and mvebu, the workaround should be
removed.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
arch/arm/mm/dma-mapping.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c6834c0..dde6514 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2322,6 +2322,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
struct dma_map_ops *dma_ops;
dev->archdata.dma_coherent = coherent;
+
+ /*
+ * Don't override the dma_ops if they have already been set. Ideally
+ * this should be the only location where dma_ops are set, remove this
+ * check when all other callers of set_dma_ops will have disappeared.
+ */
+ if (dev->archdata.dma_ops)
+ return;
+
if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
dma_ops = arm_get_iommu_dma_map_ops(coherent);
else
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V3 2/8] of: dma: Move range size workaround to of_dma_get_range()
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Invalid dma-ranges values should be worked around when retrieving the
DMA range in of_dma_get_range(), not by all callers of the function.
This isn't much of a problem now that we have a single caller, but that
situation will change when moving DMA configuration to device probe
time.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/of/address.c | 20 ++++++++++++++++++--
drivers/of/device.c | 15 ---------------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903..6aeb816 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -819,8 +819,8 @@ EXPORT_SYMBOL(of_io_request_and_map);
* CPU addr (phys_addr_t) : pna cells
* size : nsize cells
*
- * It returns -ENODEV if "dma-ranges" property was not found
- * for this device in DT.
+ * Return 0 on success, -ENODEV if the "dma-ranges" property was not found for
+ * this device in DT, or -EINVAL if the CPU address or size is invalid.
*/
int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size)
{
@@ -880,6 +880,22 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
*dma_addr = dmaaddr;
*size = of_read_number(ranges + naddr + pna, nsize);
+ /*
+ * DT nodes sometimes incorrectly set the size as a mask. Work around
+ * those incorrect DT by computing the size as mask + 1.
+ */
+ if (*size & 1) {
+ pr_warn("%s: size 0x%llx for dma-range in node(%s) set as mask\n",
+ __func__, *size, np->full_name);
+ *size = *size + 1;
+ }
+
+ if (!*size) {
+ pr_err("%s: invalid size zero for dma-range in node(%s)\n",
+ __func__, np->full_name);
+ ret = -EINVAL;
+ goto out;
+ }
pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
*dma_addr, *paddr, *size);
diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad..d9898d9 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -110,21 +110,6 @@ void of_dma_configure(struct device *dev, struct device_node *np)
size = dev->coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);
-
- /*
- * Add a work around to treat the size as mask + 1 in case
- * it is defined in DT as a mask.
- */
- if (size & 1) {
- dev_warn(dev, "Invalid size 0x%llx for dma-range\n",
- size);
- size = size + 1;
- }
-
- if (!size) {
- dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
- return;
- }
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V3 3/8] of: dma: Make of_dma_deconfigure() public
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
As part of moving DMA initializing to probe time the
of_dma_deconfigure() function will need to be called from different
source files. Make it public and move it to drivers/of/device.c where
the of_dma_configure() function is.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/of/device.c | 12 ++++++++++++
drivers/of/platform.c | 5 -----
include/linux/of_device.h | 3 +++
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index d9898d9..1c843e2 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -136,6 +136,18 @@ void of_dma_configure(struct device *dev, struct device_node *np)
}
EXPORT_SYMBOL_GPL(of_dma_configure);
+/**
+ * of_dma_deconfigure - Clean up DMA configuration
+ * @dev: Device for which to clean up DMA configuration
+ *
+ * Clean up all configuration performed by of_dma_configure_ops() and free all
+ * resources that have been allocated.
+ */
+void of_dma_deconfigure(struct device *dev)
+{
+ arch_teardown_dma_ops(dev);
+}
+
int of_device_register(struct platform_device *pdev)
{
device_initialize(&pdev->dev);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index f39ccd5..9cb7090 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -153,11 +153,6 @@ struct platform_device *of_device_alloc(struct device_node *np,
}
EXPORT_SYMBOL(of_device_alloc);
-static void of_dma_deconfigure(struct device *dev)
-{
- arch_teardown_dma_ops(dev);
-}
-
/**
* of_platform_device_create_pdata - Alloc, initialize and register an of_device
* @np: pointer to node to create device for
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index cc7dd687..d20a31a 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -56,6 +56,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
}
void of_dma_configure(struct device *dev, struct device_node *np);
+void of_dma_deconfigure(struct device *dev);
#else /* CONFIG_OF */
static inline int of_driver_match_device(struct device *dev,
@@ -100,6 +101,8 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
}
static inline void of_dma_configure(struct device *dev, struct device_node *np)
{}
+static inline void of_dma_deconfigure(struct device *dev)
+{}
#endif /* CONFIG_OF */
#endif /* _LINUX_OF_DEVICE_H */
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V3 4/8] drivers: platform: Configure dma operations at probe time
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
Configuring DMA ops at probe time will allow deferring device probe when
the IOMMU isn't available yet. The dma_configure for the device is now called
from the generic device_attach callback just before the bus/driver probe
is called. This way, configuring the dma ops for the device would be called
at the same place for all bus_types, hence the deferred probing mechanism
should work for all buses as well.
pci_bus_add_devices (platform/amba)(_device_create/driver_register)
| |
pci_bus_add_device (device_add/driver_register)
| |
device_attach device_initial_probe
| |
__device_attach_driver __device_attach_driver
|
driver_probe_device
|
really_probe
|
dma_configure
Similarly on the device/driver_unregister path __device_release_driver is
called which inturn calls dma_deconfigure.
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
drivers/base/dd.c | 10 ++++++++++
drivers/base/dma-mapping.c | 11 +++++++++++
drivers/of/platform.c | 4 ----
drivers/pci/probe.c | 5 +----
include/linux/dma-mapping.h | 3 +++
5 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 16688f5..cfebd48 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -19,6 +19,7 @@
#include <linux/device.h>
#include <linux/delay.h>
+#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/kthread.h>
#include <linux/wait.h>
@@ -353,6 +354,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
if (ret)
goto pinctrl_bind_failed;
+ ret = dma_configure(dev);
+ if (ret)
+ goto dma_failed;
+
if (driver_sysfs_add(dev)) {
printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
__func__, dev_name(dev));
@@ -395,6 +400,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
goto done;
probe_failed:
+ dma_deconfigure(dev);
+dma_failed:
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
@@ -780,6 +787,9 @@ static void __device_release_driver(struct device *dev)
dev->bus->remove(dev);
else if (drv->remove)
drv->remove(dev);
+
+ dma_deconfigure(dev);
+
devres_release_all(dev);
dev->driver = NULL;
dev_set_drvdata(dev, NULL);
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index d799662..54e87f5 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -10,6 +10,7 @@
#include <linux/dma-mapping.h>
#include <linux/export.h>
#include <linux/gfp.h>
+#include <linux/of_device.h>
#include <linux/slab.h>
#include <linux/vmalloc.h>
@@ -166,6 +167,16 @@ void dmam_free_noncoherent(struct device *dev, size_t size, void *vaddr,
}
EXPORT_SYMBOL(dmam_free_noncoherent);
+int dma_configure(struct device *dev)
+{
+ return of_dma_configure(dev, dev->of_node);
+}
+
+void dma_deconfigure(struct device *dev)
+{
+ of_dma_deconfigure(dev);
+}
+
#ifdef CONFIG_HAVE_GENERIC_DMA_COHERENT
static void dmam_coherent_decl_release(struct device *dev, void *res)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9cb7090..adbd77c 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -181,11 +181,9 @@ static struct platform_device *of_platform_device_create_pdata(
dev->dev.bus = &platform_bus_type;
dev->dev.platform_data = platform_data;
- of_dma_configure(&dev->dev, dev->dev.of_node);
of_msi_configure(&dev->dev, dev->dev.of_node);
if (of_device_add(dev) != 0) {
- of_dma_deconfigure(&dev->dev);
platform_device_put(dev);
goto err_clear_flag;
}
@@ -242,7 +240,6 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
dev_set_name(&dev->dev, "%s", bus_id);
else
of_device_make_bus_id(&dev->dev);
- of_dma_configure(&dev->dev, dev->dev.of_node);
/* Allow the HW Peripheral ID to be overridden */
prop = of_get_property(node, "arm,primecell-periphid", NULL);
@@ -536,7 +533,6 @@ static int of_platform_device_destroy(struct device *dev, void *data)
amba_device_unregister(to_amba_device(dev));
#endif
- of_dma_deconfigure(dev);
of_node_clear_flag(dev->of_node, OF_POPULATED);
of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
return 0;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 93f280d..85c9553 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1724,10 +1724,7 @@ static void pci_dma_configure(struct pci_dev *dev)
{
struct device *bridge = pci_get_host_bridge_device(dev);
- if (IS_ENABLED(CONFIG_OF) &&
- bridge->parent && bridge->parent->of_node) {
- of_dma_configure(&dev->dev, bridge->parent->of_node);
- } else if (has_acpi_companion(bridge)) {
+ if (has_acpi_companion(bridge)) {
struct acpi_device *adev = to_acpi_device_node(bridge->fwnode);
enum dev_dma_attr attr = acpi_get_dma_attr(adev);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 66533e1..2766dbe 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -656,6 +656,9 @@ dma_mark_declared_memory_occupied(struct device *dev,
}
#endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */
+int dma_configure(struct device *dev);
+void dma_deconfigure(struct device *dev);
+
/*
* Managed DMA API
*/
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V3 5/8] iommu: of: Handle IOMMU lookup failure with deferred probing or error
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Failures to look up an IOMMU when parsing the DT iommus property need to
be handled separately from the .of_xlate() failures to support deferred
probing.
The lack of a registered IOMMU can be caused by the lack of a driver for
the IOMMU, the IOMMU device probe not having been performed yet, having
been deferred, or having failed.
The first case occurs when the device tree describes the bus master and
IOMMU topology correctly but no device driver exists for the IOMMU yet
or the device driver has not been compiled in. Return NULL, the caller
will configure the device without an IOMMU.
The second and third cases are handled by deferring the probe of the bus
master device which will eventually get reprobed after the IOMMU.
The last case is currently handled by deferring the probe of the bus
master device as well. A mechanism to either configure the bus master
device without an IOMMU or to fail the bus master device probe depending
on whether the IOMMU is optional or mandatory would be a good
enhancement.
The current iommu framework handles pci and non-pci devices separately,
so taken care of both the paths in this patch. The iommu's add_device
callback is invoked after the master's configuration data is added in
xlate. This is needed because the iommu core calls add_device callback
during the BUS_ADD_DEVICE notifier, which is of no use now. Eventually
that call has to be removed.
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
drivers/of/device.c | 7 ++++++-
include/linux/of_device.h | 6 ++++--
3 files changed, 53 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5b82862..1a5e28b 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -23,6 +23,7 @@
#include <linux/of.h>
#include <linux/of_iommu.h>
#include <linux/of_pci.h>
+#include <linux/pci.h>
#include <linux/slab.h>
static const struct of_device_id __iommu_of_table_sentinel
@@ -167,12 +168,29 @@ static const struct iommu_ops
return NULL;
ops = of_iommu_get_ops(iommu_spec.np);
+
+ if (!ops) {
+ const struct of_device_id *oid;
+
+ oid = of_match_node(&__iommu_of_table, iommu_spec.np);
+ ops = oid ? ERR_PTR(-EPROBE_DEFER) : NULL;
+ return ops;
+ }
+
if (!ops || !ops->of_xlate ||
iommu_fwspec_init(&pdev->dev, &iommu_spec.np->fwnode, ops) ||
ops->of_xlate(&pdev->dev, &iommu_spec))
ops = NULL;
+ if (ops && ops->add_device) {
+ ops = (ops->add_device(&pdev->dev) == 0) ? ops : NULL;
+
+ if (!ops)
+ dev_err(&pdev->dev, "Failed to setup iommu ops\n");
+ }
+
of_node_put(iommu_spec.np);
+
return ops;
}
@@ -183,9 +201,15 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
struct device_node *np;
const struct iommu_ops *ops = NULL;
int idx = 0;
+ struct device *bridge;
+
+ if (dev_is_pci(dev)) {
+ bridge = pci_get_host_bridge_device(to_pci_dev(dev));
- if (dev_is_pci(dev))
- return of_pci_iommu_configure(to_pci_dev(dev), master_np);
+ if (bridge && bridge->parent && bridge->parent->of_node)
+ return of_pci_iommu_configure(to_pci_dev(dev),
+ bridge->parent->of_node);
+ }
/*
* We don't currently walk up the tree looking for a parent IOMMU.
@@ -198,6 +222,14 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
np = iommu_spec.np;
ops = of_iommu_get_ops(np);
+ if (!ops) {
+ const struct of_device_id *oid;
+
+ oid = of_match_node(&__iommu_of_table, np);
+ ops = oid ? ERR_PTR(-EPROBE_DEFER) : NULL;
+ goto err_put_node;
+ }
+
if (!ops || !ops->of_xlate ||
iommu_fwspec_init(dev, &np->fwnode, ops) ||
ops->of_xlate(dev, &iommu_spec))
@@ -207,11 +239,18 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
idx++;
}
+ if (ops && ops->add_device) {
+ ops = (ops->add_device(dev) == 0) ? ops : NULL;
+
+ if (!ops)
+ dev_err(dev, "Failed to setup iommu_ops\n");
+ }
+
return ops;
err_put_node:
of_node_put(np);
- return NULL;
+ return ops;
}
static int __init of_iommu_init(void)
@@ -222,7 +261,7 @@ static int __init of_iommu_init(void)
for_each_matching_node_and_match(np, matches, &match) {
const of_iommu_init_fn init_fn = match->data;
- if (init_fn(np))
+ if (init_fn && init_fn(np))
pr_err("Failed to initialise IOMMU %s\n",
of_node_full_name(np));
}
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 1c843e2..c8e74d7 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -82,7 +82,7 @@ int of_device_add(struct platform_device *ofdev)
* can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events
* to fix up DMA configuration.
*/
-void of_dma_configure(struct device *dev, struct device_node *np)
+int of_dma_configure(struct device *dev, struct device_node *np)
{
u64 dma_addr, paddr, size;
int ret;
@@ -129,10 +129,15 @@ void of_dma_configure(struct device *dev, struct device_node *np)
coherent ? " " : " not ");
iommu = of_iommu_configure(dev, np);
+ if (IS_ERR(iommu))
+ return PTR_ERR(iommu);
+
dev_dbg(dev, "device is%sbehind an iommu\n",
iommu ? " " : " not ");
arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+ return 0;
}
EXPORT_SYMBOL_GPL(of_dma_configure);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index d20a31a..2bdb872 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
return of_node_get(cpu_dev->of_node);
}
-void of_dma_configure(struct device *dev, struct device_node *np);
+int of_dma_configure(struct device *dev, struct device_node *np);
void of_dma_deconfigure(struct device *dev);
#else /* CONFIG_OF */
@@ -100,7 +100,9 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
return NULL;
}
static inline void of_dma_configure(struct device *dev, struct device_node *np)
-{}
+{
+ return 0;
+}
static inline void of_dma_deconfigure(struct device *dev)
{}
#endif /* CONFIG_OF */
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V3 6/8] arm: dma-mapping: Reset the device's dma_ops
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
The dma_ops for the device is not getting set to NULL in
arch_tear_down_dma_ops and this causes an issue when the
device's probe gets deferred and retried. So reset the
dma_ops to NULL.
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
arch/arm/mm/dma-mapping.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index dde6514..b9191f0 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2295,6 +2295,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev)
__arm_iommu_detach_device(dev);
arm_iommu_release_mapping(mapping);
+ set_dma_ops(dev, NULL);
}
#else
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V3 7/8] arm/arm64: dma-mapping: Call iommu's remove_device callback during device detach
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
dma_deconfigure calls arch_teardown_dma_ops in the device_detach path,
which is called when the device gets detached from the driver.
When the device was added, iommu's add_device callback was used to
add the device in to its iommu_group and setup the device to be ready
to use its iommu. Similarly, call remove_device callback to remove the
device from the group and reset any other device's iommu configurations.
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
arch/arm/mm/dma-mapping.c | 8 ++++++++
arch/arm64/mm/dma-mapping.c | 7 +++++++
2 files changed, 15 insertions(+)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index b9191f0..cbe22de 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2289,11 +2289,19 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
static void arm_teardown_iommu_dma_ops(struct device *dev)
{
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+ const struct iommu_ops *ops;
if (!mapping)
return;
__arm_iommu_detach_device(dev);
+
+ if (dev->iommu_fwspec) {
+ ops = dev->iommu_fwspec->ops;
+ if (ops->remove_device)
+ ops->remove_device(dev);
+ }
+
arm_iommu_release_mapping(mapping);
set_dma_ops(dev, NULL);
}
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 610d8e5..faf4b92 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -938,6 +938,13 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
void arch_teardown_dma_ops(struct device *dev)
{
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+ const struct iommu_ops *ops;
+
+ if (dev->iommu_fwspec) {
+ ops = dev->iommu_fwspec->ops;
+ if (ops->remove_device)
+ ops->remove_device(dev);
+ }
if (WARN_ON(domain))
iommu_detach_device(domain, dev);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V3 8/8] arm64: dma-mapping: Remove the notifier trick to handle early setting of dma_ops
From: Sricharan R @ 2016-10-04 17:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
With arch_setup_dma_ops now being called late during device's probe after the
device's iommu is probed, the notifier trick required to handle the early
setup of dma_ops before the iommu group gets created is not required.
So removing the notifier's here.
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
arch/arm64/mm/dma-mapping.c | 100 ++------------------------------------------
1 file changed, 3 insertions(+), 97 deletions(-)
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index faf4b92..eb593af 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -799,24 +799,6 @@ static struct dma_map_ops iommu_dma_ops = {
.mapping_error = iommu_dma_mapping_error,
};
-/*
- * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
- * everything it needs to - the device is only partially created and the
- * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
- * need this delayed attachment dance. Once IOMMU probe ordering is sorted
- * to move the arch_setup_dma_ops() call later, all the notifier bits below
- * become unnecessary, and will go away.
- */
-struct iommu_dma_notifier_data {
- struct list_head list;
- struct device *dev;
- const struct iommu_ops *ops;
- u64 dma_base;
- u64 size;
-};
-static LIST_HEAD(iommu_dma_masters);
-static DEFINE_MUTEX(iommu_dma_notifier_lock);
-
static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,
u64 dma_base, u64 size)
{
@@ -837,79 +819,9 @@ static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,
return true;
}
-static void queue_iommu_attach(struct device *dev, const struct iommu_ops *ops,
- u64 dma_base, u64 size)
-{
- struct iommu_dma_notifier_data *iommudata;
-
- iommudata = kzalloc(sizeof(*iommudata), GFP_KERNEL);
- if (!iommudata)
- return;
-
- iommudata->dev = dev;
- iommudata->ops = ops;
- iommudata->dma_base = dma_base;
- iommudata->size = size;
-
- mutex_lock(&iommu_dma_notifier_lock);
- list_add(&iommudata->list, &iommu_dma_masters);
- mutex_unlock(&iommu_dma_notifier_lock);
-}
-
-static int __iommu_attach_notifier(struct notifier_block *nb,
- unsigned long action, void *data)
-{
- struct iommu_dma_notifier_data *master, *tmp;
-
- if (action != BUS_NOTIFY_BIND_DRIVER)
- return 0;
-
- mutex_lock(&iommu_dma_notifier_lock);
- list_for_each_entry_safe(master, tmp, &iommu_dma_masters, list) {
- if (data == master->dev && do_iommu_attach(master->dev,
- master->ops, master->dma_base, master->size)) {
- list_del(&master->list);
- kfree(master);
- break;
- }
- }
- mutex_unlock(&iommu_dma_notifier_lock);
- return 0;
-}
-
-static int __init register_iommu_dma_ops_notifier(struct bus_type *bus)
-{
- struct notifier_block *nb = kzalloc(sizeof(*nb), GFP_KERNEL);
- int ret;
-
- if (!nb)
- return -ENOMEM;
-
- nb->notifier_call = __iommu_attach_notifier;
-
- ret = bus_register_notifier(bus, nb);
- if (ret) {
- pr_warn("Failed to register DMA domain notifier; IOMMU DMA ops unavailable on bus '%s'\n",
- bus->name);
- kfree(nb);
- }
- return ret;
-}
-
static int __init __iommu_dma_init(void)
{
- int ret;
-
- ret = iommu_dma_init();
- if (!ret)
- ret = register_iommu_dma_ops_notifier(&platform_bus_type);
- if (!ret)
- ret = register_iommu_dma_ops_notifier(&amba_bustype);
-#ifdef CONFIG_PCI
- if (!ret)
- ret = register_iommu_dma_ops_notifier(&pci_bus_type);
-#endif
- return ret;
+ return iommu_dma_init();
}
arch_initcall(__iommu_dma_init);
@@ -920,18 +832,12 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
if (!ops)
return;
- /*
- * TODO: As a concession to the future, we're ready to handle being
- * called both early and late (i.e. after bus_add_device). Once all
- * the platform bus code is reworked to call us late and the notifier
- * junk above goes away, move the body of do_iommu_attach here.
- */
+
group = iommu_group_get(dev);
+
if (group) {
do_iommu_attach(dev, ops, dma_base, size);
iommu_group_put(group);
- } else {
- queue_iommu_attach(dev, ops, dma_base, size);
}
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH v2] arm: Added support for getcpu() vDSO using TPIDRURW
From: Mark Rutland @ 2016-10-04 17:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475595363-4272-1-git-send-email-fredrik.markstrom@gmail.com>
On Tue, Oct 04, 2016 at 05:35:33PM +0200, Fredrik Markstrom wrote:
> This makes getcpu() ~1000 times faster, this is very useful when
> implementing per-cpu buffers in userspace (to avoid cache line
> bouncing). As an example lttng ust becomes ~30% faster.
>
> The patch will break applications using TPIDRURW (which is context switched
> since commit 4780adeefd042482f624f5e0d577bf9cdcbb760 ("ARM: 7735/2:
It looks like you dropped the leading 'a' from the commit ID. For
everyone else's benefit, the full ID is:
a4780adeefd042482f624f5e0d577bf9cdcbb760
Please note that arm64 has done similar for compat tasks since commit:
d00a3810c16207d2 ("arm64: context-switch user tls register tpidr_el0 for
compat tasks")
> Preserve the user r/w register TPIDRURW on context switch and fork")) and
> is therefore made configurable.
As you note above, this is an ABI break and *will* break some existing
applications. That's generally a no-go.
This also leaves arm64's compat with the existing behaviour, differing
from arm.
I was under the impression that other mechanisms were being considered
for fast userspace access to per-cpu data structures, e.g. restartable
sequences. What is the state of those? Why is this better?
If getcpu() specifically is necessary, is there no other way to
implement it?
> +notrace int __vdso_getcpu(unsigned int *cpup, unsigned int *nodep,
> + struct getcpu_cache *tcache)
> +{
> + unsigned long node_and_cpu;
> +
> + asm("mrc p15, 0, %0, c13, c0, 2\n" : "=r"(node_and_cpu));
> +
> + if (nodep)
> + *nodep = cpu_to_node(node_and_cpu >> 16);
> + if (cpup)
> + *cpup = node_and_cpu & 0xffffUL;
Given this is directly user-accessible, this format is a de-facto ABI,
even if it's not documented as such. Is this definitely the format you
want long-term?
Thanks,
Mark.
^ permalink raw reply
* [RFC 05/11] iommu/dma: iommu_dma_(un)map_mixed
From: Robin Murphy @ 2016-10-04 17:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161002095614.GA23218@cbox>
On 02/10/16 10:56, Christoffer Dall wrote:
> On Fri, Sep 30, 2016 at 02:24:40PM +0100, Robin Murphy wrote:
>> Hi Eric,
>>
>> On 27/09/16 21:48, Eric Auger wrote:
>>> iommu_dma_map_mixed and iommu_dma_unmap_mixed operate on
>>> IOMMU_DOMAIN_MIXED typed domains. On top of standard iommu_map/unmap
>>> they reserve the IOVA window to prevent the iova allocator to
>>> allocate in those areas.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>> drivers/iommu/dma-iommu.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/dma-iommu.h | 18 ++++++++++++++++++
>>> 2 files changed, 66 insertions(+)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 04bbc85..db21143 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -759,3 +759,51 @@ int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>> return 0;
>>> }
>>> EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
>>> +
>>> +int iommu_dma_map_mixed(struct iommu_domain *domain, unsigned long iova,
>>> + phys_addr_t paddr, size_t size, int prot)
>>> +{
>>> + struct iova_domain *iovad;
>>> + unsigned long lo, hi;
>>> + int ret;
>>> +
>>> + if (domain->type != IOMMU_DOMAIN_MIXED)
>>> + return -EINVAL;
>>> +
>>> + if (!domain->iova_cookie)
>>> + return -EINVAL;
>>> +
>>> + iovad = cookie_iovad(domain);
>>> +
>>> + lo = iova_pfn(iovad, iova);
>>> + hi = iova_pfn(iovad, iova + size - 1);
>>> + reserve_iova(iovad, lo, hi);
>>
>> This can't work reliably - reserve_iova() will (for good reason) merge
>> any adjacent or overlapping entries, so any unmap is liable to free more
>> IOVA space than actually gets unmapped, and things will get subtly out
>> of sync and go wrong later.
>>
>> The more general issue with this whole approach, though, is that it
>> effectively rules out userspace doing guest memory hotplug or similar,
>> and I'm not we want to paint ourselves into that corner. Basically, as
>> soon as a device is attached to a guest, the entirety of the unallocated
>> IPA space becomes reserved, and userspace can never add anything further
>> to it, because any given address *might* be in use for an MSI mapping.
>
> Ah, we didn't think of that when discussing this design at KVM Forum,
> because the idea was that the IOVA allocator was in charge of that
> resource, and the IOVA was a separate concept from the IPA space.
>
> I think what tripped us up, is that while the above is true for the MSI
> configuration where we trap the bar and do the allocation at VFIO init
> time, the guest device driver can program DMA to any address without
> trapping, and therefore there's an inherent relationship between the
> IOVA and the IPA space. Is that right?
Yes, for anything the guest knows about and/or can touch directly, IOVA
must equal IPA, or DMA is going to go horribly wrong. It's only direct
interactions between device and host behind the guest's back where we
(may) have some freedom with IOVA assignment.
>> I think it still makes most sense to stick with the original approach of
>> cooperating with userspace to reserve a bounded area - it's just that we
>> can then let automatic mapping take care of itself within that area.
>
> I was thinking that it's also possible to do it the other way around: To
> let userspace say wherever memory may be hotplugged and do the
> allocation within the remaining area, but I suppose that's pretty much
> the same thing, and it should just depend on what's easiest to implement
> and what userspace can best predict.
Indeed, if userspace *is* able to pre-emptively claim everything it
might ever want, that does kind of implicitly solve the "tell me where I
can put this" problem (assuming it doesn't simply claim the whole
address space, of course), but I'm not so sure it works well if there
are any specific restrictions (e.g. if some device is going to require
the MSI range to be 32-bit addressable). It also fails to address the
issue below...
>> Speaking of which, I've realised the same fundamental reservation
>> problem already applies to PCI without ACS, regardless of MSIs. I just
>> tried on my Juno with guest memory placed at 0x4000000000, (i.e.
>> matching the host PA of the 64-bit PCI window), and sure enough when the
>> guest kicks off some DMA on the passed-through NIC, the root complex
>> interprets the guest IPA as (unsupported) peer-to-peer DMA to a BAR
>> claimed by the video card, and it fails. I guess this doesn't get hit in
>> practice on x86 because the guest memory map is unlikely to be much
>> different from the host's.
>>
>> It seems like we basically need a general way of communicating fixed and
>> movable host reservations to userspace :/
>>
>
> Yes, this makes sense to me. Do we have any existing way of
> discovering this from userspace or can we think of something?
I know virtually nothing about the userspace interface, but I was under
the impression it would require something new. I wasn't even aware you
could do the VFIO-under-QEMU-TCG thing which Eric points out, so it
seems like the general "tell userspace about addresses it can't use"
issue is perhaps the more pressing one. On investigation, QEMU's static
memory map with RAM at 0x4000000 is already busted for VFIO on Juno, as
that results in attempting DMA to config space, which goes about as well
as one might expect.
Robin.
>
> Thanks,
> -Christoffer
>
^ permalink raw reply
* [RFC 05/11] iommu/dma: iommu_dma_(un)map_mixed
From: Auger Eric @ 2016-10-04 17:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <7474f131-9e44-bfde-6937-7cdbd6b2c8a5@arm.com>
Hi Robin,
On 04/10/2016 19:18, Robin Murphy wrote:
> On 02/10/16 10:56, Christoffer Dall wrote:
>> On Fri, Sep 30, 2016 at 02:24:40PM +0100, Robin Murphy wrote:
>>> Hi Eric,
>>>
>>> On 27/09/16 21:48, Eric Auger wrote:
>>>> iommu_dma_map_mixed and iommu_dma_unmap_mixed operate on
>>>> IOMMU_DOMAIN_MIXED typed domains. On top of standard iommu_map/unmap
>>>> they reserve the IOVA window to prevent the iova allocator to
>>>> allocate in those areas.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>> drivers/iommu/dma-iommu.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/dma-iommu.h | 18 ++++++++++++++++++
>>>> 2 files changed, 66 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index 04bbc85..db21143 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -759,3 +759,51 @@ int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>>> return 0;
>>>> }
>>>> EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
>>>> +
>>>> +int iommu_dma_map_mixed(struct iommu_domain *domain, unsigned long iova,
>>>> + phys_addr_t paddr, size_t size, int prot)
>>>> +{
>>>> + struct iova_domain *iovad;
>>>> + unsigned long lo, hi;
>>>> + int ret;
>>>> +
>>>> + if (domain->type != IOMMU_DOMAIN_MIXED)
>>>> + return -EINVAL;
>>>> +
>>>> + if (!domain->iova_cookie)
>>>> + return -EINVAL;
>>>> +
>>>> + iovad = cookie_iovad(domain);
>>>> +
>>>> + lo = iova_pfn(iovad, iova);
>>>> + hi = iova_pfn(iovad, iova + size - 1);
>>>> + reserve_iova(iovad, lo, hi);
>>>
>>> This can't work reliably - reserve_iova() will (for good reason) merge
>>> any adjacent or overlapping entries, so any unmap is liable to free more
>>> IOVA space than actually gets unmapped, and things will get subtly out
>>> of sync and go wrong later.
>>>
>>> The more general issue with this whole approach, though, is that it
>>> effectively rules out userspace doing guest memory hotplug or similar,
>>> and I'm not we want to paint ourselves into that corner. Basically, as
>>> soon as a device is attached to a guest, the entirety of the unallocated
>>> IPA space becomes reserved, and userspace can never add anything further
>>> to it, because any given address *might* be in use for an MSI mapping.
>>
>> Ah, we didn't think of that when discussing this design at KVM Forum,
>> because the idea was that the IOVA allocator was in charge of that
>> resource, and the IOVA was a separate concept from the IPA space.
>>
>> I think what tripped us up, is that while the above is true for the MSI
>> configuration where we trap the bar and do the allocation at VFIO init
>> time, the guest device driver can program DMA to any address without
>> trapping, and therefore there's an inherent relationship between the
>> IOVA and the IPA space. Is that right?
>
> Yes, for anything the guest knows about and/or can touch directly, IOVA
> must equal IPA, or DMA is going to go horribly wrong. It's only direct
> interactions between device and host behind the guest's back where we
> (may) have some freedom with IOVA assignment.
>
>>> I think it still makes most sense to stick with the original approach of
>>> cooperating with userspace to reserve a bounded area - it's just that we
>>> can then let automatic mapping take care of itself within that area.
>>
>> I was thinking that it's also possible to do it the other way around: To
>> let userspace say wherever memory may be hotplugged and do the
>> allocation within the remaining area, but I suppose that's pretty much
>> the same thing, and it should just depend on what's easiest to implement
>> and what userspace can best predict.
>
> Indeed, if userspace *is* able to pre-emptively claim everything it
> might ever want, that does kind of implicitly solve the "tell me where I
> can put this" problem (assuming it doesn't simply claim the whole
> address space, of course), but I'm not so sure it works well if there
> are any specific restrictions (e.g. if some device is going to require
> the MSI range to be 32-bit addressable). It also fails to address the
> issue below...
>
>>> Speaking of which, I've realised the same fundamental reservation
>>> problem already applies to PCI without ACS, regardless of MSIs. I just
>>> tried on my Juno with guest memory placed at 0x4000000000, (i.e.
>>> matching the host PA of the 64-bit PCI window), and sure enough when the
>>> guest kicks off some DMA on the passed-through NIC, the root complex
>>> interprets the guest IPA as (unsupported) peer-to-peer DMA to a BAR
>>> claimed by the video card, and it fails. I guess this doesn't get hit in
>>> practice on x86 because the guest memory map is unlikely to be much
>>> different from the host's.
>>>
>>> It seems like we basically need a general way of communicating fixed and
>>> movable host reservations to userspace :/
>>>
>>
>> Yes, this makes sense to me. Do we have any existing way of
>> discovering this from userspace or can we think of something?
>
> I know virtually nothing about the userspace interface, but I was under
> the impression it would require something new. I wasn't even aware you
> could do the VFIO-under-QEMU-TCG thing which Eric points out,
I meant running a non x86 VM on an x86 host. Quoting Alex:
"x86 isn't problem-free in this space. An x86 VM is going to know that
the 0xfee00000 address range is special, it won't be backed by RAM and
won't be a DMA target, thus we'll never attempt to map it for an iova
address. However, if we run a non-x86 VM or a userspace driver, it
doesn't necessarily know that there's anything special about that range
of iovas. I intend to resolve this with an extension to the iommu info
ioctl that describes the available iova space for the iommu. The
interrupt region would simply be excluded."
In my v12 I added such VFIO IOMMU info ioctl to retrieve the MSI
topology. Now for the issue you pointed out (PCI without ACS) I
understand this is a generalisation of the same issue and the VFIO IOMMU
info capability chain API could be used as well. I can submit something
separately. But anyway at QEMU level, due to the static mapping in
mach-virt, at the moment, we just can reject the assignment I am afraid.
Thanks
Eric
so it
> seems like the general "tell userspace about addresses it can't use"
> issue is perhaps the more pressing one. On investigation, QEMU's static
> memory map with RAM at 0x4000000 is already busted for VFIO on Juno, as
> that results in attempting DMA to config space, which goes about as well
> as one might expect.
>
> Robin.
>
>>
>> Thanks,
>> -Christoffer
>>
>
^ permalink raw reply
* ks_dw_pcie_initiate_link_train() question
From: Bjorn Helgaas @ 2016-10-04 17:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi Murali,
This code looks suspicious. Can you comment?
void ks_dw_pcie_initiate_link_train(struct keystone_pcie *ks_pcie)
{
u32 val;
/* Disable Link training */
val = readl(ks_pcie->va_app_base + CMD_STATUS);
val &= ~LTSSM_EN_VAL;
writel(LTSSM_EN_VAL | val, ks_pcie->va_app_base + CMD_STATUS);
Here we cleared the LTSSM_EN_VAL bit in "val", but then we add it
right back in before writing it back to CMD_STATUS.
That looks like a cut and paste error to me, but of course I don't
know the hardware.
/* Initiate Link Training */
val = readl(ks_pcie->va_app_base + CMD_STATUS);
writel(LTSSM_EN_VAL | val, ks_pcie->va_app_base + CMD_STATUS);
}
^ permalink raw reply
* [PATCH] clocksource: arm_arch_timer: Don't assume clock runs in suspend
From: Brian Norris @ 2016-10-04 17:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160929170847.1227a312@arm.com>
Hi Marc,
On Thu, Sep 29, 2016 at 05:08:47PM +0100, Marc Zyngier wrote:
> On Tue, 27 Sep 2016 18:23:11 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> > On Tue, Sep 20, 2016 at 08:47:07AM +0100, Marc Zyngier wrote:
> > <Begin side note>
> > rk3288 (ARMv7 system widely used for our Chromebooks) has the same
> > issue, except the kernel we're using for production (based on v3.14)
> > doesn't have the following commit, which stopped utilizing the RTC:
> >
> > commit 0fa88cb4b82b5cf7429bc1cef9db006ca035754e
> > Author: Xunlei Pang <pang.xunlei@linaro.org>
> > Date: Wed Apr 1 20:34:38 2015 -0700
> >
> > time, drivers/rtc: Don't bother with rtc_resume() for the nonstop clocksource
> >
> > And any mainline testing on rk3288 doesn't see the problem, because
> > mainline doesn't support its lowest-power sleep modes well enough (see
> > ROCKCHIP_ARM_OFF_LOGIC_DEEP in arch/arm/mach-rockchip/pm.c).
>
> Arghh... So even my favourite Chromebook (from which I'm typing this
> email) is affected? Not very nice...
Yep. But if you're running mainline, you just get to have high S3 power
consumption instead!
> > <End side note>
> As for the 64bit kernel, it would be interesting to verify that on
> resume, the VDSO does return the right (corrected) value, and not
> something stale.
It would be interesting, except all my current user spaces are built for
32-bit, so it's not too easy for me to test. Perhaps I could pull in
this [1]. (On the bright side, this means that VDSO can't possibly be
breaking on my systems!)
Brian
[1] http://www.spinics.net/lists/arm-kernel/msg530185.html
^ permalink raw reply
* [RFC] arm64: Enforce observed order for spinlock and data
From: bdegraaf at codeaurora.org @ 2016-10-04 17:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161004101208.GA18083@leverpostej>
On 2016-10-04 06:12, Mark Rutland wrote:
> On Mon, Oct 03, 2016 at 03:20:57PM -0400, bdegraaf at codeaurora.org
> wrote:
>> On 2016-10-01 14:11, Mark Rutland wrote:
>> >Hi Brent,
>> >
>> >Evidently my questions weren't sufficiently clear; even with your
>> >answers it's not clear to me what precise issue you're attempting to
>> >solve. I've tried to be more specific this time.
>> >
>> >At a high-level, can you clarify whether you're attempting to solve is:
>> >
>> >(a) a functional correctness issue (e.g. data corruption)
>> >(b) a performance issue
>> >
>> >And whether this was seen in practice, or found through code
>> >inspection?
>
>> Thinking about this, as the reader/writer code has no known "abuse"
>> case, I'll remove it from the patchset, then provide a v2 patchset
>> with a detailed explanation for the lockref problem using the commits
>> you provided as an example, as well as performance consideration.
>
> If there's a functional problem, let's consider that in isolation
> first.
> Once we understand that, then we can consider doing what is optimal.
>
> As should be obvious from the above, I'm confused because this patch
> conflates functional details with performance optimisations which (to
> me) sound architecturally dubious.
>
> I completely agree with Peter that if the problem lies with lockref, it
> should be solved in the lockref code.
>
> Thanks,
> Mark.
After looking at this, the problem is not with the lockref code per se:
it is
a problem with arch_spin_value_unlocked(). In the out-of-order case,
arch_spin_value_unlocked() can return TRUE for a spinlock that is in
fact
locked but the lock is not observable yet via an ordinary load. Other
than
ensuring order on the locking side (as the prior patch did), there is a
way
to make arch_spin_value_unlock's TRUE return value deterministic, but it
requires that it does a write-back to the lock to ensure we didn't
observe
the unlocked value while another agent was in process of writing back a
locked value.
Brent
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox