* Re: [PATCH net v2 2/2] net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988
From: Russell King (Oracle) @ 2024-03-27 15:59 UTC (permalink / raw)
To: arinc.unal
Cc: Paolo Abeni, Daniel Golle, DENG Qingfang, Sean Wang, Andrew Lunn,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Matthias Brugger, AngeloGioacchino Del Regno,
René van Dorst, SkyLake Huang, Heiner Kallweit,
Bartel Eerdekens, mithat.guner, erkin.bozoglu, netdev,
linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <ZgRCFZBFvNSZ1a2U@shell.armlinux.org.uk>
On Wed, Mar 27, 2024 at 03:58:13PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 27, 2024 at 11:46:19AM +0300, arinc.unal@arinc9.com wrote:
> > On 26.03.2024 12:19, Arınç ÜNAL wrote:
> > > On 26.03.2024 12:02, Paolo Abeni wrote:
> > > > If I read the past discussion correctly, this is a potential issue
> > > > found by code inspection and never producing problem in practice, am I
> > > > correct?
> > > >
> > > > If so I think it will deserve a 3rd party tested-by tag or similar to
> > > > go in.
> > > >
> > > > If nobody could provide such feedback in a little time, I suggest to
> > > > drop this patch and apply only 1/2.
> > >
> > > Whether a problem would happen in practice depends on when
> > > phy_init_eee()
> > > fails, meaning it returns a negative non-zero code. I requested Russell
> > > to
> > > review this patch to shed light on when phy_init_eee() would return a
> > > negative non-zero code so we have an idea whether this patch actually
> > > fixes
> > > a problem.
> >
> > I don't suppose Russell is going to review the patch at this point. I will
> > submit this to net-next then. If someone actually reports a problem in
> > practice, I can always submit it to the stable trees.
>
> So the fact that I only saw your request this morning to look at
> phy_init_eee(), and to review this patch... because... I work for
> Oracle, and I've been looking at backporting Arm64 KVM patches to
> our kernel, been testing and debugging that effort... and the
> act that less than 24 hours had passed since you made the original
> request... yea, sorry, it's clearly my fault for not jumping on this
> the moment you sent the email.
>
> I get _so_ much email that incorrectly has me in the To: header. I
> also get _so_ much email that fails to list me in the To: header
> when the author wants me to respond. I don't have time to read every
> email as it comes in. I certainly don't have time to read every
> email in any case. I do the best I can, which varies considerably
> with my workload.
>
> I already find that being single, fitting everything in during the
> day (paid work, chores, feeding oneself) is quite a mammoth task.
> There is no one else to do the laundry. There is no one else to get
> the shopping. There is no one else to do the washing up. There is no
> one else to take the rubbish out. All this I do myself, and serially
> because there is only one of me, and it all takes time away from
> sitting here reading every damn email as it comes in.
>
> And then when I end up doing something that _you_ very well could do
> (reading the phy_init_eee() code to find out when it might return a
> negative number) and then you send an email like this... yea... that
> really gets my goat.
... and now I have a 1:1 with my manager for the next 30-60 minutes.
Is it okay by you for me to be offline for that period of time while
I have a chat with him?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 4/4] drivers: watchdog: ast2500 and ast2600 support bootstatus
From: Chia Hsing Yin @ 2024-03-27 16:07 UTC (permalink / raw)
To: Guenter Roeck
Cc: patrick, Wim Van Sebroeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, linux-watchdog,
devicetree, linux-arm-kernel, linux-aspeed, linux-kernel
In-Reply-To: <f0b03c0b-eb54-420e-a4f7-8286e20b9df6@roeck-us.net>
On Wed, Mar 27, 2024 at 11:47 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/27/24 01:53, Peter Yin wrote:
> > Add WDIOF_EXTERN1 and WDIOF_CARDRESET bootstatus in ast2600
> >
> > Regarding the AST2600 specification, the WDTn Timeout Status Register
> > (WDT10) has bit 1 reserved. Bit 1 of the status register indicates
> > on ast2500 if the boot was from the second boot source.
> > It does not indicate that the most recent reset was triggered by
> > the watchdog. The code should just be changed to set WDIOF_CARDRESET
> > if bit 0 of the status register is set.
> >
> > Include SCU register to veriy WDIOF_EXTERN1 in ast2600 SCU74 or
> > ast2500 SCU3C when bit1 is set.
> >
> > Signed-off-by: Peter Yin <peteryin.openbmc@gmail.com>
> > ---
> > drivers/watchdog/aspeed_wdt.c | 60 +++++++++++++++++++++++++----------
> > 1 file changed, 44 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index b4773a6aaf8c..29e9afdee619 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -11,10 +11,12 @@
> > #include <linux/io.h>
> > #include <linux/kernel.h>
> > #include <linux/kstrtox.h>
> > +#include <linux/mfd/syscon.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_irq.h>
> > #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > #include <linux/watchdog.h>
> >
> > static bool nowayout = WATCHDOG_NOWAYOUT;
> > @@ -65,23 +67,32 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
> > #define WDT_RELOAD_VALUE 0x04
> > #define WDT_RESTART 0x08
> > #define WDT_CTRL 0x0C
> > -#define WDT_CTRL_BOOT_SECONDARY BIT(7)
> > -#define WDT_CTRL_RESET_MODE_SOC (0x00 << 5)
> > -#define WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
> > -#define WDT_CTRL_RESET_MODE_ARM_CPU (0x10 << 5)
> > -#define WDT_CTRL_1MHZ_CLK BIT(4)
> > -#define WDT_CTRL_WDT_EXT BIT(3)
> > -#define WDT_CTRL_WDT_INTR BIT(2)
> > -#define WDT_CTRL_RESET_SYSTEM BIT(1)
> > -#define WDT_CTRL_ENABLE BIT(0)
> > +#define WDT_CTRL_BOOT_SECONDARY BIT(7)
> > +#define WDT_CTRL_RESET_MODE_SOC (0x00 << 5)
> > +#define WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
> > +#define WDT_CTRL_RESET_MODE_ARM_CPU (0x10 << 5)
> > +#define WDT_CTRL_1MHZ_CLK BIT(4)
> > +#define WDT_CTRL_WDT_EXT BIT(3)
> > +#define WDT_CTRL_WDT_INTR BIT(2)
> > +#define WDT_CTRL_RESET_SYSTEM BIT(1)
> > +#define WDT_CTRL_ENABLE BIT(0)
> > #define WDT_TIMEOUT_STATUS 0x10
> > -#define WDT_TIMEOUT_STATUS_IRQ BIT(2)
> > -#define WDT_TIMEOUT_STATUS_BOOT_SECONDARY BIT(1)
> > +#define WDT_TIMEOUT_STATUS_IRQ BIT(2)
> > +#define WDT_TIMEOUT_STATUS_BOOT_SECONDARY BIT(1)
> > +#define WDT_TIMEOUT_STATUS_EVENT BIT(0)
> > #define WDT_CLEAR_TIMEOUT_STATUS 0x14
> > -#define WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
> > +#define WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION BIT(0)
> > #define WDT_RESET_MASK1 0x1c
> > #define WDT_RESET_MASK2 0x20
> >
>
> The above bit value defines were indented to show what is
> registers and what is register bit values. Why are you
> changing that other than for personal preference ?
>
> Guenter
>
Oh! I'm sorry, I didn't realize this was a rule. I thought it was just
an alignment issue. I will revert it in the next version. Thank you
for explaining.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 12/16] PCI/pwrctl: add PCI power control core code
From: Simon Horman @ 2024-03-27 16:16 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Marcel Holtmann, Luiz Augusto von Dentz, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Kalle Valo, Bjorn Andersson,
Konrad Dybcio, Liam Girdwood, Mark Brown, Catalin Marinas,
Will Deacon, Bjorn Helgaas, Saravana Kannan, Geert Uytterhoeven,
Arnd Bergmann, Neil Armstrong, Marek Szyprowski, Alex Elder,
Srini Kandagatla, Greg Kroah-Hartman, Abel Vesa,
Manivannan Sadhasivam, Lukas Wunner, Dmitry Baryshkov,
linux-bluetooth, netdev, devicetree, linux-kernel, linux-wireless,
linux-arm-msm, linux-arm-kernel, linux-pci, linux-pm,
Bartosz Golaszewski
In-Reply-To: <20240325131624.26023-13-brgl@bgdev.pl>
On Mon, Mar 25, 2024 at 02:16:20PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Some PCI devices must be powered-on before they can be detected on the
> bus. Introduce a simple framework reusing the existing PCI OF
> infrastructure.
>
> The way this works is: a DT node representing a PCI device connected to
> the port can be matched against its power control platform driver. If
> the match succeeds, the driver is responsible for powering-up the device
> and calling pcie_pwrctl_device_set_ready() which will trigger a PCI bus
> rescan as well as subscribe to PCI bus notifications.
>
> When the device is detected and created, we'll make it consume the same
> DT node that the platform device did. When the device is bound, we'll
> create a device link between it and the parent power control device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Hi Bartosz,
some minor Kernel doc nits from my side.
...
> diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
...
> +/**
> + * devm_pci_pwrctl_device_set_ready - Managed variant of
> + * pci_pwrctl_device_set_ready().
> + *
nit: @dev should be documented here
> + * @pwrctl: PCI power control data
> + *
> + * Returns:
> + * 0 on success, negative error number on error.
> + */
> +int devm_pci_pwrctl_device_set_ready(struct device *dev,
> + struct pci_pwrctl *pwrctl)
> +{
> + int ret;
> +
> + ret = pci_pwrctl_device_set_ready(pwrctl);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dev,
> + devm_pci_pwrctl_device_unset_ready,
> + pwrctl);
> +}
> +EXPORT_SYMBOL_GPL(devm_pci_pwrctl_device_set_ready);
> +
> +MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>");
> +MODULE_DESCRIPTION("PCI Device Power Control core driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/pci-pwrctl.h b/include/linux/pci-pwrctl.h
> new file mode 100644
> index 000000000000..ae8324ea7eeb
> --- /dev/null
> +++ b/include/linux/pci-pwrctl.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Linaro Ltd.
> + */
> +
> +#ifndef __PCI_PWRCTL_H__
> +#define __PCI_PWRCTL_H__
> +
> +#include <linux/notifier.h>
> +
> +struct device;
> +struct device_link;
> +
> +/*
> + * This is a simple framework for solving the issue of PCI devices that require
> + * certain resources (regulators, GPIOs, clocks) to be enabled before the
> + * device can actually be detected on the PCI bus.
> + *
> + * The idea is to reuse the platform bus to populate OF nodes describing the
> + * PCI device and its resources, let these platform devices probe and enable
> + * relevant resources and then trigger a rescan of the PCI bus allowing for the
> + * same device (with a second associated struct device) to be registered with
> + * the PCI subsystem.
> + *
> + * To preserve a correct hierarchy for PCI power management and device reset,
> + * we create a device link between the power control platform device (parent)
> + * and the supplied PCI device (child).
> + */
> +
> +/**
> + * struct pci_pwrctl - PCI device power control context.
> + * @dev - Address of the power controlling device.
nit: I think this should be "@dev: " rather than "@dev - "
As is, "./scripts/kernel-doc -none" complains.
> + *
> + * An object of this type must be allocated by the PCI power control device and
> + * passed to the pwrctl subsystem to trigger a bus rescan and setup a device
> + * link with the device once it's up.
> + */
> +struct pci_pwrctl {
> + struct device *dev;
> +
> + /* Private, don't use. */
I think Private needs to be followed by a ':' rather than a ',' to keep
kernel-doc happy.
> + struct notifier_block nb;
> + struct device_link *link;
> +};
> +
> +int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl);
> +void pci_pwrctl_device_unset_ready(struct pci_pwrctl *pwrctl);
> +int devm_pci_pwrctl_device_set_ready(struct device *dev,
> + struct pci_pwrctl *pwrctl);
> +
> +#endif /* __PCI_PWRCTL_H__ */
> --
> 2.40.1
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [WIP 0/3] Memory model and atomic API in Rust
From: comex @ 2024-03-27 16:16 UTC (permalink / raw)
To: Linus Torvalds
Cc: Dr. David Alan Gilbert, Kent Overstreet, Philipp Stanner,
Boqun Feng, rust-for-linux, linux-kernel, linux-arch, llvm,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Alan Stern, Andrea Parri, Will Deacon, Peter Zijlstra,
Nicholas Piggin, David Howells, Jade Alglave, Luc Maranget,
Paul E. McKenney, Akira Yokosawa, Daniel Lustig, Joel Fernandes,
Nathan Chancellor, Nick Desaulniers, kent.overstreet,
Greg Kroah-Hartman, Marco Elver, Mark Rutland, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Catalin Marinas, linux-arm-kernel, linux-fsdevel
In-Reply-To: <CAHk-=wjwxKD9CxYsf5x+K5fJbJa_JYZh1eKB4PT5cZJq1+foGw@mail.gmail.com>
On Mar 25, 2024, at 8:49 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> But you should _start_ the design of your language memory model around
> the unsafe "raw atomic access operations" model.
>
> Then you can use those strictly more powerful operations, and you
> create an object model *around* it.
To some extent Rust does this already, unlike C++.
C++ allows atomics to be implemented using locks. Partly for this reason,
`std::atomic<T>` is documented as not necessarily having the same
representation as `T` [1]. C++ also has strict aliasing, so even if those types
do have the same representation, you still can't cast `T *` to
`std::atomic<T> *`.
But Rust atomics are lower-level. First, they are guaranteed lock-free [2].
Second, they are documented as having "the same in-memory representation as the
underlying" type [3]. (They also usually have the same alignment, except on
x86 where u64 is only 4-byte aligned but AtomicU64 of course needs to be 8-byte
aligned.) Meanwhile, Rust intentionally lacks strict aliasing.
Combined, this means it's perfectly legal in Rust to cast e.g. `&mut u32` to
`&AtomicU32` and perform atomic accesses on it. Or the same with u64/AtomicU64
if you know the pointer is validly aligned. This is by design; the Atomic
types' methods are considered the official way to perform atomic operations on
arbitrary memory, making it unnecessary to also stabilize 'lower-level'
intrinsics.
That said, there *are* currently some holes in Rust's atomics model, based on
the fact that it's mostly inherited from C++. From the documentation:
> Since C++ does not support mixing atomic and non-atomic accesses, or
> non-synchronized different-sized accesses to the same data, Rust does not
> support those operations either. Note that both of those restrictions only
> apply if the accesses are non-synchronized.
https://doc.rust-lang.org/std/sync/atomic/index.html
There are some open issues around this:
- "How can we allow read-read races between atomic and non-atomic accesses?"
https://github.com/rust-lang/unsafe-code-guidelines/issues/483
> [..] I do think we should allow such code. However, then we have to change
> the way we document our atomics [..]
- "What about: mixed-size atomic accesses"
https://github.com/rust-lang/unsafe-code-guidelines/issues/345"
> Apparently the x86 manual says you "should" not do this [..] It is unclear
> what "should" means (or what anything else here really means, operationally
> speaking...)
[1] https://eel.is/c++draft/atomics#types.generic.general-3
[2] https://doc.rust-lang.org/std/sync/atomic/index.html#portability
[3] https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicU64.html
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line
From: Folker Schwesinger @ 2024-03-27 16:21 UTC (permalink / raw)
To: Dragan Simic, Conor Dooley
Cc: Vinod Koul, Kishon Vijay Abraham I, Heiko Stuebner, Chris Ruehl,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Christopher Obbard, Alban Browaeys, Doug Anderson, Brian Norris,
Jensen Huang, linux-phy, linux-arm-kernel, linux-rockchip,
linux-kernel, devicetree
In-Reply-To: <436f78a981ecba441a0636912ddd1cf2@manjaro.org>
[-- Attachment #1.1: Type: text/plain, Size: 3995 bytes --]
Hi Conor and Dragan,
thanks for your feedback!
On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> On 2024-03-26 20:46, Conor Dooley wrote:
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> >>
> >> Restore the behavior of the Rockchip kernel that undconditionally
> >> enables the internal strobe pulldown.
> >
> > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > mainline behave the same as the rockchip kernel previously? If not,
> > using "restore" here is misleading. "Unconditionally" is also
> > incorrect,
> > because you have a property that disables it.
Apologizes for the misleading commit message. Prior to 5.11 the Linux
kernel did not touch the pull-down registers. However, it seems the
register's (factory?) default was set to enable the pull-down. As it
was mentioned elsewhere that was the configuration recommended by
Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
pull-down in its kernel.
Of course, this has nothing to do with the Linux kernel, so "restore"
was a bad choice here.
I previously had split the driver patch into two separate patches, one
for changing the default (unconditionally at that point), the other for
adding the disable property. As both changes were minimal I decided to
squash the commits. I updated the cover letter, but forgot to update the
commit message. Sorry.
> >> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in
> >> dts")
> >> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> >> ---
> >> drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> index 20023f6eb994..6e637f3e1b19 100644
> >> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct
> >> platform_device *pdev)
> >> rk_phy->reg_offset = reg_offset;
> >> rk_phy->reg_base = grf;
> >> rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> >> - rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >> + rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >> rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
> >>
> >> if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm",
> >> &val))
> >> rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> >>
> >> - if (of_property_read_bool(dev->of_node,
> >> "rockchip,enable-strobe-pulldown"))
> >> - rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >> + if (of_property_read_bool(dev->of_node,
> >> "rockchip,disable-strobe-pulldown"))
> >> + rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break and
> > is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> >
>
> Moreover, as I already explained some time ago, [1] some boards and
> devices are unfortunately miswired, and we don't want to enable the
> DATA STROBE pull-down on such boards.
>
> [1]
> https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t
>
> > If this patch fixes a problem on a board that you have, I would suggest
> > that you add the property to enable it, as the binding tells you to.
I agree, I'll post the patches later.
Best regards
Folker
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH RFC 0/3] mm/gup: consistently call it GUP-fast
From: Arnd Bergmann @ 2024-03-27 16:22 UTC (permalink / raw)
To: David Hildenbrand, peterx
Cc: linux-kernel, Andrew Morton, Mike Rapoport, Jason Gunthorpe,
John Hubbard, linux-arm-kernel, loongarch, linux-mips,
linuxppc-dev, linux-s390, linux-sh, linux-mm, linux-perf-users,
linux-fsdevel, x86, Ryan Roberts, Alexander Viro, Matt Turner,
Vineet Gupta
In-Reply-To: <3922460a-4d01-4ecb-b8c5-7c57fd46f3fd@redhat.com>
On Wed, Mar 27, 2024, at 16:39, David Hildenbrand wrote:
> On 27.03.24 16:21, Peter Xu wrote:
>> On Wed, Mar 27, 2024 at 02:05:35PM +0100, David Hildenbrand wrote:
>>
>> I'm not sure what config you tried there; as I am doing some build tests
>> recently, I found turning off CONFIG_SAMPLES + CONFIG_GCC_PLUGINS could
>> avoid a lot of issues, I think it's due to libc missing. But maybe not the
>> case there.
>
> CCin Arnd; I use some of his compiler chains, others from Fedora directly. For
> example for alpha and arc, the Fedora gcc is "13.2.1".
>
> But there is other stuff like (arc):
>
> ./arch/arc/include/asm/mmu-arcv2.h: In function 'mmu_setup_asid':
> ./arch/arc/include/asm/mmu-arcv2.h:82:9: error: implicit declaration of
> function 'write_aux_reg' [-Werro
> r=implicit-function-declaration]
> 82 | write_aux_reg(ARC_REG_PID, asid | MMU_ENABLE);
> | ^~~~~~~~~~~~~
Seems to be missing an #include of soc/arc/aux.h, but I can't
tell when this first broke without bisecting.
> or (alpha)
>
> WARNING: modpost: "saved_config" [vmlinux] is COMMON symbol
> ERROR: modpost: "memcpy" [fs/reiserfs/reiserfs.ko] undefined!
> ERROR: modpost: "memcpy" [fs/nfs/nfs.ko] undefined!
> ERROR: modpost: "memcpy" [fs/nfs/nfsv3.ko] undefined!
> ERROR: modpost: "memcpy" [fs/nfsd/nfsd.ko] undefined!
> ERROR: modpost: "memcpy" [fs/lockd/lockd.ko] undefined!
> ERROR: modpost: "memcpy" [crypto/crypto.ko] undefined!
> ERROR: modpost: "memcpy" [crypto/crypto_algapi.ko] undefined!
> ERROR: modpost: "memcpy" [crypto/aead.ko] undefined!
> ERROR: modpost: "memcpy" [crypto/crypto_skcipher.ko] undefined!
> ERROR: modpost: "memcpy" [crypto/seqiv.ko] undefined!
Al did a series to fix various build problems on alpha, see
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.alpha
Not sure if he still has to send them to Matt, or if Matt
just needs to apply them.
I also have some alpha patches that I should send upstream.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 7/7] regulator: mcp16502: Update the names from buck regulators
From: Conor Dooley @ 2024-03-27 16:28 UTC (permalink / raw)
To: Mihai Sain
Cc: robh, conor+dt, lgirdwood, linux-kernel, alexandre.belloni,
devicetree, broonie, krzysztof.kozlowski+dt, claudiu.beznea,
andrei.simion, linux-arm-kernel
In-Reply-To: <20240327101724.2982-8-mihai.sain@microchip.com>
[-- Attachment #1.1: Type: text/plain, Size: 2047 bytes --]
On Wed, Mar 27, 2024 at 12:17:24PM +0200, Mihai Sain wrote:
> Use generic names for buck regulators to avoid any confusion.
> Update the names from buck regulators in order to match
> the datasheet block diagram for the buck regulators.
>
> Signed-off-by: Mihai Sain <mihai.sain@microchip.com>
I know the regulator core will create dummy regulators when they are not
provided in the devicetree, so I am not 100% on how backwards
compatibility works here.
You'll end up with a bunch of dummies and therefore the regulator-names
and constraints on the regulator will be lost, no?
Can you explain how is this backwards compatible with the old
devicetrees?
Thanks,
Conor.
> ---
> drivers/regulator/mcp16502.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/regulator/mcp16502.c b/drivers/regulator/mcp16502.c
> index 0c15a19fe83a..d6fc9f1afaef 100644
> --- a/drivers/regulator/mcp16502.c
> +++ b/drivers/regulator/mcp16502.c
> @@ -468,13 +468,13 @@ static const struct linear_range b234_ranges[] = {
>
> static const struct regulator_desc mcp16502_desc[] = {
> /* MCP16502_REGULATOR(_name, _id, ranges, regulator_ops, ramp_table) */
> - MCP16502_REGULATOR("VDD_IO", BUCK1, b1l12_ranges, mcp16502_buck_ops,
> + MCP16502_REGULATOR("BUCK1", BUCK1, b1l12_ranges, mcp16502_buck_ops,
> mcp16502_ramp_b1l12),
> - MCP16502_REGULATOR("VDD_DDR", BUCK2, b234_ranges, mcp16502_buck_ops,
> + MCP16502_REGULATOR("BUCK2", BUCK2, b234_ranges, mcp16502_buck_ops,
> mcp16502_ramp_b234),
> - MCP16502_REGULATOR("VDD_CORE", BUCK3, b234_ranges, mcp16502_buck_ops,
> + MCP16502_REGULATOR("BUCK3", BUCK3, b234_ranges, mcp16502_buck_ops,
> mcp16502_ramp_b234),
> - MCP16502_REGULATOR("VDD_OTHER", BUCK4, b234_ranges, mcp16502_buck_ops,
> + MCP16502_REGULATOR("BUCK4", BUCK4, b234_ranges, mcp16502_buck_ops,
> mcp16502_ramp_b234),
> MCP16502_REGULATOR("LDO1", LDO1, b1l12_ranges, mcp16502_ldo_ops,
> mcp16502_ramp_b1l12),
> --
> 2.44.0
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v4 00/13] mm/gup: Unify hugetlb, part 2
From: peterx @ 2024-03-27 15:23 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Yang Shi, Kirill A . Shutemov, Mike Kravetz, John Hubbard,
Michael Ellerman, peterx, Andrew Jones, Muchun Song, linux-riscv,
linuxppc-dev, Christophe Leroy, Andrew Morton, Christoph Hellwig,
Lorenzo Stoakes, Matthew Wilcox, Rik van Riel, linux-arm-kernel,
Andrea Arcangeli, David Hildenbrand, Aneesh Kumar K . V,
Vlastimil Babka, James Houghton, Jason Gunthorpe, Mike Rapoport,
Axel Rasmussen
From: Peter Xu <peterx@redhat.com>
v4:
- Fix build issues, tested on more archs/configs ([x86_64, i386, arm, arm64,
powerpc, riscv, s390] x [allno, alldef, allmod]).
- Squashed the fixup series into v3, touched up commit messages [1]
- Added the patch to fix pud_pfn() into the series [2]
- Fixed one more build issue on arm+alldefconfig, where pgd_t is a
two-item array.
- Manage R-bs: add some, remove some (due to the squashes above)
- Rebase to latest mm-unstable (2f6182cd23a7, March 26th)
rfc: https://lore.kernel.org/r/20231116012908.392077-1-peterx@redhat.com
v1: https://lore.kernel.org/r/20231219075538.414708-1-peterx@redhat.com
v2: https://lore.kernel.org/r/20240103091423.400294-1-peterx@redhat.com
v3: https://lore.kernel.org/r/20240321220802.679544-1-peterx@redhat.com
The series removes the hugetlb slow gup path after a previous refactor work
[1], so that slow gup now uses the exact same path to process all kinds of
memory including hugetlb.
For the long term, we may want to remove most, if not all, call sites of
huge_pte_offset(). It'll be ideal if that API can be completely dropped
from arch hugetlb API. This series is one small step towards merging
hugetlb specific codes into generic mm paths. From that POV, this series
removes one reference to huge_pte_offset() out of many others.
One goal of such a route is that we can reconsider merging hugetlb features
like High Granularity Mapping (HGM). It was not accepted in the past
because it may add lots of hugetlb specific codes and make the mm code even
harder to maintain. With a merged codeset, features like HGM can hopefully
share some code with THP, legacy (PMD+) or modern (continuous PTEs).
To make it work, the generic slow gup code will need to at least understand
hugepd, which is already done like so in fast-gup. Due to the specialty of
hugepd to be software-only solution (no hardware recognizes the hugepd
format, so it's purely artificial structures), there's chance we can merge
some or all hugepd formats with cont_pte in the future. That question is
yet unsettled from Power side to have an acknowledgement. As of now for
this series, I kept the hugepd handling because we may still need to do so
before getting a clearer picture of the future of hugepd. The other reason
is simply that we did it already for fast-gup and most codes are still
around to be reused. It'll make more sense to keep slow/fast gup behave
the same before a decision is made to remove hugepd.
There's one major difference for slow-gup on cont_pte / cont_pmd handling,
currently supported on three architectures (aarch64, riscv, ppc). Before
the series, slow gup will be able to recognize e.g. cont_pte entries with
the help of huge_pte_offset() when hstate is around. Now it's gone but
still working, by looking up pgtable entries one by one.
It's not ideal, but hopefully this change should not affect yet on major
workloads. There's some more information in the commit message of the last
patch. If this would be a concern, we can consider teaching slow gup to
recognize cont pte/pmd entries, and that should recover the lost
performance. But I doubt its necessity for now, so I kept it as simple as
it can be.
Test Done
=========
For x86_64, tested full gup_test matrix over 2MB huge pages. For aarch64,
tested the same over 64KB cont_pte huge pages.
One note is that this v3 didn't go through any ppc test anymore, as finding
such system can always take time. It's based on the fact that it was
tested in previous versions, and this version should have zero change
regarding to hugepd sections.
If anyone (Christophe?) wants to give it a shot on PowerPC, please do and I
would appreciate it: "./run_vmtests.sh -a -t gup_test" should do well
enough (please consider [2] applied if hugepd is <1MB), as long as we're
sure the hugepd pages are touched as expected.
Patch layout
=============
Patch 1-8: Preparation works, or cleanups in relevant code paths
Patch 9-11: Teach slow gup with all kinds of huge entries (pXd, hugepd)
Patch 12: Drop hugetlb_follow_page_mask()
More information can be found in the commit messages of each patch. Any
comment will be welcomed. Thanks.
[1] https://lore.kernel.org/all/20230628215310.73782-1-peterx@redhat.com
[2] https://lore.kernel.org/r/20240321215047.678172-1-peterx@redhat.com
Peter Xu (13):
mm/Kconfig: CONFIG_PGTABLE_HAS_HUGE_LEAVES
mm/hugetlb: Declare hugetlbfs_pagecache_present() non-static
mm: Make HPAGE_PXD_* macros even if !THP
mm: Introduce vma_pgtable_walk_{begin|end}()
mm/arch: Provide pud_pfn() fallback
mm/gup: Drop folio_fast_pin_allowed() in hugepd processing
mm/gup: Refactor record_subpages() to find 1st small page
mm/gup: Handle hugetlb for no_page_table()
mm/gup: Cache *pudp in follow_pud_mask()
mm/gup: Handle huge pud for follow_pud_mask()
mm/gup: Handle huge pmd for follow_pmd_mask()
mm/gup: Handle hugepd for follow_page()
mm/gup: Handle hugetlb in the generic follow_page_mask code
arch/riscv/include/asm/pgtable.h | 1 +
arch/s390/include/asm/pgtable.h | 1 +
arch/sparc/include/asm/pgtable_64.h | 1 +
arch/x86/include/asm/pgtable.h | 1 +
include/linux/huge_mm.h | 37 +-
include/linux/hugetlb.h | 16 +-
include/linux/mm.h | 3 +
include/linux/pgtable.h | 10 +
mm/Kconfig | 6 +
mm/gup.c | 518 ++++++++++++++++++++--------
mm/huge_memory.c | 133 +------
mm/hugetlb.c | 75 +---
mm/internal.h | 7 +-
mm/memory.c | 12 +
14 files changed, 441 insertions(+), 380 deletions(-)
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v4 01/13] mm/Kconfig: CONFIG_PGTABLE_HAS_HUGE_LEAVES
From: peterx @ 2024-03-27 15:23 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Yang Shi, Kirill A . Shutemov, Mike Kravetz, John Hubbard,
Michael Ellerman, peterx, Andrew Jones, Muchun Song, linux-riscv,
linuxppc-dev, Christophe Leroy, Andrew Morton, Christoph Hellwig,
Lorenzo Stoakes, Matthew Wilcox, Rik van Riel, linux-arm-kernel,
Andrea Arcangeli, David Hildenbrand, Aneesh Kumar K . V,
Vlastimil Babka, James Houghton, Jason Gunthorpe, Mike Rapoport,
Axel Rasmussen
In-Reply-To: <20240327152332.950956-1-peterx@redhat.com>
From: Peter Xu <peterx@redhat.com>
Introduce a config option that will be selected as long as huge leaves are
involved in pgtable (thp or hugetlbfs). It would be useful to mark any
code with this new config that can process either hugetlb or thp pages in
any level that is higher than pte level.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
mm/Kconfig | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/mm/Kconfig b/mm/Kconfig
index b924f4a5a3ef..497cdf4d8ebf 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -850,6 +850,12 @@ config READ_ONLY_THP_FOR_FS
endif # TRANSPARENT_HUGEPAGE
+#
+# The architecture supports pgtable leaves that is larger than PAGE_SIZE
+#
+config PGTABLE_HAS_HUGE_LEAVES
+ def_bool TRANSPARENT_HUGEPAGE || HUGETLB_PAGE
+
#
# UP and nommu archs use km based percpu allocator
#
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v4 02/13] mm/hugetlb: Declare hugetlbfs_pagecache_present() non-static
From: peterx @ 2024-03-27 15:23 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Yang Shi, Kirill A . Shutemov, Mike Kravetz, John Hubbard,
Michael Ellerman, peterx, Andrew Jones, Muchun Song, linux-riscv,
linuxppc-dev, Christophe Leroy, Andrew Morton, Christoph Hellwig,
Lorenzo Stoakes, Matthew Wilcox, Rik van Riel, linux-arm-kernel,
Andrea Arcangeli, David Hildenbrand, Aneesh Kumar K . V,
Vlastimil Babka, James Houghton, Jason Gunthorpe, Mike Rapoport,
Axel Rasmussen
In-Reply-To: <20240327152332.950956-1-peterx@redhat.com>
From: Peter Xu <peterx@redhat.com>
It will be used outside hugetlb.c soon.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/linux/hugetlb.h | 9 +++++++++
mm/hugetlb.c | 4 ++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index d748628efc5e..294c78b3549f 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -174,6 +174,9 @@ u32 hugetlb_fault_mutex_hash(struct address_space *mapping, pgoff_t idx);
pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
unsigned long addr, pud_t *pud);
+bool hugetlbfs_pagecache_present(struct hstate *h,
+ struct vm_area_struct *vma,
+ unsigned long address);
struct address_space *hugetlb_page_mapping_lock_write(struct page *hpage);
@@ -1228,6 +1231,12 @@ static inline void hugetlb_register_node(struct node *node)
static inline void hugetlb_unregister_node(struct node *node)
{
}
+
+static inline bool hugetlbfs_pagecache_present(
+ struct hstate *h, struct vm_area_struct *vma, unsigned long address)
+{
+ return false;
+}
#endif /* CONFIG_HUGETLB_PAGE */
static inline spinlock_t *huge_pte_lock(struct hstate *h,
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f9640a81226e..65b9c9a48fd2 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6110,8 +6110,8 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
/*
* Return whether there is a pagecache page to back given address within VMA.
*/
-static bool hugetlbfs_pagecache_present(struct hstate *h,
- struct vm_area_struct *vma, unsigned long address)
+bool hugetlbfs_pagecache_present(struct hstate *h,
+ struct vm_area_struct *vma, unsigned long address)
{
struct address_space *mapping = vma->vm_file->f_mapping;
pgoff_t idx = linear_page_index(vma, address);
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v5 05/27] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()
From: Jason Gunthorpe @ 2024-03-27 16:42 UTC (permalink / raw)
To: Mostafa Saleh
Cc: iommu, Joerg Roedel, linux-arm-kernel, Robin Murphy, Will Deacon,
Eric Auger, Jean-Philippe Brucker, Moritz Fischer, Michael Shavit,
Nicolin Chen, patches, Shameerali Kolothum Thodi
In-Reply-To: <ZgPqnzA_a4ZdYGXM@google.com>
On Wed, Mar 27, 2024 at 09:45:03AM +0000, Mostafa Saleh wrote:
> On Tue, Mar 26, 2024 at 07:27:58PM -0300, Jason Gunthorpe wrote:
> > On Tue, Mar 26, 2024 at 07:12:53PM +0000, Mostafa Saleh wrote:
> > > On Tue, Mar 26, 2024 at 03:30:55PM -0300, Jason Gunthorpe wrote:
> > > > On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote:
> > > > > > +static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
> > > > > > +{
> > > > > > + used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
> > > > > > + if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
> > > > > > + return;
> > > > > > + memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));
> > > > >
> > > > > This is a slightly different approach than what the driver does for STEs,
> > > > > where it explicitly sets the used bits. Is there a reason for that?
> > > >
> > > > It is just more compact this way
> > >
> > > IMHO, it seems too much to have this mechanism for CDs for just one
> > > SVA case, but I'll need to go through the whole seires first to make
> > > sure I am not missing anything.
> >
> > It is pretty ugly if you try to do it that way. You still need to
> > create some ops because the entry_set should be re-used (I mean I
> > guess you could copy it as well). Then you have to open code the
> > logic. And then the EPD0 path is somewhat fragile. Something sort of
> > like this:
> >
> > void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
> > struct arm_smmu_cd *cdptr,
> > const struct arm_smmu_cd *target)
> > {
> > bool target_valid = target->data[0] & cpu_to_le64(CTXDESC_CD_0_V);
> > bool cur_valid = cdptr->data[0] & cpu_to_le64(CTXDESC_CD_0_V);
> > struct arm_smmu_cd_writer cd_writer = {
> > .writer = {
> > .ops = &arm_smmu_cd_writer_ops,
> > .master = master,
> > },
> > .ssid = ssid,
> > };
> >
> > if (ssid != IOMMU_NO_PASID && cur_valid != target_valid) {
> > if (cur_valid)
> > master->cd_table.used_ssids--;
> > else
> > master->cd_table.used_ssids++;
> > }
> >
> > /* Force a V=0/V=1 update*/
> > __le64 update = target[0] & ~cpu_to_le64(CTXDESC_CD_0_V);
> > entry_set(&cd_writer.writer, cdptr->data, &update, 0, 1);
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 1, NUM_ENTRY_QWORDS - 1);
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1);
> > }
> >
> > void arm_smmu_write_cd_entry_epd0(struct arm_smmu_master *master, int ssid,
> > struct arm_smmu_cd *cdptr,
> > const struct arm_smmu_cd *target)
> > {
> > struct arm_smmu_cd_writer cd_writer = {
> > .writer = {
> > .ops = &arm_smmu_cd_writer_ops,
> > .master = master,
> > },
> > .ssid = ssid,
> > };
> >
> > /*
> > * Target must the EPD0 = 1 version of the existing CD entry, caller
> > * must enforce it. Assume used_ssids doesn't need updating
> > * for this reason.
> > */
> > /* Update EPD0 */
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1);
> > /* Update everthing else */
> > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, NUM_ENTRY_QWORDS - 1);
> > }
> >
> > IMOH, at this point it is saner to have just implemented the used
> > function and use the mechanism robustly. Less special cases, less
> > fragility, less duplication.
> >
>
> But that adds extra cost of adding ops, indirection, modifying STE
> code..., for a case that is not common, so I think special casing it
> is actually better for readability and maintainability.
The above is what the special case would have to look like. This
programming work is not trivial. The programmer code was always
intended to be re-used for the CD and STE together so there is only
one logic, not two or three copies.
Reducing duplicated logic in such a tricky area is worth it, IMHO.
Jason
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH v1 1/4] mm: Introduce ptep_get_lockless_norecency()
From: David Hildenbrand @ 2024-03-27 17:02 UTC (permalink / raw)
To: Ryan Roberts, Mark Rutland, Catalin Marinas, Will Deacon,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrew Morton, Muchun Song
Cc: linux-arm-kernel, linux-mm, linux-kernel
In-Reply-To: <31b903f8-99dd-4790-8338-f4b9950b1ee6@arm.com>
>>>>
>>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>>> index 68283e54c899..41dc44eb8454 100644
>>>>> --- a/mm/hugetlb.c
>>>>> +++ b/mm/hugetlb.c
>>>>> @@ -7517,7 +7517,7 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct
>>>>> vm_area_struct *vma,
>>>>> }
>>>>>
>>>>> if (pte) {
>>>>> - pte_t pteval = ptep_get_lockless(pte);
>>>>> + pte_t pteval = ptep_get_lockless_norecency(pte);
>>>>>
>>>>> BUG_ON(pte_present(pteval) && !pte_huge(pteval));
>>>>> }
>>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>>>> index 2771fc043b3b..1a6c9ed8237a 100644
>>>>> --- a/mm/khugepaged.c
>>>>> +++ b/mm/khugepaged.c
>>>>> @@ -1019,7 +1019,7 @@ static int __collapse_huge_page_swapin(struct mm_struct
>>>>> *mm,
>>>>> }
>>>>> }
>>>>>
>>>>> - vmf.orig_pte = ptep_get_lockless(pte);
>>>>> + vmf.orig_pte = ptep_get_lockless_norecency(pte);
>>>>> if (!is_swap_pte(vmf.orig_pte))
>>>>> continue;
>>>>
>>>>
>>>> Hm, I think you mentioned that we want to be careful with vmf.orig_pte.
>>>
>>> Yeah good point. So I guess this should move to patch 3 (which may be dropped -
>>> tbd)?
>>>
>>
>> Yes. Or a separate one where you explain in detail why do_swap_page() can handle
>> it just fine.
>
> Ahh no wait - I remember now; the reason I believe this is a "trivial" case is
> because we only leak vmf.orig_pte to the rest of the world if its a swap entry.
Ugh, yes!
--
Cheers,
David / dhildenb
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH v1 3/4] mm/memory: Use ptep_get_lockless_norecency() for orig_pte
From: David Hildenbrand @ 2024-03-27 17:05 UTC (permalink / raw)
To: Ryan Roberts, Mark Rutland, Catalin Marinas, Will Deacon,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrew Morton, Muchun Song
Cc: linux-arm-kernel, linux-mm, linux-kernel
In-Reply-To: <de03fcd0-53fe-4672-b148-7a5eda19be03@arm.com>
On 27.03.24 10:51, Ryan Roberts wrote:
> On 26/03/2024 17:58, David Hildenbrand wrote:
>>>>>>
>>>>>> vmf->orig_pte = ptep_get_lockless_norecency(vmf->pte)
>>>>>> /* not dirty */
>>>>>>
>>>>>> /* Now, thread 2 ends up setting the PTE dirty under PT lock. */
>>>
>>> Ahh, this comment about thread 2 is not referring to the code immediately below
>>> it. It all makes much more sense now. :)
>>
>> Sorry :)
>>
>>>
>>>>>>
>>>>>> spin_lock(vmf->ptl);
>>>>>> entry = vmf->orig_pte;
>>>>>> if (unlikely(!pte_same(ptep_get(vmf->pte), entry))) {
>>>>>> ...
>>>>>> }
>>>>>> ...
>>>>>> entry = pte_mkyoung(entry);
>>>>>
>>>>> Do you mean pte_mkdirty() here? You're talking about dirty everywhere else.
>>>>
>>>> No, that is just thread 1 seeing "oh, nothing to do" and then goes ahead and
>>>> unconditionally does that in handle_pte_fault().
>>>>
>>>>>
>>>>>> if (ptep_set_access_flags(vmf->vma, ...)
>>>>>> ...
>>>>>> pte_unmap_unlock(vmf->pte, vmf->ptl);
>>>>>>
>>>>>>
>>>>>> Generic ptep_set_access_flags() will do another pte_same() check and realize
>>>>>> "hey, there was a change!" let's update the PTE!
>>>>>>
>>>>>> set_pte_at(vma->vm_mm, address, ptep, entry);
>>>>>
>>>>> This is called from the generic ptep_set_access_flags() in your example, right?
>>>>>
>>>>
>>>> Yes.
>>>>
>>>>>>
>>>>>> would overwrite the dirty bit set by thread 2.
>>>>>
>>>>> I'm not really sure what you are getting at... Is your concern that there is a
>>>>> race where the page could become dirty in the meantime and it now gets lost? I
>>>>> think that's why arm64 overrides ptep_set_access_flags(); since the hw can
>>>>> update access/dirty we have to deal with the races.
>>>>
>>>> My concern is that your patch can in subtle ways lead to use losing PTE dirty
>>>> bits on architectures that don't have the HW-managed dirty bit. They do exist ;)
>>>
>>> But I think the example you give can already happen today? Thread 1 reads
>>> orig_pte = ptep_get_lockless(). So that's already racy, if thread 2 is going to
>>> set dirty just after the get, then thread 1 is going to set the PTE back to (a
>>> modified version of) orig_pte. Isn't it already broken?
>>
>> No, because the pte_same() check under PTL would have detected it, and we would
>> have backed out. And I think the problem comes to live when we convert
>> pte_same()->pte_same_norecency(), because we fail to protect PTE access/dirty
>> changes that happend under PTL from another thread.
>
> Ahh yep. Got it. I absolutely knew that you would be correct, but I still walked
> right into it!
>
> I think one could argue that the generic ptep_set_access_flags() is not
> implementing its own spec:
>
> "
> ... Only sets the access flags (dirty, accessed), as well as write permission.
> Furthermore, we know it always gets set to a "more permissive" setting ...
> "
>
> Surely it should be folding the access and dirty bits from *ptep into entry if
> they are set?
Likely yes. Unless it's also used to clear access/dirty (don't think so,
and would not be documented).
But the simplification made sense for now, because you previously
checked that pte_same(), and nobody can modify it concurrently.
>
> Regardless, I think this example proves that its fragile and subtle. I'm not
> really sure how to fix it more generally/robustly. Any thoughts? If not perhaps
> we are better off keeping ptep_get_lockless() around and only using
> ptep_get_lockless_norecency() for the really obviously correct cases?
Maybe one of the "sources of problems" is that we have a
ptep_get_lockless_norecency() call followed by a pte_same() check, like
done here.
Not the source of all problems I believe, though ...
--
Cheers,
David / dhildenb
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/4] remoteproc: Add TEE support
From: Mathieu Poirier @ 2024-03-27 17:07 UTC (permalink / raw)
To: Arnaud POULIQUEN
Cc: Bjorn Andersson, Jens Wiklander, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-stm32, linux-arm-kernel, linux-remoteproc,
linux-kernel, op-tee, devicetree
In-Reply-To: <5bb9f583-351d-45a4-90e1-ce0b8dde8ce2@foss.st.com>
On Tue, Mar 26, 2024 at 08:18:23PM +0100, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 3/25/24 17:46, Mathieu Poirier wrote:
> > On Fri, Mar 08, 2024 at 03:47:05PM +0100, Arnaud Pouliquen wrote:
> >> Add a remoteproc TEE (Trusted Execution Environment) driver
> >> that will be probed by the TEE bus. If the associated Trusted
> >> application is supported on secure part this device offers a client
> >
> > Device or driver? I thought I touched on that before.
>
> Right, I changed the first instance and missed this one
>
> >
> >> interface to load a firmware in the secure part.
> >> This firmware could be authenticated by the secure trusted application.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >> Updates from V3:
> >> - rework TEE_REMOTEPROC description in Kconfig
> >> - fix some namings
> >> - add tee_rproc_parse_fw to support rproc_ops::parse_fw
> >> - add proc::tee_interface;
> >> - add rproc struct as parameter of the tee_rproc_register() function
> >> ---
> >> drivers/remoteproc/Kconfig | 10 +
> >> drivers/remoteproc/Makefile | 1 +
> >> drivers/remoteproc/tee_remoteproc.c | 434 ++++++++++++++++++++++++++++
> >> include/linux/remoteproc.h | 4 +
> >> include/linux/tee_remoteproc.h | 112 +++++++
> >> 5 files changed, 561 insertions(+)
> >> create mode 100644 drivers/remoteproc/tee_remoteproc.c
> >> create mode 100644 include/linux/tee_remoteproc.h
> >>
> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >> index 48845dc8fa85..2cf1431b2b59 100644
> >> --- a/drivers/remoteproc/Kconfig
> >> +++ b/drivers/remoteproc/Kconfig
> >> @@ -365,6 +365,16 @@ config XLNX_R5_REMOTEPROC
> >>
> >> It's safe to say N if not interested in using RPU r5f cores.
> >>
> >> +
> >> +config TEE_REMOTEPROC
> >> + tristate "remoteproc support by a TEE application"
> >
> > s/remoteproc/Remoteproc
> >
> >> + depends on OPTEE
> >> + help
> >> + Support a remote processor with a TEE application. The Trusted
> >> + Execution Context is responsible for loading the trusted firmware
> >> + image and managing the remote processor's lifecycle.
> >> + This can be either built-in or a loadable module.
> >> +
> >> endif # REMOTEPROC
> >>
> >> endmenu
> >> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> >> index 91314a9b43ce..fa8daebce277 100644
> >> --- a/drivers/remoteproc/Makefile
> >> +++ b/drivers/remoteproc/Makefile
> >> @@ -36,6 +36,7 @@ obj-$(CONFIG_RCAR_REMOTEPROC) += rcar_rproc.o
> >> obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> >> obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> >> obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> >> +obj-$(CONFIG_TEE_REMOTEPROC) += tee_remoteproc.o
> >> obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> >> obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
> >> obj-$(CONFIG_XLNX_R5_REMOTEPROC) += xlnx_r5_remoteproc.o
> >> diff --git a/drivers/remoteproc/tee_remoteproc.c b/drivers/remoteproc/tee_remoteproc.c
> >> new file mode 100644
> >> index 000000000000..c855210e52e3
> >> --- /dev/null
> >> +++ b/drivers/remoteproc/tee_remoteproc.c
> >> @@ -0,0 +1,434 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2024 - All Rights Reserved
> >> + * Author: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> + */
> >> +
> >> +#include <linux/firmware.h>
> >> +#include <linux/io.h>
> >> +#include <linux/module.h>
> >> +#include <linux/remoteproc.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/tee_drv.h>
> >> +#include <linux/tee_remoteproc.h>
> >> +
> >> +#include "remoteproc_internal.h"
> >> +
> >> +#define MAX_TEE_PARAM_ARRY_MEMBER 4
> >> +
> >> +/*
> >> + * Authentication of the firmware and load in the remote processor memory
> >> + *
> >> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >> + * [in] params[1].memref: buffer containing the image of the buffer
> >> + */
> >> +#define TA_RPROC_FW_CMD_LOAD_FW 1
> >> +
> >> +/*
> >> + * Start the remote processor
> >> + *
> >> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >> + */
> >> +#define TA_RPROC_FW_CMD_START_FW 2
> >> +
> >> +/*
> >> + * Stop the remote processor
> >> + *
> >> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >> + */
> >> +#define TA_RPROC_FW_CMD_STOP_FW 3
> >> +
> >> +/*
> >> + * Return the address of the resource table, or 0 if not found
> >> + * No check is done to verify that the address returned is accessible by
> >> + * the non secure context. If the resource table is loaded in a protected
> >> + * memory the access by the non secure context will lead to a data abort.
> >> + *
> >> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >> + * [out] params[1].value.a: 32bit LSB resource table memory address
> >> + * [out] params[1].value.b: 32bit MSB resource table memory address
> >> + * [out] params[2].value.a: 32bit LSB resource table memory size
> >> + * [out] params[2].value.b: 32bit MSB resource table memory size
> >> + */
> >> +#define TA_RPROC_FW_CMD_GET_RSC_TABLE 4
> >> +
> >> +/*
> >> + * Return the address of the core dump
> >> + *
> >> + * [in] params[0].value.a: unique 32bit identifier of the remote processor
> >> + * [out] params[1].memref: address of the core dump image if exist,
> >> + * else return Null
> >> + */
> >> +#define TA_RPROC_FW_CMD_GET_COREDUMP 5
> >> +
> >> +struct tee_rproc_context {
> >> + struct list_head sessions;
> >> + struct tee_context *tee_ctx;
> >> + struct device *dev;
> >> +};
> >> +
> >> +static struct tee_rproc_context *tee_rproc_ctx;
> >> +
> >> +static void tee_rproc_prepare_args(struct tee_rproc *trproc, int cmd,
> >> + struct tee_ioctl_invoke_arg *arg,
> >> + struct tee_param *param,
> >> + unsigned int num_params)
> >> +{
> >> + memset(arg, 0, sizeof(*arg));
> >> + memset(param, 0, MAX_TEE_PARAM_ARRY_MEMBER * sizeof(*param));
> >> +
> >> + arg->func = cmd;
> >> + arg->session = trproc->session_id;
> >> + arg->num_params = num_params + 1;
> >> +
> >> + param[0] = (struct tee_param) {
> >> + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> >> + .u.value.a = trproc->rproc_id,
> >> + };
> >> +}
> >> +
> >> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
> >> +{
> >> + struct tee_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = rproc->tee_interface;
> >> + struct tee_shm *fw_shm;
> >> + int ret;
> >
> > Declarations in reverse ascending order here and everywhere in the driver.
> > Sometimes it is done properly, sometimes it isn't.
> >
> >> +
> >> + if (!trproc)
> >> + return -EINVAL;
> >> +
> >> + fw_shm = tee_shm_register_kernel_buf(tee_rproc_ctx->tee_ctx, (void *)fw->data, fw->size);
> >> + if (IS_ERR(fw_shm))
> >> + return PTR_ERR(fw_shm);
> >> +
> >> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_LOAD_FW, &arg, param, 1);
> >> +
> >> + /* Provide the address of the firmware image */
> >> + param[1] = (struct tee_param) {
> >> + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
> >> + .u.memref = {
> >> + .shm = fw_shm,
> >> + .size = fw->size,
> >> + .shm_offs = 0,
> >> + },
> >> + };
> >> +
> >> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >> + if (ret < 0 || arg.ret != 0) {
> >> + dev_err(tee_rproc_ctx->dev,
> >> + "TA_RPROC_FW_CMD_LOAD_FW invoke failed TEE err: %x, ret:%x\n",
> >> + arg.ret, ret);
> >> + if (!ret)
> >> + ret = -EIO;
> >> + }
> >> +
> >> + tee_shm_free(fw_shm);
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_load_fw);
> >> +
> >> +struct resource_table *tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> >> +{
> >> + struct tee_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = rproc->tee_interface;
> >> + struct resource_table *rsc_table;
> >> + int ret;
> >> +
> >> + if (!trproc)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_GET_RSC_TABLE, &arg, param, 2);
> >> +
> >> + param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> >> + param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> >> +
> >> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >> + if (ret < 0 || arg.ret != 0) {
> >> + dev_err(tee_rproc_ctx->dev,
> >> + "TA_RPROC_FW_CMD_GET_RSC_TABLE invoke failed TEE err: %x, ret:%x\n",
> >> + arg.ret, ret);
> >> + return ERR_PTR(-EIO);
> >> + }
> >> +
> >> + *table_sz = param[2].u.value.a;
> >> +
> >> + /* If the size is null no resource table defined in the image */
> >> + if (!*table_sz)
> >> + return NULL;
> >> +
> >> + /* Store the resource table address that would be updated by the remote core. */
> >> + rsc_table = ioremap_wc(param[1].u.value.a, *table_sz);
> >> + if (IS_ERR_OR_NULL(rsc_table)) {
> >> + dev_err(tee_rproc_ctx->dev, "Unable to map memory region: %lld+%zx\n",
> >> + param[1].u.value.a, *table_sz);
> >> + return ERR_PTR(-ENOMEM);
> >> + }
> >> +
> >> + return rsc_table;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_get_loaded_rsc_table);
> >> +
> >> +int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> >> +{
> >> + struct tee_rproc *trproc = rproc->tee_interface;
> >> + struct resource_table *rsc_table;
> >> + size_t table_sz;
> >> + int ret;
> >> +
> >> + ret = tee_rproc_load_fw(rproc, fw);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
> >> + if (IS_ERR(rsc_table))
> >> + return PTR_ERR(rsc_table);
> >> +
> >> + /* Create a copy of the resource table to have same behaviour than the elf loader. */
> >> + rproc->cached_table = kmemdup(rsc_table, table_sz, GFP_KERNEL);
> >> + if (!rproc->cached_table)
> >> + return -ENOMEM;
> >
> > Why not ->table_ptr and setting ->cached_table to NULL?
>
> It was my plan preparing this version. But during implementarion it looks
> to me that having exactly same behavior than the ELF loader would be
> simpler to maintain the remoteproc avoiding to update in the remoteproc core
> to manage for the cached resource table (see also my comment below abourt recovery)
> That why I propose this implementation
>
> That said what you proposal should also work (with some updates in
> remoteproc_core for the management of the cached table).
>
Yes
> So please just comfirm your preference.
>
Definitely keep ->cached_table to NULL.
> >
> >> +
> >> + rproc->table_ptr = rproc->cached_table;
> >> + rproc->table_sz = table_sz;
> >> + trproc->rsc_table = rsc_table;
> >
> > I really don't see why this is needed, please remove and use rproc->table_ptr
> > instead.
>
> I need to store it for the iounmap in tee_rproc_remove.
iounmap(entry->rproc->rsc_table);
What am I missing?
>
> >
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_parse_fw);
> >> +
> >> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> >> + const struct firmware *fw)
> >> +{
> >> + struct tee_rproc *trproc = rproc->tee_interface;
> >> + struct resource_table *rsc_table;
> >> + size_t table_sz;
> >> +
> >> + if (!trproc)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + /* Check if the resourse table has already been obtained in tee_rproc_parse_fw() */
> >> + if (trproc->rsc_table)
> >> + return trproc->rsc_table;
> >
> > Again, why not simply use rproc->rsc_table? This function should only return
> > the resource table that was set in tee_rproc_parse_fw().
>
> In case of recovery rproc->_rsc_table point to the cached table [1]
In [1], on line 1554, add a check for rproc->tee_interface and if it is valid
call rproc_find_loaded_rsc_table().
> and we need to reapply the configuration in rproc_start() called during the
> recovery[2]
1) Rename rproc_set_rsc_table() to rproc_set_rsc_table_on_attach()
2) Introduce a new function called rproc_set_rsc_table_on_start()
3) Move code from [2], line 1278 to 1292, to that new function. In the new
function, add a check on rproc->tee_interface. If it is valid then call
rproc_find_loaded_rsc_table().
4) in rproc_start(), replace lines 1278 to 1292 with a call to
rproc_set_rsc_table_on_start().
> [1]https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_core.c#L1586
> [2]https://elixir.bootlin.com/linux/latest/source/drivers/remoteproc/remoteproc_core.c#L1287
>
> >
> >> +
> >> + rsc_table = tee_rproc_get_loaded_rsc_table(rproc, &table_sz);
> >> + if (IS_ERR(rsc_table))
> >> + return rsc_table;
> >> +
> >> + rproc->table_sz = table_sz;
> >> + trproc->rsc_table = rsc_table;
> >> +
> >> + return rsc_table;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_find_loaded_rsc_table);
> >> +
> >> +int tee_rproc_start(struct rproc *rproc)
> >> +{
> >> + struct tee_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = rproc->tee_interface;
> >> + int ret;
> >> +
> >> + if (!trproc)
> >> + return -EINVAL;
> >> +
> >> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_START_FW, &arg, param, 0);
> >> +
> >> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >> + if (ret < 0 || arg.ret != 0) {
> >> + dev_err(tee_rproc_ctx->dev,
> >> + "TA_RPROC_FW_CMD_START_FW invoke failed TEE err: %x, ret:%x\n",
> >> + arg.ret, ret);
> >> + if (!ret)
> >> + ret = -EIO;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_start);
> >> +
> >> +int tee_rproc_stop(struct rproc *rproc)
> >> +{
> >> + struct tee_ioctl_invoke_arg arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc = rproc->tee_interface;
> >> + int ret;
> >> +
> >> + if (!trproc)
> >> + return -EINVAL;
> >> +
> >> + tee_rproc_prepare_args(trproc, TA_RPROC_FW_CMD_STOP_FW, &arg, param, 0);
> >> +
> >> + ret = tee_client_invoke_func(tee_rproc_ctx->tee_ctx, &arg, param);
> >> + if (ret < 0 || arg.ret != 0) {
> >> + dev_err(tee_rproc_ctx->dev,
> >> + "TA_RPROC_FW_CMD_STOP_FW invoke failed TEE err: %x, ret:%x\n",
> >> + arg.ret, ret);
> >> + if (!ret)
> >> + ret = -EIO;
> >> + }
> >> +
> >> + if (!rproc->table_ptr)
> >> + return ret;
> >> +
> >> + iounmap(trproc->rsc_table);
> >> + trproc->rsc_table = NULL;
> >> + rproc->table_ptr = NULL;
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_stop);
> >> +
> >> +static const struct tee_client_device_id stm32_tee_rproc_id_table[] = {
> >> + {UUID_INIT(0x80a4c275, 0x0a47, 0x4905,
> >> + 0x82, 0x85, 0x14, 0x86, 0xa9, 0x77, 0x1a, 0x08)},
> >> + {}
> >> +};
> >> +
> >> +struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc, unsigned int rproc_id)
> >> +{
> >> + struct tee_client_device *tee_device;
> >> + struct tee_ioctl_open_session_arg sess_arg;
> >> + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMBER];
> >> + struct tee_rproc *trproc;
> >> + int ret;
> >> +
> >> + /*
> >> + * Test if the device has been probed by the TEE bus. In case of failure, we ignore the
> >> + * reason. The bus could be not yet probed or the service not available in the secure
> >> + * firmware.The assumption in such a case is that the TEE remoteproc is not probed.
> >> + */
> >> + if (!tee_rproc_ctx)
> >> + return ERR_PTR(-EPROBE_DEFER);
> >> +
> >> + trproc = devm_kzalloc(dev, sizeof(*trproc), GFP_KERNEL);
> >> + if (!trproc)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + tee_device = to_tee_client_device(tee_rproc_ctx->dev);
> >> + memset(&sess_arg, 0, sizeof(sess_arg));
> >> +
> >> + memcpy(sess_arg.uuid, tee_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
> >> +
> >> + sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> >> + sess_arg.num_params = 1;
> >> +
> >> + param[0] = (struct tee_param) {
> >> + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> >> + .u.value.a = rproc_id,
> >> + };
> >> +
> >> + ret = tee_client_open_session(tee_rproc_ctx->tee_ctx, &sess_arg, param);
> >> + if (ret < 0 || sess_arg.ret != 0) {
> >> + dev_err(dev, "tee_client_open_session failed, err: %x\n", sess_arg.ret);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + trproc->parent = dev;
> >> + trproc->rproc_id = rproc_id;
> >> + trproc->session_id = sess_arg.session;
> >> +
> >> + trproc->rproc = rproc;
> >> + rproc->tee_interface = trproc;
> >> +
> >> + list_add_tail(&trproc->node, &tee_rproc_ctx->sessions);
> >> +
> >> + return trproc;
> >
> > Once this function was called by a client, what prevents a user from unloading
> > the tee_remoteproc module and breaking everything?
>
> Good point! seems better toremove the module build capability
>
I was thinking more along the lines of try_module_get() and module_put() to
avoid bloating the core.
> Thanks,
> Arnaud
>
> >
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_register);
> >> +
> >> +int tee_rproc_unregister(struct tee_rproc *trproc)
> >> +{
> >
> > If you pass a struct_rproc instead of a struct tee_rproc there is no need for
> > tee_rproc::rproc, which is only ever used in this function.
> >
> >
> >> + struct rproc *rproc = trproc->rproc;
> >> + int ret;
> >> +
> >> + ret = tee_client_close_session(tee_rproc_ctx->tee_ctx, trproc->session_id);
> >> + if (ret < 0)
> >> + dev_err(trproc->parent, "tee_client_close_session failed, err: %x\n", ret);
> >> +
> >> + list_del(&trproc->node);
> >> + rproc->tee_interface = NULL;
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tee_rproc_unregister);
> >> +
> >> +static int tee_rproc_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> >> +{
> >> + /* Today we support only the OP-TEE, could be extend to other tees */
> >> + return (ver->impl_id == TEE_IMPL_ID_OPTEE);
> >> +}
> >> +
> >> +static int tee_rproc_probe(struct device *dev)
> >> +{
> >> + struct tee_context *tee_ctx;
> >> + int ret;
> >> +
> >> + /* Open context with TEE driver */
> >> + tee_ctx = tee_client_open_context(NULL, tee_rproc_ctx_match, NULL, NULL);
> >> + if (IS_ERR(tee_ctx))
> >> + return PTR_ERR(tee_ctx);
> >> +
> >> + tee_rproc_ctx = devm_kzalloc(dev, sizeof(*tee_ctx), GFP_KERNEL);
> >> + if (!tee_rproc_ctx) {
> >> + ret = -ENOMEM;
> >> + goto err;
> >> + }
> >> +
> >> + tee_rproc_ctx->dev = dev;
> >> + tee_rproc_ctx->tee_ctx = tee_ctx;
> >> + INIT_LIST_HEAD(&tee_rproc_ctx->sessions);
> >> +
> >> + return 0;
> >> +err:
> >> + tee_client_close_context(tee_ctx);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int tee_rproc_remove(struct device *dev)
> >> +{
> >> + struct tee_rproc *entry, *tmp;
> >> +
> >> + list_for_each_entry_safe(entry, tmp, &tee_rproc_ctx->sessions, node) {
> >> + tee_client_close_session(tee_rproc_ctx->tee_ctx, entry->session_id);
> >> + list_del(&entry->node);
> >> + if (entry->rsc_table)
> >> + iounmap(entry->rsc_table);
> >> + kfree(entry);
> >> + }
> >> +
> >> + tee_client_close_context(tee_rproc_ctx->tee_ctx);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +MODULE_DEVICE_TABLE(tee, stm32_tee_rproc_id_table);
> >> +
> >> +static struct tee_client_driver tee_rproc_fw_driver = {
> >> + .id_table = stm32_tee_rproc_id_table,
> >> + .driver = {
> >> + .name = KBUILD_MODNAME,
> >> + .bus = &tee_bus_type,
> >> + .probe = tee_rproc_probe,
> >> + .remove = tee_rproc_remove,
> >> + },
> >> +};
> >> +
> >> +static int __init tee_rproc_fw_mod_init(void)
> >> +{
> >> + return driver_register(&tee_rproc_fw_driver.driver);
> >> +}
> >> +
> >> +static void __exit tee_rproc_fw_mod_exit(void)
> >> +{
> >> + driver_unregister(&tee_rproc_fw_driver.driver);
> >> +}
> >> +
> >> +module_init(tee_rproc_fw_mod_init);
> >> +module_exit(tee_rproc_fw_mod_exit);
> >> +
> >> +MODULE_DESCRIPTION(" TEE remote processor control driver");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index b4795698d8c2..8b678009e481 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -503,6 +503,8 @@ enum rproc_features {
> >> RPROC_MAX_FEATURES,
> >> };
> >>
> >> +struct tee_rproc;
> >> +
> >> /**
> >> * struct rproc - represents a physical remote processor device
> >> * @node: list node of this rproc object
> >> @@ -545,6 +547,7 @@ enum rproc_features {
> >> * @cdev: character device of the rproc
> >> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> >> * @features: indicate remoteproc features
> >> + * @tee_interface: pointer to the remoteproc tee context
> >> */
> >> struct rproc {
> >> struct list_head node;
> >> @@ -586,6 +589,7 @@ struct rproc {
> >> struct cdev cdev;
> >> bool cdev_put_on_release;
> >> DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
> >> + struct tee_rproc *tee_interface;
> >> };
> >>
> >> /**
> >> diff --git a/include/linux/tee_remoteproc.h b/include/linux/tee_remoteproc.h
> >> new file mode 100644
> >> index 000000000000..571e47190d02
> >> --- /dev/null
> >> +++ b/include/linux/tee_remoteproc.h
> >> @@ -0,0 +1,112 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright(c) 2024 STMicroelectronics - All Rights Reserved
> >> + */
> >> +
> >> +#ifndef TEE_REMOTEPROC_H
> >> +#define TEE_REMOTEPROC_H
> >> +
> >> +#include <linux/tee_drv.h>
> >> +#include <linux/firmware.h>
> >> +#include <linux/remoteproc.h>
> >> +
> >> +struct rproc;
> >> +
> >> +/**
> >> + * struct tee_rproc - TEE remoteproc structure
> >> + * @node: Reference in list
> >> + * @rproc: Remoteproc reference
> >> + * @parent: Parent device
> >> + * @rproc_id: Identifier of the target firmware
> >> + * @session_id: TEE session identifier
> >> + * @rsc_table: Resource table virtual address.
> >> + */
> >> +struct tee_rproc {
> >> + struct list_head node;
> >> + struct rproc *rproc;
> >> + struct device *parent;
> >> + u32 rproc_id;
> >> + u32 session_id;
> >> + struct resource_table *rsc_table;
> >> +};
> >> +
> >> +#if IS_REACHABLE(CONFIG_TEE_REMOTEPROC)
> >> +
> >> +struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc,
> >> + unsigned int rproc_id);
> >> +int tee_rproc_unregister(struct tee_rproc *trproc);
> >> +int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw);
> >> +int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw);
> >> +struct resource_table *tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz);
> >> +struct resource_table *tee_rproc_find_loaded_rsc_table(struct rproc *rproc,
> >> + const struct firmware *fw);
> >> +int tee_rproc_start(struct rproc *rproc);
> >> +int tee_rproc_stop(struct rproc *rproc);
> >> +
> >> +#else
> >> +
> >> +static inline struct tee_rproc *tee_rproc_register(struct device *dev, struct rproc *rproc,
> >> + unsigned int rproc_id)
> >> +{
> >> + return ERR_PTR(-ENODEV);
> >> +}
> >> +
> >> +static int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline int tee_rproc_unregister(struct tee_rproc *trproc)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline int tee_rproc_load_fw(struct rproc *rproc, const struct firmware *fw)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline int tee_rproc_start(struct rproc *rproc)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline int tee_rproc_stop(struct rproc *rproc)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline struct resource_table *
> >> +tee_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +static inline struct resource_table *
> >> +tee_rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
> >> +{
> >> + /* This shouldn't be possible */
> >> + WARN_ON(1);
> >> +
> >> + return NULL;
> >> +}
> >> +#endif /* CONFIG_TEE_REMOTEPROC */
> >> +#endif /* TEE_REMOTEPROC_H */
> >> --
> >> 2.25.1
> >>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 4/4] remoteproc: stm32: Add support of an OP-TEE TA to load the firmware
From: Mathieu Poirier @ 2024-03-27 17:14 UTC (permalink / raw)
To: Arnaud POULIQUEN
Cc: Bjorn Andersson, Jens Wiklander, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-stm32, linux-arm-kernel, linux-remoteproc,
linux-kernel, op-tee, devicetree
In-Reply-To: <a08add21-b8ff-434a-9689-6af8b05b1965@foss.st.com>
On Tue, Mar 26, 2024 at 08:31:33PM +0100, Arnaud POULIQUEN wrote:
>
>
> On 3/25/24 17:51, Mathieu Poirier wrote:
> > On Fri, Mar 08, 2024 at 03:47:08PM +0100, Arnaud Pouliquen wrote:
> >> The new TEE remoteproc device is used to manage remote firmware in a
> >> secure, trusted context. The 'st,stm32mp1-m4-tee' compatibility is
> >> introduced to delegate the loading of the firmware to the trusted
> >> execution context. In such cases, the firmware should be signed and
> >> adhere to the image format defined by the TEE.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >> Updates from V3:
> >> - remove support of the attach use case. Will be addressed in a separate
> >> thread,
> >> - add st_rproc_tee_ops::parse_fw ops,
> >> - inverse call of devm_rproc_alloc()and tee_rproc_register() to manage cross
> >> reference between the rproc struct and the tee_rproc struct in tee_rproc.c.
> >> ---
> >> drivers/remoteproc/stm32_rproc.c | 60 +++++++++++++++++++++++++++++---
> >> 1 file changed, 56 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >> index 8cd838df4e92..13df33c78aa2 100644
> >> --- a/drivers/remoteproc/stm32_rproc.c
> >> +++ b/drivers/remoteproc/stm32_rproc.c
> >> @@ -20,6 +20,7 @@
> >> #include <linux/remoteproc.h>
> >> #include <linux/reset.h>
> >> #include <linux/slab.h>
> >> +#include <linux/tee_remoteproc.h>
> >> #include <linux/workqueue.h>
> >>
> >> #include "remoteproc_internal.h"
> >> @@ -49,6 +50,9 @@
> >> #define M4_STATE_STANDBY 4
> >> #define M4_STATE_CRASH 5
> >>
> >> +/* Remote processor unique identifier aligned with the Trusted Execution Environment definitions */
> >
> > Why is this the case? At least from the kernel side it is possible to call
> > tee_rproc_register() with any kind of value, why is there a need to be any
> > kind of alignment with the TEE?
>
>
> The use of the proc_id is to identify a processor in case of multi co-processors.
>
That is well understood.
> For instance we can have a system with A DSP and a modem. We would use the same
> TEE service, but
That too.
> the TEE driver will probably be different, same for the signature key.
What TEE driver are we talking about here?
> In such case the proc ID allows to identify the the processor you want to address.
>
That too is well understood, but there is no alignment needed with the TEE, i.e
the TEE application is not expecting a value of '0'. We could set
STM32_MP1_M4_PROC_ID to 0xDEADBEEF and things would work. This driver won't go
anywhere for as long as it is not the case.
>
> >
> >> +#define STM32_MP1_M4_PROC_ID 0
> >> +
> >> struct stm32_syscon {
> >> struct regmap *map;
> >> u32 reg;
> >> @@ -257,6 +261,19 @@ static int stm32_rproc_release(struct rproc *rproc)
> >> return 0;
> >> }
> >>
> >> +static int stm32_rproc_tee_stop(struct rproc *rproc)
> >> +{
> >> + int err;
> >> +
> >> + stm32_rproc_request_shutdown(rproc);
> >> +
> >> + err = tee_rproc_stop(rproc);
> >> + if (err)
> >> + return err;
> >> +
> >> + return stm32_rproc_release(rproc);
> >> +}
> >> +
> >> static int stm32_rproc_prepare(struct rproc *rproc)
> >> {
> >> struct device *dev = rproc->dev.parent;
> >> @@ -693,8 +710,19 @@ static const struct rproc_ops st_rproc_ops = {
> >> .get_boot_addr = rproc_elf_get_boot_addr,
> >> };
> >>
> >> +static const struct rproc_ops st_rproc_tee_ops = {
> >> + .prepare = stm32_rproc_prepare,
> >> + .start = tee_rproc_start,
> >> + .stop = stm32_rproc_tee_stop,
> >> + .kick = stm32_rproc_kick,
> >> + .load = tee_rproc_load_fw,
> >> + .parse_fw = tee_rproc_parse_fw,
> >> + .find_loaded_rsc_table = tee_rproc_find_loaded_rsc_table,
> >> +};
> >> +
> >> static const struct of_device_id stm32_rproc_match[] = {
> >> - { .compatible = "st,stm32mp1-m4" },
> >> + {.compatible = "st,stm32mp1-m4",},
> >> + {.compatible = "st,stm32mp1-m4-tee",},
> >> {},
> >> };
> >> MODULE_DEVICE_TABLE(of, stm32_rproc_match);
> >> @@ -853,6 +881,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >> struct device *dev = &pdev->dev;
> >> struct stm32_rproc *ddata;
> >> struct device_node *np = dev->of_node;
> >> + struct tee_rproc *trproc = NULL;
> >> struct rproc *rproc;
> >> unsigned int state;
> >> int ret;
> >> @@ -861,9 +890,26 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >> if (ret)
> >> return ret;
> >>
> >> - rproc = devm_rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
> >> - if (!rproc)
> >> - return -ENOMEM;
> >> + if (of_device_is_compatible(np, "st,stm32mp1-m4-tee")) {
> >> + /*
> >> + * Delegate the firmware management to the secure context.
> >> + * The firmware loaded has to be signed.
> >> + */
> >> + rproc = devm_rproc_alloc(dev, np->name, &st_rproc_tee_ops, NULL, sizeof(*ddata));
> >> + if (!rproc)
> >> + return -ENOMEM;
> >> +
> >> + trproc = tee_rproc_register(dev, rproc, STM32_MP1_M4_PROC_ID);
> >> + if (IS_ERR(trproc)) {
> >> + dev_err_probe(dev, PTR_ERR(trproc),
> >> + "signed firmware not supported by TEE\n");
> >> + return PTR_ERR(trproc);
> >> + }
> >> + } else {
> >> + rproc = devm_rproc_alloc(dev, np->name, &st_rproc_ops, NULL, sizeof(*ddata));
> >> + if (!rproc)
> >> + return -ENOMEM;
> >> + }
> >>
> >> ddata = rproc->priv;
> >>
> >> @@ -915,6 +961,9 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >> dev_pm_clear_wake_irq(dev);
> >> device_init_wakeup(dev, false);
> >> }
> >> + if (trproc)
> >
> > if (rproc->tee_interface)
> >
> >
> > I am done reviewing this set.
>
> Thank for your review!
> Arnaud
>
> >
> > Thanks,
> > Mathieu
> >
> >> + tee_rproc_unregister(trproc);
> >> +
> >> return ret;
> >> }
> >>
> >> @@ -935,6 +984,9 @@ static void stm32_rproc_remove(struct platform_device *pdev)
> >> dev_pm_clear_wake_irq(dev);
> >> device_init_wakeup(dev, false);
> >> }
> >> + if (rproc->tee_interface)
> >> + tee_rproc_unregister(rproc->tee_interface);
> >> +
> >> }
> >>
> >> static int stm32_rproc_suspend(struct device *dev)
> >> --
> >> 2.25.1
> >>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 0/3] Speed up boot with faster linear map creation
From: Ryan Roberts @ 2024-03-27 16:11 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Catalin Marinas, Will Deacon, Mark Rutland, David Hildenbrand,
Donald Dutile, Eric Chanudet, linux-arm-kernel, linux-kernel
In-Reply-To: <CAMj1kXGVNWrm6=sWixL3PiXr1DQN_buvm1YxQW2+zALgd3b-hA@mail.gmail.com>
On 27/03/2024 15:57, Ard Biesheuvel wrote:
> On Wed, 27 Mar 2024 at 17:01, Ryan Roberts <ryan.roberts@arm.com> wrote:
>>
>> On 27/03/2024 13:36, Ard Biesheuvel wrote:
>>> On Wed, 27 Mar 2024 at 12:43, Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>
>>>> On 27/03/2024 10:09, Ard Biesheuvel wrote:
> ...
>>>
>>> I think a mix of the fixmap approach with a 1:1 map could work here:
>>> - use TTBR0 to create a temp 1:1 map of DRAM
>>> - map page tables lazily as they are allocated but using a coarse mapping
>>> - avoid all TLB maintenance except at the end when tearing down the 1:1 mapping.
>>
>> Yes that could work I think. So to make sure I've understood:
>>
>> - create a 1:1 map for all of DRAM using block and cont mappings where possible
>> - use memblock_phys_alloc_*() to allocate pgtable memory
>> - access via fixmap (should be minimal due to block mappings)
>
> Yes but you'd only need the fixmap for pages that are not in the 1:1
> map yet, so after an initial ramp up you wouldn't need it at all,
> assuming locality of memblock allocations and the use of PMD mappings.
> The only tricky thing here is ensuring that we are not mapping memory
> that we shouldn't be touching.
That sounds a bit nasty though. I think it would be simpler to just reuse the
machinery we have, doing the 1:1 map using blocks and fixmap; It should be a
factor of 512 better than what we have, so probably not a problem at that point.
That way, we can rely on memblock to tell us what to map. If its still
problematic I can add a layer to support 1G mappings too.
>
>> - install it in TTBR0
>> - create all the swapper mappings as normal (no block or cont mappings)
>> - use memblock_phys_alloc_*() to alloc pgtable memory
>> - phys address is also virtual address due to installed 1:1 map
>> - Remove 1:1 map from TTBR0
>> - memblock_phys_free() all the memory associated with 1:1 map
>>
>
> Indeed.
One question on the state of TTBR0 at entrance to paging_init(); what is it? I
need to know so I can set it back after.
Currently I'm thinking I can do:
cpu_install_ttbr0(my_dram_idmap, TCR_T0SZ(vabits_actual));
<create swapper>
cpu_set_reserved_ttbr0();
local_flush_tlb_all();
But is it ok to leave the reserved pdg in ttbr0, or is it expecting something else?
>
>> That sounds doable on top of the first 2 patches in this series - I'll have a
>> crack. The only missing piece is depth-first 1:1 map traversal to free the
>> tables. I'm guessing something already exists that I can repurpose?
>>
>
> Not that I am aware of, but that doesn't sound too complicated.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/6] arm64: dts: qcom: qcs6490-rb3gen2: Add DP output
From: Konrad Dybcio @ 2024-03-27 17:20 UTC (permalink / raw)
To: Bjorn Andersson, cros-qcom-dts-watchers, Bjorn Andersson,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Catalin Marinas,
Will Deacon, Dmitry Baryshkov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240326-rb3gen2-dp-connector-v2-2-a9f1bc32ecaf@quicinc.com>
On 27.03.2024 3:04 AM, Bjorn Andersson wrote:
> The RB3Gen2 board comes with a mini DP connector, describe this, enable
> MDSS, DP controller and the PHY that drives this.
>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 4/6] arm64: dts: qcom: qcs6490-rb3gen2: Introduce USB redriver
From: Konrad Dybcio @ 2024-03-27 17:20 UTC (permalink / raw)
To: Bjorn Andersson, cros-qcom-dts-watchers, Bjorn Andersson,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Catalin Marinas,
Will Deacon, Dmitry Baryshkov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240326-rb3gen2-dp-connector-v2-4-a9f1bc32ecaf@quicinc.com>
On 27.03.2024 3:04 AM, Bjorn Andersson wrote:
> The RB3gen2 has a USB redriver on APPS_I2C, enable the bus and introduce
> the redriver. The plumbing with other components is kept separate for
> clarity.
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 5/6] arm64: dts: qcom: qcs6490-rb3gen2: Enable USB Type-C display
From: Konrad Dybcio @ 2024-03-27 17:20 UTC (permalink / raw)
To: Bjorn Andersson, cros-qcom-dts-watchers, Bjorn Andersson,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Catalin Marinas,
Will Deacon, Dmitry Baryshkov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-arm-kernel,
Neil Armstrong, Krishna Kurapati PSSNV
In-Reply-To: <20240326-rb3gen2-dp-connector-v2-5-a9f1bc32ecaf@quicinc.com>
On 27.03.2024 3:04 AM, Bjorn Andersson wrote:
> With the ADSP remoteproc loaded pmic_glink can be introduced and
> together with the redriver wired up to provide role and orientation
> switching signals as well as USB Type-C display on the RB3gen2.
>
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> Tested-By: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line
From: Conor Dooley @ 2024-03-27 17:21 UTC (permalink / raw)
To: Folker Schwesinger
Cc: Dragan Simic, Vinod Koul, Kishon Vijay Abraham I, Heiko Stuebner,
Chris Ruehl, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Christopher Obbard, Alban Browaeys, Doug Anderson, Brian Norris,
Jensen Huang, linux-phy, linux-arm-kernel, linux-rockchip,
linux-kernel, devicetree
In-Reply-To: <D04O57YQHYI4.1QNGSVKVT44CS@folker-schwesinger.de>
[-- Attachment #1.1: Type: text/plain, Size: 1965 bytes --]
On Wed, Mar 27, 2024 at 04:21:45PM +0000, Folker Schwesinger wrote:
> Hi Conor and Dragan,
>
> thanks for your feedback!
>
> On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> > On 2024-03-26 20:46, Conor Dooley wrote:
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > > Relay wrote:
> > >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> > >>
> > >> Restore the behavior of the Rockchip kernel that undconditionally
> > >> enables the internal strobe pulldown.
> > >
> > > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > > mainline behave the same as the rockchip kernel previously? If not,
> > > using "restore" here is misleading. "Unconditionally" is also
> > > incorrect,
> > > because you have a property that disables it.
>
> Apologizes for the misleading commit message. Prior to 5.11 the Linux
> kernel did not touch the pull-down registers. However, it seems the
> register's (factory?) default was set to enable the pull-down. As it
> was mentioned elsewhere that was the configuration recommended by
> Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
> pull-down in its kernel.
Yeah, seems like a bit of a sticky situation. Probably the wrong
polarity was chosen when the property was implemented and the property
should have been the one you wanted to switch to given the default
before it existed was the factory defaults.
> Of course, this has nothing to do with the Linux kernel, so "restore"
> was a bad choice here.
>
> I previously had split the driver patch into two separate patches, one
> for changing the default (unconditionally at that point), the other for
> adding the disable property. As both changes were minimal I decided to
> squash the commits. I updated the cover letter, but forgot to update the
> commit message. Sorry.
No worries. Squashing them was probably the right thing to do anyway.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/2] media: dt-binding: media: Document rk3588’s VEPU121
From: Conor Dooley @ 2024-03-27 17:23 UTC (permalink / raw)
To: Emmanuel Gil Peyrot
Cc: linux-kernel, Ezequiel Garcia, Philipp Zabel,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Joerg Roedel, Will Deacon,
Robin Murphy, Sebastian Reichel, Cristian Ciocaltea, Dragan Simic,
Shreeya Patel, Chris Morgan, Andy Yan, Nicolas Frattaroli,
linux-media, linux-rockchip, devicetree, linux-arm-kernel, iommu
In-Reply-To: <20240327134115.424846-2-linkmauve@linkmauve.fr>
[-- Attachment #1.1: Type: text/plain, Size: 1501 bytes --]
On Wed, Mar 27, 2024 at 02:41:11PM +0100, Emmanuel Gil Peyrot wrote:
> This encoder-only device is present four times on this SoC, and should
> support everything the rk3568 vepu supports (so JPEG, H.264 and VP8
> encoding).
>
> According to the TRM[1], there is also the VEPU580 encoder which
> supports H.264 and H.265, and various VDPU* decoders, of which only the
> VDPU981 is currently supported. This patch describes only the VEPU121.
>
> [1] https://github.com/FanX-Tek/rk3588-TRM-and-Datasheet
>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> ---
> .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> index 9d90d8d0565a..4c6cb21da041 100644
> --- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> @@ -15,8 +15,12 @@ description:
>
> properties:
> compatible:
> - enum:
> - - rockchip,rk3568-vepu
> + oneOf:
> + - const: rockchip,rk3568-vepu
> + - items:
> + - enum:
> + - rockchip,rk3588-vepu121
> + - const: rockchip,rk3568-vepu
>
> reg:
> maxItems: 1
> --
> 2.44.0
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/1] arm64: mm: swap: support THP_SWAP on hardware with MTE
From: Matthew Wilcox @ 2024-03-27 17:34 UTC (permalink / raw)
To: Ryan Roberts
Cc: David Hildenbrand, Barry Song, catalin.marinas, will, akpm, hughd,
linux-mm, linux-arm-kernel, chrisl, mark.rutland, steven.price,
Barry Song, Kemeng Shi, Anshuman Khandual, Peter Collingbourne,
Yosry Ahmed, Peter Xu, Lorenzo Stoakes, Mike Rapoport (IBM),
Aneesh Kumar K.V, Rick Edgecombe
In-Reply-To: <775d6fb4-f555-4edc-a3c2-2f5cfde3d9b6@arm.com>
On Wed, Mar 27, 2024 at 03:15:33PM +0000, Ryan Roberts wrote:
> >>> Can we go further than this patch and only support PG_mte_tagged and
> >>> PG_mte_lock on folio->flags instead of page->flags? We're down to using
>
> Also, I wouldn't want to hold this patch up in order to do this extra work. I
> think the 2 things are orthogonal? (supporting THP swap with MTE vs not using
> tail page flags for MTE). Can we discuss and handle it separately?
Yes, I'm not trying to hold this patch hostage.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/2] media: dt-binding: media: Document rk3588’s VEPU121
From: Sebastian Reichel @ 2024-03-27 17:44 UTC (permalink / raw)
To: Emmanuel Gil Peyrot
Cc: linux-kernel, Ezequiel Garcia, Philipp Zabel,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Joerg Roedel, Will Deacon,
Robin Murphy, Cristian Ciocaltea, Dragan Simic, Shreeya Patel,
Chris Morgan, Andy Yan, Nicolas Frattaroli, linux-media,
linux-rockchip, devicetree, linux-arm-kernel, iommu
In-Reply-To: <20240327134115.424846-2-linkmauve@linkmauve.fr>
[-- Attachment #1.1: Type: text/plain, Size: 1518 bytes --]
Hi,
On Wed, Mar 27, 2024 at 02:41:11PM +0100, Emmanuel Gil Peyrot wrote:
> This encoder-only device is present four times on this SoC, and should
> support everything the rk3568 vepu supports (so JPEG, H.264 and VP8
> encoding).
>
> According to the TRM[1], there is also the VEPU580 encoder which
> supports H.264 and H.265, and various VDPU* decoders, of which only the
> VDPU981 is currently supported. This patch describes only the VEPU121.
>
> [1] https://github.com/FanX-Tek/rk3588-TRM-and-Datasheet
>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
-- Sebastian
> .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> index 9d90d8d0565a..4c6cb21da041 100644
> --- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> @@ -15,8 +15,12 @@ description:
>
> properties:
> compatible:
> - enum:
> - - rockchip,rk3568-vepu
> + oneOf:
> + - const: rockchip,rk3568-vepu
> + - items:
> + - enum:
> + - rockchip,rk3588-vepu121
> + - const: rockchip,rk3568-vepu
>
> reg:
> maxItems: 1
> --
> 2.44.0
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: rockchip: Add VEPU121 to rk3588
From: Sebastian Reichel @ 2024-03-27 17:45 UTC (permalink / raw)
To: Emmanuel Gil Peyrot
Cc: linux-kernel, Ezequiel Garcia, Philipp Zabel,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Joerg Roedel, Will Deacon,
Robin Murphy, Cristian Ciocaltea, Dragan Simic, Shreeya Patel,
Chris Morgan, Andy Yan, Nicolas Frattaroli, linux-media,
linux-rockchip, devicetree, linux-arm-kernel, iommu
In-Reply-To: <20240327134115.424846-3-linkmauve@linkmauve.fr>
[-- Attachment #1.1: Type: text/plain, Size: 4157 bytes --]
Hi,
On Wed, Mar 27, 2024 at 02:41:12PM +0100, Emmanuel Gil Peyrot wrote:
> The TRM (version 1.0 page 385) lists five VEPU121 cores, but only four
> interrupts are listed (on page 24), so I’ve only enabled four of them
> for now.
>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
-- Sebastian
> arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 80 +++++++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index 87b83c87bd55..510ed3db9d01 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -2488,6 +2488,86 @@ gpio4: gpio@fec50000 {
> };
> };
>
> + jpeg_enc0: video-codec@fdba0000 {
> + compatible = "rockchip,rk3588-vepu121", "rockchip,rk3568-vepu";
> + reg = <0x0 0xfdba0000 0x0 0x800>;
> + interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER0>, <&cru HCLK_JPEG_ENCODER0>;
> + clock-names = "aclk", "hclk";
> + iommus = <&jpeg_enc0_mmu>;
> + power-domains = <&power RK3588_PD_VDPU>;
> + };
> +
> + jpeg_enc0_mmu: iommu@fdba0800 {
> + compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
> + reg = <0x0 0xfdba0800 0x0 0x40>;
> + interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER0>, <&cru HCLK_JPEG_ENCODER0>;
> + clock-names = "aclk", "iface";
> + power-domains = <&power RK3588_PD_VDPU>;
> + #iommu-cells = <0>;
> + };
> +
> + jpeg_enc1: video-codec@fdba4000 {
> + compatible = "rockchip,rk3588-vepu121", "rockchip,rk3568-vepu";
> + reg = <0x0 0xfdba4000 0x0 0x800>;
> + interrupts = <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER1>, <&cru HCLK_JPEG_ENCODER1>;
> + clock-names = "aclk", "hclk";
> + iommus = <&jpeg_enc1_mmu>;
> + power-domains = <&power RK3588_PD_VDPU>;
> + };
> +
> + jpeg_enc1_mmu: iommu@fdba4800 {
> + compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
> + reg = <0x0 0xfdba4800 0x0 0x40>;
> + interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER1>, <&cru HCLK_JPEG_ENCODER1>;
> + clock-names = "aclk", "iface";
> + power-domains = <&power RK3588_PD_VDPU>;
> + #iommu-cells = <0>;
> + };
> +
> + jpeg_enc2: video-codec@fdba8000 {
> + compatible = "rockchip,rk3588-vepu121", "rockchip,rk3568-vepu";
> + reg = <0x0 0xfdba8000 0x0 0x800>;
> + interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER2>, <&cru HCLK_JPEG_ENCODER2>;
> + clock-names = "aclk", "hclk";
> + iommus = <&jpeg_enc2_mmu>;
> + power-domains = <&power RK3588_PD_VDPU>;
> + };
> +
> + jpeg_enc2_mmu: iommu@fdba8800 {
> + compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
> + reg = <0x0 0xfdba8800 0x0 0x40>;
> + interrupts = <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER2>, <&cru HCLK_JPEG_ENCODER2>;
> + clock-names = "aclk", "iface";
> + power-domains = <&power RK3588_PD_VDPU>;
> + #iommu-cells = <0>;
> + };
> +
> + jpeg_enc3: video-codec@fdbac000 {
> + compatible = "rockchip,rk3588-vepu121", "rockchip,rk3568-vepu";
> + reg = <0x0 0xfdbac000 0x0 0x800>;
> + interrupts = <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER3>, <&cru HCLK_JPEG_ENCODER3>;
> + clock-names = "aclk", "hclk";
> + iommus = <&jpeg_enc3_mmu>;
> + power-domains = <&power RK3588_PD_VDPU>;
> + };
> +
> + jpeg_enc3_mmu: iommu@fdbac800 {
> + compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
> + reg = <0x0 0xfdbac800 0x0 0x40>;
> + interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru ACLK_JPEG_ENCODER3>, <&cru HCLK_JPEG_ENCODER3>;
> + clock-names = "aclk", "iface";
> + power-domains = <&power RK3588_PD_VDPU>;
> + #iommu-cells = <0>;
> + };
> +
> av1d: video-codec@fdc70000 {
> compatible = "rockchip,rk3588-av1-vpu";
> reg = <0x0 0xfdc70000 0x0 0x800>;
> --
> 2.44.0
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] dmaengine: xilinx: xdma: Fix synchronization issue
From: Lizhi Hou @ 2024-03-27 17:46 UTC (permalink / raw)
To: Louis Chauvet, Brian Xu, Raj Kumar Rampelli, Vinod Koul,
Michal Simek, Jan Kuliga, Miquel Raynal
Cc: dmaengine, linux-arm-kernel, linux-kernel, stable
In-Reply-To: <20240327-digigram-xdma-fixes-v1-2-45f4a52c0283@bootlin.com>
On 3/27/24 02:58, Louis Chauvet wrote:
> The current xdma_synchronize method does not properly wait for the last
> transfer to be done. Due to limitations of the XMDA engine, it is not
> possible to stop a transfer in the middle of a descriptor. Said
> otherwise, if a stop is requested at the end of descriptor "N" and the OS
> is fast enough, the DMA controller will effectively stop immediately.
> However, if the OS is slightly too slow to request the stop and the DMA
> engine starts descriptor "N+1", the N+1 transfer will be performed until
> its end. This means that after a terminate_all, the last descriptor must
> remain valid and the synchronization must wait for this last descriptor to
> be terminated.
>
> Fixes: 855c2e1d1842 ("dmaengine: xilinx: xdma: Rework xdma_terminate_all()")
> Fixes: f5c392d106e7 ("dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks")
> Cc: stable@vger.kernel.org
> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/dma/xilinx/xdma-regs.h | 3 +++
> drivers/dma/xilinx/xdma.c | 26 ++++++++++++++++++--------
> 2 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xdma-regs.h b/drivers/dma/xilinx/xdma-regs.h
> index 98f5f6fb9ff9..6ad08878e938 100644
> --- a/drivers/dma/xilinx/xdma-regs.h
> +++ b/drivers/dma/xilinx/xdma-regs.h
> @@ -117,6 +117,9 @@ struct xdma_hw_desc {
> CHAN_CTRL_IE_WRITE_ERROR | \
> CHAN_CTRL_IE_DESC_ERROR)
>
> +/* bits of the channel status register */
> +#define XDMA_CHAN_STATUS_BUSY BIT(0)
> +
> #define XDMA_CHAN_STATUS_MASK CHAN_CTRL_START
>
> #define XDMA_CHAN_ERROR_MASK (CHAN_CTRL_IE_DESC_ALIGN_MISMATCH | \
> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> index b9788aa8f6b7..5a3a3293b21b 100644
> --- a/drivers/dma/xilinx/xdma.c
> +++ b/drivers/dma/xilinx/xdma.c
> @@ -71,6 +71,8 @@ struct xdma_chan {
> enum dma_transfer_direction dir;
> struct dma_slave_config cfg;
> u32 irq;
> + struct completion last_interrupt;
> + bool stop_requested;
> };
>
> /**
> @@ -376,6 +378,8 @@ static int xdma_xfer_start(struct xdma_chan *xchan)
> return ret;
>
> xchan->busy = true;
> + xchan->stop_requested = false;
> + reinit_completion(&xchan->last_interrupt);
If stop_requested is true, it should not start another transfer. So I
would suggest to add
if (xchan->stop_requested)
return -ENODEV;
at the beginning of xdma_xfer_start().
xdma_xfer_start() is protected by chan lock.
>
> return 0;
> }
> @@ -387,7 +391,6 @@ static int xdma_xfer_start(struct xdma_chan *xchan)
> static int xdma_xfer_stop(struct xdma_chan *xchan)
> {
> int ret;
> - u32 val;
> struct xdma_device *xdev = xchan->xdev_hdl;
>
> /* clear run stop bit to prevent any further auto-triggering */
> @@ -395,13 +398,7 @@ static int xdma_xfer_stop(struct xdma_chan *xchan)
> CHAN_CTRL_RUN_STOP);
> if (ret)
> return ret;
Above two lines can be removed with your change.
> -
> - /* Clear the channel status register */
> - ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS_RC, &val);
> - if (ret)
> - return ret;
> -
> - return 0;
> + return ret;
> }
>
> /**
> @@ -474,6 +471,8 @@ static int xdma_alloc_channels(struct xdma_device *xdev,
> xchan->xdev_hdl = xdev;
> xchan->base = base + i * XDMA_CHAN_STRIDE;
> xchan->dir = dir;
> + xchan->stop_requested = false;
> + init_completion(&xchan->last_interrupt);
>
> ret = xdma_channel_init(xchan);
> if (ret)
> @@ -521,6 +520,7 @@ static int xdma_terminate_all(struct dma_chan *chan)
> spin_lock_irqsave(&xdma_chan->vchan.lock, flags);
>
> xdma_chan->busy = false;
> + xdma_chan->stop_requested = true;
> vd = vchan_next_desc(&xdma_chan->vchan);
> if (vd) {
> list_del(&vd->node);
> @@ -542,6 +542,13 @@ static int xdma_terminate_all(struct dma_chan *chan)
> static void xdma_synchronize(struct dma_chan *chan)
> {
> struct xdma_chan *xdma_chan = to_xdma_chan(chan);
> + struct xdma_device *xdev = xdma_chan->xdev_hdl;
> + int st = 0;
> +
> + /* If the engine continues running, wait for the last interrupt */
> + regmap_read(xdev->rmap, xdma_chan->base + XDMA_CHAN_STATUS, &st);
> + if (st & XDMA_CHAN_STATUS_BUSY)
> + wait_for_completion_timeout(&xdma_chan->last_interrupt, msecs_to_jiffies(1000));
I suggest to add error message for timeout case.
>
> vchan_synchronize(&xdma_chan->vchan);
> }
> @@ -876,6 +883,9 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
> u32 st;
> bool repeat_tx;
>
> + if (xchan->stop_requested)
> + complete(&xchan->last_interrupt);
> +
This should be moved to the end of function to make sure processing
previous transfer is completed.
out:
if (xchan->stop_requested) {
xchan->busy = false;
complete(&xchan->last_interrupt);
}
spin_unlock(&xchan->vchan.lock);
return IRQ_HANDLED;
Thanks,
Lizhi
> spin_lock(&xchan->vchan.lock);
>
> /* get submitted request */
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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