* Re: [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
From: Paul E. McKenney @ 2019-07-16 18:43 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-9-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:37:04AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it for acpi_ioremaps list traversal.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Given that Rafael acked it, this one looks ready.
Thanx, Paul
> ---
> drivers/acpi/osl.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9c0edf2fc0dd..2f9d0d20b836 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -14,6 +14,7 @@
> #include <linux/slab.h>
> #include <linux/mm.h>
> #include <linux/highmem.h>
> +#include <linux/lockdep.h>
> #include <linux/pci.h>
> #include <linux/interrupt.h>
> #include <linux/kmod.h>
> @@ -80,6 +81,7 @@ struct acpi_ioremap {
>
> static LIST_HEAD(acpi_ioremaps);
> static DEFINE_MUTEX(acpi_ioremap_lock);
> +#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
>
> static void __init acpi_request_region (struct acpi_generic_address *gas,
> unsigned int length, char *desc)
> @@ -206,7 +208,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
> {
> struct acpi_ioremap *map;
>
> - list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> + list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
> if (map->phys <= phys &&
> phys + size <= map->phys + map->size)
> return map;
> @@ -249,7 +251,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> {
> struct acpi_ioremap *map;
>
> - list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> + list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
> if (map->virt <= virt &&
> virt + size <= map->virt + map->size)
> return map;
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2)
From: Paul E. McKenney @ 2019-07-16 18:41 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-7-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:37:02AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu now has support to check for RCU reader sections
> as well as lock. Just use the support in it, instead of explictly
> checking in the caller.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
We need an ack from one of the subsystem maintainers on this one.
Thanx, Paul
> ---
> kernel/workqueue.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 601d61150b65..e882477ebf6e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> !lockdep_is_held(&wq_pool_mutex), \
> "RCU or wq_pool_mutex should be held")
>
> -#define assert_rcu_or_wq_mutex(wq) \
> - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> - !lockdep_is_held(&wq->mutex), \
> - "RCU or wq->mutex should be held")
> -
> #define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
> RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> !lockdep_is_held(&wq->mutex) && \
> @@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> * ignored.
> */
> #define for_each_pwq(pwq, wq) \
> - list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
> - if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
> - else
> + list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
> + lock_is_held(&(wq->mutex).dep_map))
>
> #ifdef CONFIG_DEBUG_OBJECTS_WORK
>
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
From: Paul E. McKenney @ 2019-07-16 18:42 UTC (permalink / raw)
To: Joel Fernandes
Cc: Bjorn Helgaas, linux-kernel, Alexey Kuznetsov, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716040303.GA73383@google.com>
On Tue, Jul 16, 2019 at 12:03:03AM -0400, Joel Fernandes wrote:
> On Mon, Jul 15, 2019 at 03:02:35PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote:
> > > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> > > reader-lock held, because the pci_mmcfg_lock is already held. Make this
> > > known to the list macro so that it fixes new lockdep warnings that
> > > trigger due to lockdep checks added to list_for_each_entry_rcu().
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > Ingo takes care of most patches to this file, but FWIW,
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Thanks.
>
> > I would personally prefer if you capitalized the subject to match the
> > "x86/PCI:" convention that's used fairly consistently in
> > arch/x86/pci/.
> >
> > Also, I didn't apply this to be sure, but it looks like this might
> > make a line or two wider than 80 columns, which I would rewrap if I
> > were applying this.
>
> Updated below is the patch with the nits corrected:
I am OK with this going either way, but it does depend on an earlier
patch.
Thanx, Paul
> ---8<-----------------------
>
> >From 73fab09d7e33ca2110c24215f8ed428c12625dbe Mon Sep 17 00:00:00 2001
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Date: Sat, 1 Jun 2019 15:05:49 -0400
> Subject: [PATCH] x86/PCI: Pass lockdep condition to pcm_mmcfg_list iterator
> (v1)
>
> The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> reader-lock held, because the pci_mmcfg_lock is already held. Make this
> known to the list macro so that it fixes new lockdep warnings that
> trigger due to lockdep checks added to list_for_each_entry_rcu().
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> arch/x86/pci/mmconfig-shared.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 7389db538c30..9e3250ec5a37 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -29,6 +29,7 @@
> static bool pci_mmcfg_running_state;
> static bool pci_mmcfg_arch_init_failed;
> static DEFINE_MUTEX(pci_mmcfg_lock);
> +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
>
> LIST_HEAD(pci_mmcfg_list);
>
> @@ -54,7 +55,8 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
> struct pci_mmcfg_region *cfg;
>
> /* keep list sorted by segment and starting bus number */
> - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
> + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list,
> + pci_mmcfg_lock_held()) {
> if (cfg->segment > new->segment ||
> (cfg->segment == new->segment &&
> cfg->start_bus >= new->start_bus)) {
> @@ -118,7 +120,8 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
> {
> struct pci_mmcfg_region *cfg;
>
> - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
> + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list
> + pci_mmcfg_lock_held())
> if (cfg->segment == segment &&
> cfg->start_bus <= bus && bus <= cfg->end_bus)
> return cfg;
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 02/14] docs: power: add it to to the main documentation index
From: Pavel Machek @ 2019-07-16 18:42 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jonathan Corbet, Rafael J. Wysocki, Len Brown, linux-doc,
linux-pm
In-Reply-To: <95abe2e389f5bfb9dd03d55de384c2b9b5bb78da.1563277838.git.mchehab+samsung@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 801 bytes --]
On Tue 2019-07-16 09:10:41, Mauro Carvalho Chehab wrote:
> The power docs are orphaned at the documentation body.
>
> While it could likely be moved to be inside some guide, I'm opting to just
> adding it to the main index.rst, removing the :orphan: and adding the SPDX
> header.
>
> The reason is similar to what it was done for other driver-specific
> subsystems: the docs there contain a mix of Kernelspace, uAPI and
> admin-guide. So, better to keep them on its own directory,
> while the docs there are not properly classified.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 5/9] driver/core: Convert to use built-in RCU list checking (v1)
From: Paul E. McKenney @ 2019-07-16 18:40 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Greg Kroah-Hartman, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-6-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:37:01AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it in driver core.
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
This one looks ready.
Thanx, Paul
> ---
> drivers/base/base.h | 1 +
> drivers/base/core.c | 10 ++++++++++
> drivers/base/power/runtime.c | 15 ++++++++++-----
> 3 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index b405436ee28e..0d32544b6f91 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -165,6 +165,7 @@ static inline int devtmpfs_init(void) { return 0; }
> /* Device links support */
> extern int device_links_read_lock(void);
> extern void device_links_read_unlock(int idx);
> +extern int device_links_read_lock_held(void);
> extern int device_links_check_suppliers(struct device *dev);
> extern void device_links_driver_bound(struct device *dev);
> extern void device_links_driver_cleanup(struct device *dev);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index da84a73f2ba6..85e82f38717f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -68,6 +68,11 @@ void device_links_read_unlock(int idx)
> {
> srcu_read_unlock(&device_links_srcu, idx);
> }
> +
> +int device_links_read_lock_held(void)
> +{
> + return srcu_read_lock_held(&device_links_srcu);
> +}
> #else /* !CONFIG_SRCU */
> static DECLARE_RWSEM(device_links_lock);
>
> @@ -91,6 +96,11 @@ void device_links_read_unlock(int not_used)
> {
> up_read(&device_links_lock);
> }
> +
> +int device_links_read_lock_held(void)
> +{
> + return lock_is_held(&device_links_lock);
> +}
> #endif /* !CONFIG_SRCU */
>
> /**
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 952a1e7057c7..7a10e8379a70 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -287,7 +287,8 @@ static int rpm_get_suppliers(struct device *dev)
> {
> struct device_link *link;
>
> - list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> + device_links_read_lock_held()) {
> int retval;
>
> if (!(link->flags & DL_FLAG_PM_RUNTIME) ||
> @@ -309,7 +310,8 @@ static void rpm_put_suppliers(struct device *dev)
> {
> struct device_link *link;
>
> - list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> + device_links_read_lock_held()) {
> if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
> continue;
>
> @@ -1640,7 +1642,8 @@ void pm_runtime_clean_up_links(struct device *dev)
>
> idx = device_links_read_lock();
>
> - list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
> + list_for_each_entry_rcu(link, &dev->links.consumers, s_node,
> + device_links_read_lock_held()) {
> if (link->flags & DL_FLAG_STATELESS)
> continue;
>
> @@ -1662,7 +1665,8 @@ void pm_runtime_get_suppliers(struct device *dev)
>
> idx = device_links_read_lock();
>
> - list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> + device_links_read_lock_held())
> if (link->flags & DL_FLAG_PM_RUNTIME) {
> link->supplier_preactivated = true;
> refcount_inc(&link->rpm_active);
> @@ -1683,7 +1687,8 @@ void pm_runtime_put_suppliers(struct device *dev)
>
> idx = device_links_read_lock();
>
> - list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> + list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> + device_links_read_lock_held())
> if (link->supplier_preactivated) {
> link->supplier_preactivated = false;
> if (refcount_dec_not_one(&link->rpm_active))
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1)
From: Paul E. McKenney @ 2019-07-16 18:39 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-5-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:37:00AM -0400, Joel Fernandes (Google) wrote:
> Using the previous support added, use it for adding lockdep conditions
> to list usage here.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
We need an ack or better from the subsystem maintainer for this one.
Thanx, Paul
> ---
> net/ipv4/fib_frontend.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 317339cd7f03..26b0fb24e2c2 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -124,7 +124,8 @@ struct fib_table *fib_get_table(struct net *net, u32 id)
> h = id & (FIB_TABLE_HASHSZ - 1);
>
> head = &net->ipv4.fib_table_hash[h];
> - hlist_for_each_entry_rcu(tb, head, tb_hlist) {
> + hlist_for_each_entry_rcu(tb, head, tb_hlist,
> + lockdep_rtnl_is_held()) {
> if (tb->tb_id == id)
> return tb;
> }
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
From: Paul E. McKenney @ 2019-07-16 18:38 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-3-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> This patch adds support for checking RCU reader sections in list
> traversal macros. Optionally, if the list macro is called under SRCU or
> other lock/mutex protection, then appropriate lockdep expressions can be
> passed to make the checks pass.
>
> Existing list_for_each_entry_rcu() invocations don't need to pass the
> optional fourth argument (cond) unless they are under some non-RCU
> protection and needs to make lockdep check pass.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Now that I am on the correct version, again please fold in the checks
for the extra argument. The ability to have an optional argument looks
quite helpful, especially when compared to growing the RCU API!
A few more things below.
> ---
> include/linux/rculist.h | 28 ++++++++++++++++++++-----
> include/linux/rcupdate.h | 7 +++++++
> kernel/rcu/Kconfig.debug | 11 ++++++++++
> kernel/rcu/update.c | 44 ++++++++++++++++++++++++----------------
> 4 files changed, 67 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..1048160625bb 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> */
> #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
>
> +/*
> + * Check during list traversal that we are within an RCU reader
> + */
> +
> +#ifdef CONFIG_PROVE_RCU_LIST
This new Kconfig option is OK temporarily, but unless there is reason to
fear malfunction that a few weeks of rcutorture, 0day, and -next won't
find, it would be better to just use CONFIG_PROVE_RCU. The overall goal
is to reduce the number of RCU knobs rather than grow them, must though
history might lead one to believe otherwise. :-/
> +#define __list_check_rcu(dummy, cond, ...) \
> + ({ \
> + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> + "RCU-list traversed in non-reader section!"); \
> + })
> +#else
> +#define __list_check_rcu(dummy, cond, ...) ({})
> +#endif
> +
> /*
> * Insert a new entry between two known consecutive entries.
> *
> @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> * @pos: the type * to use as a loop cursor.
> * @head: the head for your list.
> * @member: the name of the list_head within the struct.
> + * @cond: optional lockdep expression if called from non-RCU protection.
> *
> * This list-traversal primitive may safely run concurrently with
> * the _rcu list-mutation primitives such as list_add_rcu()
> * as long as the traversal is guarded by rcu_read_lock().
> */
> -#define list_for_each_entry_rcu(pos, head, member) \
> - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> - &pos->member != (head); \
> +#define list_for_each_entry_rcu(pos, head, member, cond...) \
> + for (__list_check_rcu(dummy, ## cond, 0), \
> + pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> + &pos->member != (head); \
> pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
>
> /**
> @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> * @pos: the type * to use as a loop cursor.
> * @head: the head for your list.
> * @member: the name of the hlist_node within the struct.
> + * @cond: optional lockdep expression if called from non-RCU protection.
> *
> * This list-traversal primitive may safely run concurrently with
> * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> * as long as the traversal is guarded by rcu_read_lock().
> */
> -#define hlist_for_each_entry_rcu(pos, head, member) \
> - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
> + for (__list_check_rcu(dummy, ## cond, 0), \
> + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> typeof(*(pos)), member); \
> pos; \
> pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 8f7167478c1d..f3c29efdf19a 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
> int rcu_read_lock_held(void);
> int rcu_read_lock_bh_held(void);
> int rcu_read_lock_sched_held(void);
> +int rcu_read_lock_any_held(void);
>
> #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
> {
> return !preemptible();
> }
> +
> +static inline int rcu_read_lock_any_held(void)
> +{
> + return !preemptible();
> +}
> +
> #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> #ifdef CONFIG_PROVE_RCU
> diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> index 5ec3ea4028e2..7fbd21dbfcd0 100644
> --- a/kernel/rcu/Kconfig.debug
> +++ b/kernel/rcu/Kconfig.debug
> @@ -8,6 +8,17 @@ menu "RCU Debugging"
> config PROVE_RCU
> def_bool PROVE_LOCKING
>
> +config PROVE_RCU_LIST
> + bool "RCU list lockdep debugging"
> + depends on PROVE_RCU
This must also depend on RCU_EXPERT.
> + default n
> + help
> + Enable RCU lockdep checking for list usages. By default it is
> + turned off since there are several list RCU users that still
> + need to be converted to pass a lockdep expression. To prevent
> + false-positive splats, we keep it default disabled but once all
> + users are converted, we can remove this config option.
> +
> config TORTURE_TEST
> tristate
> default n
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 9dd5aeef6e70..b7a4e3b5fa98 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
> * Similarly, we avoid claiming an SRCU read lock held if the current
> * CPU is offline.
> */
> +#define rcu_read_lock_held_common() \
> + if (!debug_lockdep_rcu_enabled()) \
> + return 1; \
> + if (!rcu_is_watching()) \
> + return 0; \
> + if (!rcu_lockdep_current_cpu_online()) \
> + return 0;
Nice abstraction of common code!
Thanx, Paul
> +
> int rcu_read_lock_sched_held(void)
> {
> - if (!debug_lockdep_rcu_enabled())
> - return 1;
> - if (!rcu_is_watching())
> - return 0;
> - if (!rcu_lockdep_current_cpu_online())
> - return 0;
> + rcu_read_lock_held_common();
> +
> return lock_is_held(&rcu_sched_lock_map) || !preemptible();
> }
> EXPORT_SYMBOL(rcu_read_lock_sched_held);
> @@ -257,12 +261,8 @@ NOKPROBE_SYMBOL(debug_lockdep_rcu_enabled);
> */
> int rcu_read_lock_held(void)
> {
> - if (!debug_lockdep_rcu_enabled())
> - return 1;
> - if (!rcu_is_watching())
> - return 0;
> - if (!rcu_lockdep_current_cpu_online())
> - return 0;
> + rcu_read_lock_held_common();
> +
> return lock_is_held(&rcu_lock_map);
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_held);
> @@ -284,16 +284,24 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_held);
> */
> int rcu_read_lock_bh_held(void)
> {
> - if (!debug_lockdep_rcu_enabled())
> - return 1;
> - if (!rcu_is_watching())
> - return 0;
> - if (!rcu_lockdep_current_cpu_online())
> - return 0;
> + rcu_read_lock_held_common();
> +
> return in_softirq() || irqs_disabled();
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> +int rcu_read_lock_any_held(void)
> +{
> + rcu_read_lock_held_common();
> +
> + if (lock_is_held(&rcu_lock_map) ||
> + lock_is_held(&rcu_bh_lock_map) ||
> + lock_is_held(&rcu_sched_lock_map))
> + return 1;
> + return !preemptible();
> +}
> +EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
> +
> #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> /**
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH 3/9] rcu/sync: Remove custom check for reader-section (v2)
From: Paul E. McKenney @ 2019-07-16 18:39 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Oleg Nesterov, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Pavel Machek, peterz, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715143705.117908-4-joel@joelfernandes.org>
On Mon, Jul 15, 2019 at 10:36:59AM -0400, Joel Fernandes (Google) wrote:
> The rcu/sync code was doing its own check whether we are in a reader
> section. With RCU consolidating flavors and the generic helper added in
> this series, this is no longer need. We can just use the generic helper
> and it results in a nice cleanup.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
This one looks good!
Thanx, Paul
> ---
> include/linux/rcu_sync.h | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> index 9b83865d24f9..0027d4c8087c 100644
> --- a/include/linux/rcu_sync.h
> +++ b/include/linux/rcu_sync.h
> @@ -31,9 +31,7 @@ struct rcu_sync {
> */
> static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> {
> - RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> - !rcu_read_lock_bh_held() &&
> - !rcu_read_lock_sched_held(),
> + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> "suspicious rcu_sync_is_idle() usage");
> return !READ_ONCE(rsp->gp_state); /* GP_IDLE */
> }
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH v2 2/9] rcu: Add support for consolidated-RCU reader checking
From: Joel Fernandes @ 2019-07-16 18:35 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716182237.GA22819@linux.ibm.com>
On Tue, Jul 16, 2019 at 11:22:37AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 12, 2019 at 01:00:17PM -0400, Joel Fernandes (Google) wrote:
> > This patch adds support for checking RCU reader sections in list
> > traversal macros. Optionally, if the list macro is called under SRCU or
> > other lock/mutex protection, then appropriate lockdep expressions can be
> > passed to make the checks pass.
> >
> > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > optional fourth argument (cond) unless they are under some non-RCU
> > protection and needs to make lockdep check pass.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> If you fold in the checks for extra parameters, I will take this
> one and also 1/9.
I folded the checks in and also threw in the rcu-sync with Oleg's ack:
Could you pull into /dev branch?
git pull https://github.com/joelagnel/linux-kernel.git list-first-three
(Based on your dev branch)
^ permalink raw reply
* Re: [PATCH v2 2/9] rcu: Add support for consolidated-RCU reader checking
From: Paul E. McKenney @ 2019-07-16 18:22 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-3-joel@joelfernandes.org>
On Fri, Jul 12, 2019 at 01:00:17PM -0400, Joel Fernandes (Google) wrote:
> This patch adds support for checking RCU reader sections in list
> traversal macros. Optionally, if the list macro is called under SRCU or
> other lock/mutex protection, then appropriate lockdep expressions can be
> passed to make the checks pass.
>
> Existing list_for_each_entry_rcu() invocations don't need to pass the
> optional fourth argument (cond) unless they are under some non-RCU
> protection and needs to make lockdep check pass.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
If you fold in the checks for extra parameters, I will take this
one and also 1/9.
Thanx, Paul
> ---
> include/linux/rculist.h | 28 +++++++++++++++++++++++-----
> include/linux/rcupdate.h | 7 +++++++
> kernel/rcu/Kconfig.debug | 11 +++++++++++
> kernel/rcu/update.c | 14 ++++++++++++++
> 4 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..1048160625bb 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> */
> #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
>
> +/*
> + * Check during list traversal that we are within an RCU reader
> + */
> +
> +#ifdef CONFIG_PROVE_RCU_LIST
> +#define __list_check_rcu(dummy, cond, ...) \
> + ({ \
> + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> + "RCU-list traversed in non-reader section!"); \
> + })
> +#else
> +#define __list_check_rcu(dummy, cond, ...) ({})
> +#endif
> +
> /*
> * Insert a new entry between two known consecutive entries.
> *
> @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> * @pos: the type * to use as a loop cursor.
> * @head: the head for your list.
> * @member: the name of the list_head within the struct.
> + * @cond: optional lockdep expression if called from non-RCU protection.
> *
> * This list-traversal primitive may safely run concurrently with
> * the _rcu list-mutation primitives such as list_add_rcu()
> * as long as the traversal is guarded by rcu_read_lock().
> */
> -#define list_for_each_entry_rcu(pos, head, member) \
> - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> - &pos->member != (head); \
> +#define list_for_each_entry_rcu(pos, head, member, cond...) \
> + for (__list_check_rcu(dummy, ## cond, 0), \
> + pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> + &pos->member != (head); \
> pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
>
> /**
> @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> * @pos: the type * to use as a loop cursor.
> * @head: the head for your list.
> * @member: the name of the hlist_node within the struct.
> + * @cond: optional lockdep expression if called from non-RCU protection.
> *
> * This list-traversal primitive may safely run concurrently with
> * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> * as long as the traversal is guarded by rcu_read_lock().
> */
> -#define hlist_for_each_entry_rcu(pos, head, member) \
> - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
> + for (__list_check_rcu(dummy, ## cond, 0), \
> + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> typeof(*(pos)), member); \
> pos; \
> pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 922bb6848813..712b464ab960 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -223,6 +223,7 @@ int debug_lockdep_rcu_enabled(void);
> int rcu_read_lock_held(void);
> int rcu_read_lock_bh_held(void);
> int rcu_read_lock_sched_held(void);
> +int rcu_read_lock_any_held(void);
>
> #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> @@ -243,6 +244,12 @@ static inline int rcu_read_lock_sched_held(void)
> {
> return !preemptible();
> }
> +
> +static inline int rcu_read_lock_any_held(void)
> +{
> + return !preemptible();
> +}
> +
> #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> #ifdef CONFIG_PROVE_RCU
> diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> index 0ec7d1d33a14..b20d0e2903d1 100644
> --- a/kernel/rcu/Kconfig.debug
> +++ b/kernel/rcu/Kconfig.debug
> @@ -7,6 +7,17 @@ menu "RCU Debugging"
> config PROVE_RCU
> def_bool PROVE_LOCKING
>
> +config PROVE_RCU_LIST
> + bool "RCU list lockdep debugging"
> + depends on PROVE_RCU
> + default n
> + help
> + Enable RCU lockdep checking for list usages. By default it is
> + turned off since there are several list RCU users that still
> + need to be converted to pass a lockdep expression. To prevent
> + false-positive splats, we keep it default disabled but once all
> + users are converted, we can remove this config option.
> +
> config TORTURE_TEST
> tristate
> default n
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index bb961cd89e76..0cc7be0fb6b5 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -294,6 +294,20 @@ int rcu_read_lock_bh_held(void)
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> +int rcu_read_lock_any_held(void)
> +{
> + if (!debug_lockdep_rcu_enabled())
> + return 1;
> + if (!rcu_is_watching())
> + return 0;
> + if (!rcu_lockdep_current_cpu_online())
> + return 0;
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> + return 1;
> + return !preemptible();
> +}
> +EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
> +
> #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> /**
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-16 18:28 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Oleg Nesterov, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Pavel Machek, peterz, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716182642.GB22819@linux.ibm.com>
On Tue, Jul 16, 2019 at 11:26:42AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > The rcu/sync code was doing its own check whether we are in a reader
> > section. With RCU consolidating flavors and the generic helper added in
> > this series, this is no longer need. We can just use the generic helper
> > and it results in a nice cleanup.
> >
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> This needs to be forward-ported to current mainline. (Or, I believe
> equivalently for this file, to branch "dev" of -rcu.)
>
> Especially given that you have Oleg's Ack, I would be happy to
> take the forward-ported version.
Never mind, I am one version behind. Apologies for the noise!
Thanx, Paul
> > ---
> > Please note: Only build and boot tested this particular patch so far.
> >
> > include/linux/rcu_sync.h | 5 ++---
> > kernel/rcu/sync.c | 22 ----------------------
> > 2 files changed, 2 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > index 6fc53a1345b3..c954f1efc919 100644
> > --- a/include/linux/rcu_sync.h
> > +++ b/include/linux/rcu_sync.h
> > @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *);
> > */
> > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > {
> > -#ifdef CONFIG_PROVE_RCU
> > - rcu_sync_lockdep_assert(rsp);
> > -#endif
> > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > + "suspicious rcu_sync_is_idle() usage");
> > return !rsp->gp_state; /* GP_IDLE */
> > }
> >
> > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> > index a8304d90573f..535e02601f56 100644
> > --- a/kernel/rcu/sync.c
> > +++ b/kernel/rcu/sync.c
> > @@ -10,37 +10,25 @@
> > #include <linux/rcu_sync.h>
> > #include <linux/sched.h>
> >
> > -#ifdef CONFIG_PROVE_RCU
> > -#define __INIT_HELD(func) .held = func,
> > -#else
> > -#define __INIT_HELD(func)
> > -#endif
> > -
> > static const struct {
> > void (*sync)(void);
> > void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
> > void (*wait)(void);
> > -#ifdef CONFIG_PROVE_RCU
> > - int (*held)(void);
> > -#endif
> > } gp_ops[] = {
> > [RCU_SYNC] = {
> > .sync = synchronize_rcu,
> > .call = call_rcu,
> > .wait = rcu_barrier,
> > - __INIT_HELD(rcu_read_lock_held)
> > },
> > [RCU_SCHED_SYNC] = {
> > .sync = synchronize_rcu,
> > .call = call_rcu,
> > .wait = rcu_barrier,
> > - __INIT_HELD(rcu_read_lock_sched_held)
> > },
> > [RCU_BH_SYNC] = {
> > .sync = synchronize_rcu,
> > .call = call_rcu,
> > .wait = rcu_barrier,
> > - __INIT_HELD(rcu_read_lock_bh_held)
> > },
> > };
> >
> > @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
> >
> > #define rss_lock gp_wait.lock
> >
> > -#ifdef CONFIG_PROVE_RCU
> > -void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
> > -{
> > - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(),
> > - "suspicious rcu_sync_is_idle() usage");
> > -}
> > -
> > -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert);
> > -#endif
> > -
> > /**
> > * rcu_sync_init() - Initialize an rcu_sync structure
> > * @rsp: Pointer to rcu_sync structure to be initialized
> > --
> > 2.22.0.510.g264f2c817a-goog
> >
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-16 18:26 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Oleg Nesterov, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Pavel Machek, peterz, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-4-joel@joelfernandes.org>
On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> The rcu/sync code was doing its own check whether we are in a reader
> section. With RCU consolidating flavors and the generic helper added in
> this series, this is no longer need. We can just use the generic helper
> and it results in a nice cleanup.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
This needs to be forward-ported to current mainline. (Or, I believe
equivalently for this file, to branch "dev" of -rcu.)
Especially given that you have Oleg's Ack, I would be happy to
take the forward-ported version.
Thanx, Paul
> ---
> Please note: Only build and boot tested this particular patch so far.
>
> include/linux/rcu_sync.h | 5 ++---
> kernel/rcu/sync.c | 22 ----------------------
> 2 files changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> index 6fc53a1345b3..c954f1efc919 100644
> --- a/include/linux/rcu_sync.h
> +++ b/include/linux/rcu_sync.h
> @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *);
> */
> static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> {
> -#ifdef CONFIG_PROVE_RCU
> - rcu_sync_lockdep_assert(rsp);
> -#endif
> + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> + "suspicious rcu_sync_is_idle() usage");
> return !rsp->gp_state; /* GP_IDLE */
> }
>
> diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> index a8304d90573f..535e02601f56 100644
> --- a/kernel/rcu/sync.c
> +++ b/kernel/rcu/sync.c
> @@ -10,37 +10,25 @@
> #include <linux/rcu_sync.h>
> #include <linux/sched.h>
>
> -#ifdef CONFIG_PROVE_RCU
> -#define __INIT_HELD(func) .held = func,
> -#else
> -#define __INIT_HELD(func)
> -#endif
> -
> static const struct {
> void (*sync)(void);
> void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
> void (*wait)(void);
> -#ifdef CONFIG_PROVE_RCU
> - int (*held)(void);
> -#endif
> } gp_ops[] = {
> [RCU_SYNC] = {
> .sync = synchronize_rcu,
> .call = call_rcu,
> .wait = rcu_barrier,
> - __INIT_HELD(rcu_read_lock_held)
> },
> [RCU_SCHED_SYNC] = {
> .sync = synchronize_rcu,
> .call = call_rcu,
> .wait = rcu_barrier,
> - __INIT_HELD(rcu_read_lock_sched_held)
> },
> [RCU_BH_SYNC] = {
> .sync = synchronize_rcu,
> .call = call_rcu,
> .wait = rcu_barrier,
> - __INIT_HELD(rcu_read_lock_bh_held)
> },
> };
>
> @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
>
> #define rss_lock gp_wait.lock
>
> -#ifdef CONFIG_PROVE_RCU
> -void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
> -{
> - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(),
> - "suspicious rcu_sync_is_idle() usage");
> -}
> -
> -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert);
> -#endif
> -
> /**
> * rcu_sync_init() - Initialize an rcu_sync structure
> * @rsp: Pointer to rcu_sync structure to be initialized
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH v2 11/11] interconnect: Add devfreq support
From: Sibi Sankar @ 2019-07-16 18:12 UTC (permalink / raw)
To: Saravana Kannan, Georgi Djakov
Cc: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Rajendra Nayak, Jordan Crouse, Vincent Guittot,
Bjorn Andersson, amit.kucheria, seansw, daidavid1, evgreen,
Android Kernel Team, linux-pm, devicetree, linux-kernel, adharmap
In-Reply-To: <CAGETcx-xU9i1FJB5JecUoyZEfWpD8f+o9bC3SQmb-=3fLVbmQw@mail.gmail.com>
Hey Saravana,
On 6/18/19 2:48 AM, Saravana Kannan wrote:
> On Mon, Jun 17, 2019 at 8:44 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>>
>> Hi Saravana,
>>
>> On 6/14/19 07:17, Saravana Kannan wrote:
>>> Add a icc_create_devfreq() and icc_remove_devfreq() to create and remove
>>> devfreq devices for interconnect paths. A driver can create/remove devfreq
>>> devices for the interconnects needed for its device by calling these APIs.
>>> This would allow various devfreq governors to work with interconnect paths
>>> and the device driver itself doesn't have to actively manage the bandwidth
>>> votes for the interconnects.
>>
>> Thanks for the patches, but creating devfreq devices for each interconnect path
>> seems odd to me - at least for consumers that already use a governor.
>
> Each governor instance always handles one "frequency" (more like
> performance) domain at a time. So if a consumer is already using a
> governor to scale the hardware block, then using another governor to
> scale the interconnect performance points is the right way to go about
> it. In fact, that's exactly what devfreq passive governor's
> documentation even says it's meant for. That's also what cpufreq does
> for each cluster/CPU frequency domain too.
>
>> So for DDR
>> scaling for example, are you suggesting that we add a devfreq device from the
>> cpufreq driver in order to scale the interconnect between CPU<->DDR?
>
> Yes in general. Although, CPUs are a special case because CPUs don't
> go through devfreq. So passive governor as it stands today won't work.
> CPU<->DDR scaling might need a separate governor (unlikely) or some
> changes to the passive governor that I'm happy to work on once we
> settle this for general devices like GPU, etc. But the DT format for
> CPUs will be identical to GPUs or any other device.
using icc_create_devfreq from the cpufreq-hw driver on SDM845 SoC
to scale CPU<->DDR would cause a circular dependency. (i.e) with
the addition of cpufreq notifier to the passive governor as in
https://patchwork.kernel.org/patch/11046147/ devm_devfreq_add_device
would require the cpufreq transistion notifier register and cpu
freq_cpu_get to go through. Please add your thought on addressing this.
>
>> Also if the
>> GPU is already using devfreq, should we add a devfreq per each interconnect
>> path? What would be the benefit in this case - using different governors for
>> bandwidth scaling maybe?
>
> When saying "separate/different governors" in this email, I mean both
> different instance of the same governor logic with different tunables
> AND actually different algorithms/governor logic entirely.
>
> The heuristics to use for each interconnect path might be (more like,
> will be) different based on hardware characteristics (Eg: what voltage
> domains the interconnect is sitting on) and what interconnect
> information is available (Eg: Just busy time vs bandwidth count vs no
> information etc) -- so having separate governors for each interconnect
> path makes a lot of sense. It also allows userspace to control the
> policy for scaling each of those paths based on product use cases.
>
> For example, when the GPU is just doing simple UI rendering, userspace
> can use the max_freq sysfs file for the devfreq device to disallow high
> bandwidth OPPs on the GPU<->DDR path, but those higher OPPs could be
> allowed by userspace when the GPU is used for games. Having devfreq
> device for each interconnect path also make it easy to debug
> performance issues -- you can independently change the votes for each
> path to figure out what is causing the bottleneck, etc.
>
> Adding a devfreq device for interconnect voting with a few lines gives
> all these features "for free".
>
> This doesn't mean all users of interconnect framework NEED to use
> devfreq for interconnect. They might do it simply based on
> calculations based on the use case (Eg: display driver from display
> resolution). But if they are trying to use any kind of
> algorithm/heuristics, writing it as a devfreq governor should be
> encouraged.
>
> Also want to point out that BW OPPs also work for drivers that don't
> use devfreq at all. The interconnect-opp-table just lists the
> meaningful OPP leveld for the path and the device driver can pick one
> entry from the table based on the use case.
>
> Thanks,
> Saravana
>
>
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v11 3/5] sched/core: uclamp: Propagate system defaults to root group
From: Patrick Bellasi @ 2019-07-16 18:00 UTC (permalink / raw)
To: Michal Koutný
Cc: Alessio Balsini, Dietmar Eggemann, Morten Rasmussen,
Quentin Perret, Joel Fernandes, Paul Turner, Steve Muckle,
Suren Baghdasaryan, Todd Kjos, Peter Zijlstra, Rafael J . Wysocki,
Tejun Heo, Vincent Guittot, Viresh Kumar, Juri Lelli, Ingo Molnar,
linux-kernel, linux-pm
In-Reply-To: <20190716153610.GE32540@blackbody.suse.cz>
On 16-Jul 17:36, Michal Koutný wrote:
> On Tue, Jul 16, 2019 at 03:34:17PM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > > cpu_util_update_eff internally calls css_for_each_descendant_pre() so
> > > this should be protected with rcu_read_lock().
> >
> > Right, good catch! Will add in v12.
> When I responded to your other patch, it occurred to me that since
> cpu_util_update_eff goes writing down to child csses, it should take
> also uclamp_mutex here to avoid race with direct cgroup attribute
> writers.
Yep, I should drop the "dedicated" mutex we have now in
sysctl_sched_uclamp_handler() and use the uclamp_mutex we already
have.
Thanks, Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v11 2/5] sched/core: uclamp: Propagate parent clamps
From: Patrick Bellasi @ 2019-07-16 17:55 UTC (permalink / raw)
To: Michal Koutný
Cc: Alessio Balsini, Dietmar Eggemann, Morten Rasmussen,
Quentin Perret, Joel Fernandes, Paul Turner, Steve Muckle,
Suren Baghdasaryan, Todd Kjos, Peter Zijlstra, Rafael J . Wysocki,
Tejun Heo, Vincent Guittot, Viresh Kumar, Juri Lelli, Ingo Molnar,
linux-kernel, linux-pm
In-Reply-To: <20190716152929.GD32540@blackbody.suse.cz>
On 16-Jul 17:29, Michal Koutný wrote:
> On Tue, Jul 16, 2019 at 03:07:06PM +0100, Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > That note comes from the previous review cycle and it's based on a
> > request from Tejun to align uclamp behaviors with the way the
> > delegation model is supposed to work.
> I saw and hopefully understood that reasoning -- uclamp.min has the
> protection semantics and uclamp.max the limit semantics.
>
> However, what took me some time to comprehend when the effected
> uclamp.min and uclamp.max cross over, i.e. that uclamp.min is then bound
> by uclamp.max (besides parent's uclamp.min). Your commit message
> explains that and I think it's relevant for the kernel docs file
> itself.
Right, I've just added a paragraph to the cpu.uclamp.min documentation.
> > You right, the synchronization is introduced by a later patch:
> >
> > sched/core: uclamp: Update CPU's refcount on TG's clamp changes
> I saw that lock but didn't realize __setscheduler_uclamp() touches only
> task's struct uclamp_se, none of task_group's/css's (which is under
> uclamp_mutex). That seems correct.
Right, the mutex is used only on the cgroup side. That's because the
CGroup API can affect multiple tasks running on different CPUs, thus
we wanna make sure we don't come up with race conditions. In that
path we can also afford to go a bit slower.
In the fast path instead we rely on the rq-locks to ensure
serialization on RUNNABLE tasks clamp updates.
Coming from the __setscheduler_uclamp() side however we don't sync
RUNNABLE tasks immediately. We delay the update to next enqueue
opportunity.
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq
From: Sibi Sankar @ 2019-07-16 17:36 UTC (permalink / raw)
To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: kernel-team, linux-pm, linux-kernel, adharmap
In-Reply-To: <20190625213337.157525-4-saravanak@google.com>
Hey Saravana,
On 6/26/19 3:03 AM, Saravana Kannan wrote:
> The OPP table can be used often in devfreq. Trying to get it each time can
> be expensive, so cache it in the devfreq struct.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/devfreq/devfreq.c | 6 ++++++
> include/linux/devfreq.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..ac62b78dc035 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,6 +597,8 @@ static void devfreq_dev_release(struct device *dev)
> if (devfreq->profile->exit)
> devfreq->profile->exit(devfreq->dev.parent);
>
> + if (devfreq->opp_table)
> + dev_pm_opp_put_opp_table(devfreq->opp_table);
> mutex_destroy(&devfreq->lock);
> kfree(devfreq);
> }
> @@ -677,6 +679,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->max_freq = devfreq->scaling_max_freq;
>
> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> + devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
> + if (IS_ERR(devfreq->opp_table))
> + devfreq->opp_table = NULL;
> +
> atomic_set(&devfreq->suspend_count, 0);
>
> dev_set_name(&devfreq->dev, "devfreq%d",
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index fbffa74bfc1b..0d877c9513d7 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -156,6 +156,7 @@ struct devfreq {
> struct devfreq_dev_profile *profile;
> const struct devfreq_governor *governor;
> char governor_name[DEVFREQ_NAME_LEN];
> + struct opp_table *opp_table;
please add it to the function docs as well
> struct notifier_block nb;
> struct delayed_work work;
>
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v3 2/6] OPP: Add support for bandwidth OPP tables
From: Sibi Sankar @ 2019-07-16 17:33 UTC (permalink / raw)
To: Saravana Kannan, Georgi Djakov, Rob Herring, Mark Rutland,
Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: vincent.guittot, seansw, daidavid1, Rajendra Nayak,
bjorn.andersson, evgreen, kernel-team, linux-pm, devicetree,
linux-kernel, adharmap
In-Reply-To: <20190703011020.151615-3-saravanak@google.com>
Hey Saravana,
On 7/3/19 6:40 AM, Saravana Kannan wrote:
> Not all devices quantify their performance points in terms of frequency.
> Devices like interconnects quantify their performance points in terms of
> bandwidth. We need a way to represent these bandwidth levels in OPP. So,
> add support for parsing bandwidth OPPs from DT.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/opp/of.c | 34 ++++++++++++++++++++++++++++++++--
> drivers/opp/opp.h | 4 +++-
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index c10c782d15aa..54fa70ed2adc 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -552,6 +552,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
>
> +static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
> +{
> + int ret;
> + u64 rate;
> + u32 bw;
> +
> + ret = of_property_read_u64(np, "opp-hz", &rate);
> + if (!ret) {
> + /*
> + * Rate is defined as an unsigned long in clk API, and so
> + * casting explicitly to its type. Must be fixed once rate is 64
> + * bit guaranteed in clk API.
> + */
> + new_opp->rate = (unsigned long)rate
now that the rate gets set here, please remove the rate assignment in
_opp_add_static_v2
> + return 0;
> + }
> +
> + ret = of_property_read_u32(np, "opp-peak-KBps", &bw);
> + if (ret)
> + return ret;
> + new_opp->rate = (unsigned long) &bw;
should be bw instead
> +
> + ret = of_property_read_u32(np, "opp-avg-KBps", &bw);
> + if (!ret)
> + new_opp->avg_bw = (unsigned long) &bw;
ditto
> +
> + return 0;
> +}
> +
> /**
> * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> * @opp_table: OPP table
> @@ -589,11 +618,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> if (!new_opp)
> return ERR_PTR(-ENOMEM);
>
> - ret = of_property_read_u64(np, "opp-hz", &rate);
> + ret = _read_opp_key(new_opp, np);
> if (ret < 0) {
> /* "opp-hz" is optional for devices like power domains. */
> if (!opp_table->is_genpd) {
> - dev_err(dev, "%s: opp-hz not found\n", __func__);
> + dev_err(dev, "%s: opp-hz or opp-peak-bw not found\n",
> + __func__);
please remove the else part where rate value will be reset.
> goto free_opp;
> }
>
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 569b3525aa67..ead2cdafe957 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -59,7 +59,8 @@ extern struct list_head opp_tables;
> * @turbo: true if turbo (boost) OPP
> * @suspend: true if suspend OPP
> * @pstate: Device's power domain's performance state.
> - * @rate: Frequency in hertz
> + * @rate: Frequency in hertz OR Peak bandwidth in kilobytes per second
> + * @avg_bw: Average bandwidth in kilobytes per second
> * @level: Performance level
> * @supplies: Power supplies voltage/current values
> * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> @@ -81,6 +82,7 @@ struct dev_pm_opp {
> bool suspend;
> unsigned int pstate;
> unsigned long rate;
> + unsigned long avg_bw;
> unsigned int level;
>
> struct dev_pm_opp_supply *supplies;
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Sibi Sankar @ 2019-07-16 17:25 UTC (permalink / raw)
To: Saravana Kannan, Georgi Djakov, Rob Herring, Mark Rutland,
Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: vincent.guittot, seansw, daidavid1, Rajendra Nayak,
bjorn.andersson, evgreen, kernel-team, linux-pm, devicetree,
linux-kernel
In-Reply-To: <20190703011020.151615-2-saravanak@google.com>
Hey Saravana,
https://patchwork.kernel.org/patch/10850815/
There was already a discussion ^^ on how bandwidth bindings were to be
named.
On 7/3/19 6:40 AM, Saravana Kannan wrote:
> Interconnects often quantify their performance points in terms of
> bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
> allow specifying Bandwidth OPP tables in DT.
>
> opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
> tables.
>
> opp-avg-KBps is an optional property that can be used in Bandwidth OPP
> tables.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 76b6c79604a5..c869e87caa2a 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -83,9 +83,14 @@ properties.
>
> Required properties:
> - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> - required property for all device nodes but devices like power domains. The
> - power domain nodes must have another (implementation dependent) property which
> - uniquely identifies the OPP nodes.
> + required property for all device nodes but for devices like power domains or
> + bandwidth opp tables. The power domain nodes must have another (implementation
> + dependent) property which uniquely identifies the OPP nodes. The interconnect
> + opps are required to have the opp-peak-bw property.
> +
> +- opp-peak-KBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
> + big-endian integer. This is a required property for all devices that don't
> + have opp-hz. For example, bandwidth OPP tables for interconnect paths.
>
> Optional properties:
> - opp-microvolt: voltage in micro Volts.
> @@ -132,6 +137,10 @@ Optional properties:
> - opp-level: A value representing the performance level of the device,
> expressed as a 32-bit integer.
>
> +- opp-avg-KBps: Average bandwidth in kilobytes per second, expressed as a
> + 32-bit big-endian integer. This property is only meaningful in OPP tables
> + where opp-peak-KBps is present.
> +
> - clock-latency-ns: Specifies the maximum possible transition latency (in
> nanoseconds) for switching to this OPP from any other OPP.
>
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 1/4] OPP: Allow required-opps even if the device doesn't have power-domains
From: Sibi Sankar @ 2019-07-16 17:17 UTC (permalink / raw)
To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Chanwoo Choi,
Viresh Kumar, Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: kernel-team, linux-pm, linux-kernel
In-Reply-To: <20190625213337.157525-2-saravanak@google.com>
Hey Saravana,
Thanks for taking time to post out this series.
On 6/26/19 3:03 AM, Saravana Kannan wrote:
> A Device-A can have a (minimum) performance requirement on another
> Device-B to be able to function correctly. This performance requirement
> on Device-B can also change based on the current performance level of
> Device-A.
>
> The existing required-opps feature fits well to describe this need. So,
> instead of limiting required-opps to point to only PM-domain devices,
> allow it to point to any device.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/opp/core.c | 2 +-
> drivers/opp/of.c | 14 --------------
> 2 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..74c7bdc6f463 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -710,7 +710,7 @@ static int _set_required_opps(struct device *dev,
> return 0;
>
> /* Single genpd case */
> - if (!genpd_virt_devs) {
> + if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
https://patchwork.kernel.org/patch/10940671/
This was already removed as a part of ^^ and is in linux-next.
> pstate = opp->required_opps[0]->pstate;
> ret = dev_pm_genpd_set_performance_state(dev, pstate);
> if (ret) {
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index c10c782d15aa..7c8336e94aff 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -195,9 +195,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> */
> count_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells");
> - if (!count_pd)
> - goto put_np;
> -
> if (count_pd > 1) {
> genpd_virt_devs = kcalloc(count, sizeof(*genpd_virt_devs),
> GFP_KERNEL);
> @@ -226,17 +223,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>
> if (IS_ERR(required_opp_tables[i]))
> goto free_required_tables;
> -
> - /*
> - * We only support genpd's OPPs in the "required-opps" for now,
> - * as we don't know how much about other cases. Error out if the
> - * required OPP doesn't belong to a genpd.
> - */
> - if (!required_opp_tables[i]->is_genpd) {
> - dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
> - required_np);
> - goto free_required_tables;
> - }
I expect the series to not work as is in its current state since I
see a circular dependency here. The required-opp tables of the parent
devfreq won't be populated until we add the opp-table of the child
devfreq node while the child devfreq using passive governor would
return -EPROBE_DEFER until the parent devfreq probes.
The same applies to this patch -> https://patchwork.kernel.org/patch
/11046147/ I posted out based on your series. So we would probably have
to address the dependency here.
> }
>
> goto put_np;
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH 8/8] PM: sleep: Drop dpm_noirq_begin() and dpm_noirq_end()
From: Rafael J. Wysocki @ 2019-07-16 16:21 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Note that after previous changes dpm_noirq_begin() and
dpm_noirq_end() each have only one caller, so move the code from
them to their respective callers and drop them.
Also note that dpm_noirq_resume_devices() and
dpm_noirq_suspend_devices() need not be exported any more, so make
them both static.
This change is not expected to alter functionality.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/base/power/main.c | 30 ++++++++++++------------------
include/linux/pm.h | 4 ----
2 files changed, 12 insertions(+), 22 deletions(-)
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -716,7 +716,7 @@ static void async_resume_noirq(void *dat
put_device(dev);
}
-void dpm_noirq_resume_devices(pm_message_t state)
+static void dpm_noirq_resume_devices(pm_message_t state)
{
struct device *dev;
ktime_t starttime = ktime_get();
@@ -760,13 +760,6 @@ void dpm_noirq_resume_devices(pm_message
trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, false);
}
-void dpm_noirq_end(void)
-{
- resume_device_irqs();
- device_wakeup_disarm_wake_irqs();
- cpuidle_resume();
-}
-
/**
* dpm_resume_noirq - Execute "noirq resume" callbacks for all devices.
* @state: PM transition of the system being carried out.
@@ -777,7 +770,11 @@ void dpm_noirq_end(void)
void dpm_resume_noirq(pm_message_t state)
{
dpm_noirq_resume_devices(state);
- dpm_noirq_end();
+
+ resume_device_irqs();
+ device_wakeup_disarm_wake_irqs();
+
+ cpuidle_resume();
}
static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
@@ -1357,14 +1354,7 @@ static int device_suspend_noirq(struct d
return __device_suspend_noirq(dev, pm_transition, false);
}
-void dpm_noirq_begin(void)
-{
- cpuidle_pause();
- device_wakeup_arm_wake_irqs();
- suspend_device_irqs();
-}
-
-int dpm_noirq_suspend_devices(pm_message_t state)
+static int dpm_noirq_suspend_devices(pm_message_t state)
{
ktime_t starttime = ktime_get();
int error = 0;
@@ -1421,7 +1411,11 @@ int dpm_suspend_noirq(pm_message_t state
{
int ret;
- dpm_noirq_begin();
+ cpuidle_pause();
+
+ device_wakeup_arm_wake_irqs();
+ suspend_device_irqs();
+
ret = dpm_noirq_suspend_devices(state);
if (ret)
dpm_resume_noirq(resume_event(state));
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -712,8 +712,6 @@ struct dev_pm_domain {
extern void device_pm_lock(void);
extern void dpm_resume_start(pm_message_t state);
extern void dpm_resume_end(pm_message_t state);
-extern void dpm_noirq_resume_devices(pm_message_t state);
-extern void dpm_noirq_end(void);
extern void dpm_resume_noirq(pm_message_t state);
extern void dpm_resume_early(pm_message_t state);
extern void dpm_resume(pm_message_t state);
@@ -722,8 +720,6 @@ extern void dpm_complete(pm_message_t st
extern void device_pm_unlock(void);
extern int dpm_suspend_end(pm_message_t state);
extern int dpm_suspend_start(pm_message_t state);
-extern void dpm_noirq_begin(void);
-extern int dpm_noirq_suspend_devices(pm_message_t state);
extern int dpm_suspend_noirq(pm_message_t state);
extern int dpm_suspend_late(pm_message_t state);
extern int dpm_suspend(pm_message_t state);
^ permalink raw reply
* [PATCH 5/8] ACPI: PM: Set s2idle_wakeup earlier and clear it later
From: Rafael J. Wysocki @ 2019-07-16 16:15 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
The role of the s2idle_wakeup variable is to cause
acpi_pm_wakeup_event() and acpi_pm_notify_handler() to
increment pm_abort_suspend and trigger a wakeup from
suspend-to-idle in case the ACPI SCI wakeup was canceled
by acpi_s2idle_wake().
However, for this purpose it need not be set in acpi_s2idle_wake()
and cleared in acpi_s2idle_sync(), respectively. In fact, it
may be set as early as in acpi_s2idle_prepare() and cleared as
late as in acpi_s2idle_restore(), so do that to allow subsequent
changes to be simpler.
This change is not expected to alter functionality.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/sleep.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -972,6 +972,8 @@ static int acpi_s2idle_prepare(void)
/* Change the configuration of GPEs to avoid spurious wakeup. */
acpi_enable_all_wakeup_gpes();
acpi_os_wait_events_complete();
+
+ s2idle_wakeup = true;
return 0;
}
@@ -991,7 +993,6 @@ static void acpi_s2idle_wake(void)
if (acpi_sci_irq_valid() &&
!irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) {
pm_system_cancel_wakeup();
- s2idle_wakeup = true;
/*
* On some platforms with the LPS0 _DSM device noirq resume
* takes too much time for EC wakeup events to survive, so look
@@ -1012,11 +1013,12 @@ static void acpi_s2idle_sync(void)
acpi_os_wait_events_complete(); /* synchronize SCI IRQ handling */
acpi_ec_flush_work();
acpi_os_wait_events_complete(); /* synchronize Notify handling */
- s2idle_wakeup = false;
}
static void acpi_s2idle_restore(void)
{
+ s2idle_wakeup = false;
+
acpi_enable_all_runtime_gpes();
acpi_disable_wakeup_devices(ACPI_STATE_S0);
^ permalink raw reply
* [PATCH 3/8] ACPI: EC: Return bool from acpi_ec_dispatch_gpe()
From: Rafael J. Wysocki @ 2019-07-16 16:12 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
On some systems, if suspend-to-idle is used, the EC may singal system
wakeup events (power button events, for example) as well as events
that should not cause the system to resume and acpi_ec_dispatch_gpe()
needs to be called to determine whether or not the system should
resume then. In particular, if acpi_ec_dispatch_gpe() doesn't detect
any EC events at all, the system should remain suspended, so it is
useful to know when that is the case.
For this reason, make acpi_ec_dispatch_gpe() return a bool value
indicating whether or not any EC events have been detected by it.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/ec.c | 11 ++++++++---
drivers/acpi/internal.h | 2 +-
2 files changed, 9 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1060,10 +1060,15 @@ void acpi_ec_set_gpe_wake_mask(u8 action
acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
}
-void acpi_ec_dispatch_gpe(void)
+bool acpi_ec_dispatch_gpe(void)
{
- if (first_ec)
- acpi_dispatch_gpe(NULL, first_ec->gpe);
+ u32 ret;
+
+ if (!first_ec)
+ return false;
+
+ ret = acpi_dispatch_gpe(NULL, first_ec->gpe);
+ return ret == ACPI_INTERRUPT_HANDLED;
}
/* --------------------------------------------------------------------------
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -196,7 +196,7 @@ void acpi_ec_block_transactions(void);
void acpi_ec_unblock_transactions(void);
void acpi_ec_mark_gpe_for_wake(void);
void acpi_ec_set_gpe_wake_mask(u8 action);
-void acpi_ec_dispatch_gpe(void);
+bool acpi_ec_dispatch_gpe(void);
int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
acpi_handle handle, acpi_ec_query_func func,
void *data);
^ permalink raw reply
* [PATCH 6/8] PM: sleep: Simplify suspend-to-idle control flow
From: Rafael J. Wysocki @ 2019-07-16 16:17 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
After commit 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups
from suspend-to-idle") the "noirq" phases of device suspend and
resume may run for multiple times during suspend-to-idle, if there
are spurious system wakeup events while suspended. However, this
is complicated and fragile and actually unnecessary.
The main reason for doing this is that on some systems the EC may
signal system wakeup events (power button events, for example) as
well as events that should not cause the system to resume (spurious
system wakeup events). Thus, in order to determine whether or not
a given event signaled by the EC while suspended is a proper system
wakeup one, the EC GPE needs to be dispatched and to start with that
was achieved by allowing the ACPI SCI action handler to run, which
was only possible after calling resume_device_irqs().
However, dispatching the EC GPE this way turned out to take too much
time in some cases and some EC events might be missed due to that, so
commit 68e22011856f ("ACPI: EC: Dispatch the EC GPE directly on
s2idle wake") started to dispatch the EC GPE right after a wakeup
event has been detected, so in fact the full ACPI SCI action handler
doesn't need to run any more to deal with the wakeups coming from the
EC.
Use this observation to simplify the suspend-to-idle control flow
so that the "noirq" phases of device suspend and resume are each
run only once in every suspend-to-idle cycle, which is reported to
significantly reduce power drawn by some systems when suspended to
idle (by allowing them to reach a deep platform-wide low-power state
through the suspend-to-idle flow). [What appears to happen is that
the "noirq" resume of devices after a spurious EC wakeup brings some
devices into a state in which they prevent the platform from reaching
the deep low-power state going forward, even after a subsequent
"noirq" suspend phase, and on some systems the EC triggers such
wakeups already when the "noirq" suspend of devices is running for
the first time in the given suspend/resume cycle, so the platform
cannot reach the deep low-power state at all.]
First, make acpi_s2idle_wake() use the acpi_ec_dispatch_gpe() return
value to determine whether or not the wakeup may have been triggered
by the EC (in which case the system wakeup is canceled and ACPI
events are processed in order to determine whether or not the event
is a proper system wakeup one) and use rearm_wake_irq() (introduced
by a previous change) in it to rearm the ACPI SCI for system wakeup
detection in case the system will remain suspended.
Second, drop acpi_s2idle_sync(), which is not needed any more, and
the corresponding global platform suspend-to-idle callback.
Next, drop the pm_wakeup_pending() check (which is an optimization
only) from __device_suspend_noirq() to prevent it from returning
errors on system wakeups occurring before the "noirq" phase of
device suspend is complete (as in the case of suspend-to-idle it is
not known whether or not these wakeups are suprious at that point),
in order to avoid having to carry out a "noirq" resume of devices
on a spurious system wakeup.
Finally, change the code flow in s2idle_loop() to (1) run the
"noirq" suspend of devices once before starting the loop, (2) check
for spurious EC wakeups (via the platform ->wake callback) for the
first time before calling s2idle_enter(), and (3) run the "noirq"
resume of devices once after leaving the loop.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/sleep.c | 47 +++++++++++++++++++++-------------------
drivers/base/power/main.c | 5 ----
include/linux/suspend.h | 1
kernel/power/suspend.c | 53 +++++++++++++++++++---------------------------
4 files changed, 48 insertions(+), 58 deletions(-)
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -986,33 +986,37 @@ static void acpi_s2idle_wake(void)
lpi_check_constraints();
/*
- * If IRQD_WAKEUP_ARMED is not set for the SCI at this point, it means
- * that the SCI has triggered while suspended, so cancel the wakeup in
- * case it has not been a wakeup event (the GPEs will be checked later).
+ * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has
+ * not triggered while suspended, so bail out.
*/
- if (acpi_sci_irq_valid() &&
- !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) {
+ if (!acpi_sci_irq_valid() ||
+ irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
+ return;
+
+ /*
+ * If there are EC events to process, the wakeup may be a spurious one
+ * coming from the EC.
+ */
+ if (acpi_ec_dispatch_gpe()) {
+ /*
+ * Cancel the wakeup and process all pending events in case
+ * there are any wakeup ones in there.
+ *
+ * Note that if any non-EC GPEs are active at this point, the
+ * SCI will retrigger after the rearming below, so no events
+ * should be missed by canceling the wakeup here.
+ */
pm_system_cancel_wakeup();
/*
- * On some platforms with the LPS0 _DSM device noirq resume
- * takes too much time for EC wakeup events to survive, so look
- * for them now.
+ * The EC driver uses the system workqueue and an additional
+ * special one, so those need to be flushed too.
*/
- acpi_ec_dispatch_gpe();
+ acpi_os_wait_events_complete(); /* synchronize EC GPE processing */
+ acpi_ec_flush_work();
+ acpi_os_wait_events_complete(); /* synchronize Notify handling */
}
-}
-static void acpi_s2idle_sync(void)
-{
- /*
- * Process all pending events in case there are any wakeup ones.
- *
- * The EC driver uses the system workqueue and an additional special
- * one, so those need to be flushed too.
- */
- acpi_os_wait_events_complete(); /* synchronize SCI IRQ handling */
- acpi_ec_flush_work();
- acpi_os_wait_events_complete(); /* synchronize Notify handling */
+ rearm_wake_irq(acpi_sci_irq);
}
static void acpi_s2idle_restore(void)
@@ -1044,7 +1048,6 @@ static const struct platform_s2idle_ops
.begin = acpi_s2idle_begin,
.prepare = acpi_s2idle_prepare,
.wake = acpi_s2idle_wake,
- .sync = acpi_s2idle_sync,
.restore = acpi_s2idle_restore,
.end = acpi_s2idle_end,
};
Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -191,7 +191,6 @@ struct platform_s2idle_ops {
int (*begin)(void);
int (*prepare)(void);
void (*wake)(void);
- void (*sync)(void);
void (*restore)(void);
void (*end)(void);
};
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1291,11 +1291,6 @@ static int __device_suspend_noirq(struct
if (async_error)
goto Complete;
- if (pm_wakeup_pending()) {
- async_error = -EBUSY;
- goto Complete;
- }
-
if (dev->power.syscore || dev->power.direct_complete)
goto Complete;
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -119,48 +119,41 @@ static void s2idle_enter(void)
static void s2idle_loop(void)
{
- pm_pr_dbg("suspend-to-idle\n");
-
- for (;;) {
- int error;
+ int error;
- dpm_noirq_begin();
+ dpm_noirq_begin();
+ error = dpm_noirq_suspend_devices(PMSG_SUSPEND);
+ if (error)
+ goto resume;
- /*
- * Suspend-to-idle equals
- * frozen processes + suspended devices + idle processors.
- * Thus s2idle_enter() should be called right after
- * all devices have been suspended.
- *
- * Wakeups during the noirq suspend of devices may be spurious,
- * so prevent them from terminating the loop right away.
- */
- error = dpm_noirq_suspend_devices(PMSG_SUSPEND);
- if (!error)
- s2idle_enter();
- else if (error == -EBUSY && pm_wakeup_pending())
- error = 0;
+ pm_pr_dbg("suspend-to-idle\n");
- if (!error && s2idle_ops && s2idle_ops->wake)
+ /*
+ * Suspend-to-idle equals:
+ * frozen processes + suspended devices + idle processors.
+ * Thus s2idle_enter() should be called right after all devices have
+ * been suspended.
+ *
+ * Wakeups during the noirq suspend of devices may be spurious, so try
+ * to avoid them upfront.
+ */
+ for (;;) {
+ if (s2idle_ops && s2idle_ops->wake)
s2idle_ops->wake();
- dpm_noirq_resume_devices(PMSG_RESUME);
-
- dpm_noirq_end();
-
- if (error)
- break;
-
- if (s2idle_ops && s2idle_ops->sync)
- s2idle_ops->sync();
-
if (pm_wakeup_pending())
break;
pm_wakeup_clear(false);
+
+ s2idle_enter();
}
pm_pr_dbg("resume from suspend-to-idle\n");
+
+resume:
+ dpm_noirq_resume_devices(PMSG_RESUME);
+ dpm_noirq_end();
}
void s2idle_wake(void)
^ permalink raw reply
* [PATCH 4/8] PM: sleep: Fix possible overflow in pm_system_cancel_wakeup()
From: Rafael J. Wysocki @ 2019-07-16 16:14 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
It is not actually guaranteed that pm_abort_suspend will be
nonzero when pm_system_cancel_wakeup() is called which may lead to
subtle issues, so make it use atomic_dec_if_positive() instead of
atomic_dec() for the safety sake.
Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from suspend-to-idle")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/base/power/wakeup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -859,7 +859,7 @@ EXPORT_SYMBOL_GPL(pm_system_wakeup);
void pm_system_cancel_wakeup(void)
{
- atomic_dec(&pm_abort_suspend);
+ atomic_dec_if_positive(&pm_abort_suspend);
}
void pm_wakeup_clear(bool reset)
^ permalink raw reply
* [PATCH 1/8] PCI: irq: Introduce rearm_wake_irq()
From: Rafael J. Wysocki @ 2019-07-16 16:10 UTC (permalink / raw)
To: Linux PM
Cc: Linux ACPI, LKML, Len Brown, Srinivas Pandruvada, Thomas Gleixner,
Rajneesh Bhardwaj, David E. Box
In-Reply-To: <71085220.z6FKkvYQPX@kreacher>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Introduce a new function, rearm_wake_irq(), allowing a wakeup IRQ
to be armed for systen wakeup detection again without running any
action handlers associated with it after it has been armed for
wakeup detection and triggered.
That is useful for IRQs, like ACPI SCI, that may deliver wakeup
as well as non-wakeup interrupts when armed for systen wakeup
detection. In those cases, it may be possible to determine whether
or not the delivered interrupt is a systen wakeup one without
running the entire action handler (or handlers, if the IRQ is
shared) for the IRQ, and if the interrupt turns out to be a
non-wakeup one, the IRQ can be rearmed with the help of the
new function.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
include/linux/interrupt.h | 1 +
kernel/irq/pm.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
Index: linux-pm/kernel/irq/pm.c
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -177,6 +177,26 @@ static void resume_irqs(bool want_early)
}
/**
+ * rearm_wake_irq - rearm a wakeup interrupt line after signaling wakeup
+ * @irq: Interrupt to rearm
+ */
+void rearm_wake_irq(unsigned int irq)
+{
+ unsigned long flags;
+ struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+
+ if (!desc || !(desc->istate & IRQS_SUSPENDED) ||
+ !irqd_is_wakeup_set(&desc->irq_data))
+ return;
+
+ desc->istate &= ~IRQS_SUSPENDED;
+ irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
+ __enable_irq(desc);
+
+ irq_put_desc_busunlock(desc, flags);
+}
+
+/**
* irq_pm_syscore_ops - enable interrupt lines early
*
* Enable all interrupt lines with %IRQF_EARLY_RESUME set.
Index: linux-pm/include/linux/interrupt.h
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -238,6 +238,7 @@ extern void teardown_percpu_nmi(unsigned
/* The following three functions are for the core kernel use only. */
extern void suspend_device_irqs(void);
extern void resume_device_irqs(void);
+extern void rearm_wake_irq(unsigned int irq);
/**
* struct irq_affinity_notify - context for notification of IRQ affinity changes
^ 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