Check if lockdep lock checking is disabled. If so, then do not define
device_links_read_lock_held(). It is used only from places where lockdep
checking is enabled.
Also fix a bug where I was not checking dep_map. Previously, I did not
test !SRCU configs so this got missed. Now it is sorted.
Link: https://lore.kernel.org/lkml/201908080026.WSAFx14k%[email protected]/
Fixes: c9e4d3a2fee8 ("acpi: Use built-in RCU list checking for acpi_ioremaps list")
(Based on RCU's dev branch)
Cc: [email protected]
Cc: kbuild test robot <[email protected]>,
Cc: Greg Kroah-Hartman <[email protected]>,
Cc: Josh Triplett <[email protected]>,
Cc: Lai Jiangshan <[email protected]>,
Cc: [email protected],
Cc: Mathieu Desnoyers <[email protected]>,
Cc: "Paul E. McKenney" <[email protected]>,
Cc: "Rafael J. Wysocki" <[email protected]>,
Cc: [email protected],
Cc: Steven Rostedt <[email protected]>,
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
drivers/base/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 32cf83d1c744..c22271577c84 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
up_read(&device_links_lock);
}
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
int device_links_read_lock_held(void)
{
- return lock_is_held(&device_links_lock);
+ return lock_is_held(&(device_links_lock.dep_map));
}
+#endif
#endif /* !CONFIG_SRCU */
/**
--
2.23.0.rc1.153.gdeed80330f-goog
On Mon, Aug 12, 2019 at 05:49:17PM -0400, Joel Fernandes (Google) wrote:
> Check if lockdep lock checking is disabled. If so, then do not define
> device_links_read_lock_held(). It is used only from places where lockdep
> checking is enabled.
>
> Also fix a bug where I was not checking dep_map. Previously, I did not
> test !SRCU configs so this got missed. Now it is sorted.
>
> Link: https://lore.kernel.org/lkml/201908080026.WSAFx14k%[email protected]/
> Fixes: c9e4d3a2fee8 ("acpi: Use built-in RCU list checking for acpi_ioremaps list")
> (Based on RCU's dev branch)
>
> Cc: [email protected]
> Cc: kbuild test robot <[email protected]>,
> Cc: Greg Kroah-Hartman <[email protected]>,
> Cc: Josh Triplett <[email protected]>,
> Cc: Lai Jiangshan <[email protected]>,
> Cc: [email protected],
> Cc: Mathieu Desnoyers <[email protected]>,
> Cc: "Paul E. McKenney" <[email protected]>,
> Cc: "Rafael J. Wysocki" <[email protected]>,
> Cc: [email protected],
> Cc: Steven Rostedt <[email protected]>,
>
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
Nit, drop those blank lines above, should all be in one big "block">
> ---
> drivers/base/core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 32cf83d1c744..c22271577c84 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
> up_read(&device_links_lock);
> }
>
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> int device_links_read_lock_held(void)
> {
> - return lock_is_held(&device_links_lock);
> + return lock_is_held(&(device_links_lock.dep_map));
> }
> +#endif
I don't know what the original code looks like here, but I'm guessing
that some .h file will need to be fixed up as you are just preventing
this function from ever being present without that option enabled?
thanks,
greg k-h
On Tue, Aug 13, 2019 at 08:05:40AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 12, 2019 at 05:49:17PM -0400, Joel Fernandes (Google) wrote:
> > Check if lockdep lock checking is disabled. If so, then do not define
> > device_links_read_lock_held(). It is used only from places where lockdep
> > checking is enabled.
> >
> > Also fix a bug where I was not checking dep_map. Previously, I did not
> > test !SRCU configs so this got missed. Now it is sorted.
> >
> > Link: https://lore.kernel.org/lkml/201908080026.WSAFx14k%[email protected]/
> > Fixes: c9e4d3a2fee8 ("acpi: Use built-in RCU list checking for acpi_ioremaps list")
> > (Based on RCU's dev branch)
> >
> > Cc: [email protected]
> > Cc: kbuild test robot <[email protected]>,
> > Cc: Greg Kroah-Hartman <[email protected]>,
> > Cc: Josh Triplett <[email protected]>,
> > Cc: Lai Jiangshan <[email protected]>,
> > Cc: [email protected],
> > Cc: Mathieu Desnoyers <[email protected]>,
> > Cc: "Paul E. McKenney" <[email protected]>,
> > Cc: "Rafael J. Wysocki" <[email protected]>,
> > Cc: [email protected],
> > Cc: Steven Rostedt <[email protected]>,
> >
> > Reported-by: kbuild test robot <[email protected]>
> > Signed-off-by: Joel Fernandes (Google) <[email protected]>
>
> Nit, drop those blank lines above, should all be in one big "block">
Ok.
> > drivers/base/core.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 32cf83d1c744..c22271577c84 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
> > up_read(&device_links_lock);
> > }
> >
> > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > int device_links_read_lock_held(void)
> > {
> > - return lock_is_held(&device_links_lock);
> > + return lock_is_held(&(device_links_lock.dep_map));
> > }
> > +#endif
>
> I don't know what the original code looks like here, but I'm guessing
> that some .h file will need to be fixed up as you are just preventing
> this function from ever being present without that option enabled?
No, it doesn't. I already thought about that and it is not an issue. I know
this may be confusing because the patch I am fixing is not yet in mainline
but in -rcu dev branch, however I did CC you on that RCU patch before but
understandably it is not in the series so it was harder to review.
Let me explain, the lock checking facility that this patch uses is here:
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=28875945ba98d1b47a8a706812b6494d165bb0a0
If you see, the CONFIG_PROVE_RCU_LIST defines an alternate __list_check_rcu()
which is just a NOOP if CONFIG_PROVE_RCU_LIST=n.
CONFIG_PROVE_RCU_LIST depends on CONFIG_PROVE_RCU which is def_bool on
CONFIG_PROVE_LOCKING which selects CONFIG_DEBUG_LOCK_ALLOC.
So there cannot be a scenario where CONFIG_PROVE_RCU_LIST is enabled but
CONFIG_DEBUG_LOCK_ALLOC is disabled.
To verify this, one could clone the RCU tree's dev branch and do this:
Initially PROVE_RCU_LIST is not in config:
joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
Neither is DEBUG_LOCK_ALLOC:
joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
# CONFIG_DEBUG_LOCK_ALLOC is not set
Enable all configs:
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_RCU_EXPERT
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_LOCKING
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU_LIST
joelaf@joelaf:~/repo/linux-master$ make olddefconfig
DEBUG_LOCK_ALLOC shows up:
joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock_all .config
CONFIG_DEBUG_LOCK_ALLOC=y
So does PROVE_RCU options:
joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
CONFIG_PROVE_RCU=y
CONFIG_PROVE_RCU_LIST=y
Now, force disable DEBUG_LOCK_ALLOC:
joelaf@joelaf:~/repo/linux-master$ ./scripts/config -d CONFIG_DEBUG_LOCK_ALLOC
joelaf@joelaf:~/repo/linux-master$ make olddefconfig
Options are still enabled:
joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
CONFIG_DEBUG_LOCK_ALLOC=y
joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
CONFIG_PROVE_RCU=y
CONFIG_PROVE_RCU_LIST=y
thanks,
- Joel
On Tue, Aug 13, 2019 at 09:39:05AM -0400, Joel Fernandes wrote:
[snip]
> > > drivers/base/core.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 32cf83d1c744..c22271577c84 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
> > > up_read(&device_links_lock);
> > > }
> > >
> > > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > int device_links_read_lock_held(void)
> > > {
> > > - return lock_is_held(&device_links_lock);
> > > + return lock_is_held(&(device_links_lock.dep_map));
> > > }
> > > +#endif
> >
> > I don't know what the original code looks like here, but I'm guessing
> > that some .h file will need to be fixed up as you are just preventing
> > this function from ever being present without that option enabled?
>
> No, it doesn't. I already thought about that and it is not an issue. I know
> this may be confusing because the patch I am fixing is not yet in mainline
> but in -rcu dev branch, however I did CC you on that RCU patch before but
> understandably it is not in the series so it was harder to review.
>
> Let me explain, the lock checking facility that this patch uses is here:
> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=28875945ba98d1b47a8a706812b6494d165bb0a0
>
> If you see, the CONFIG_PROVE_RCU_LIST defines an alternate __list_check_rcu()
> which is just a NOOP if CONFIG_PROVE_RCU_LIST=n.
>
> CONFIG_PROVE_RCU_LIST depends on CONFIG_PROVE_RCU which is def_bool on
> CONFIG_PROVE_LOCKING which selects CONFIG_DEBUG_LOCK_ALLOC.
>
> So there cannot be a scenario where CONFIG_PROVE_RCU_LIST is enabled but
> CONFIG_DEBUG_LOCK_ALLOC is disabled.
>
> To verify this, one could clone the RCU tree's dev branch and do this:
>
> Initially PROVE_RCU_LIST is not in config:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
>
> Neither is DEBUG_LOCK_ALLOC:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> # CONFIG_DEBUG_LOCK_ALLOC is not set
>
> Enable all configs:
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_RCU_EXPERT
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_LOCKING
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU_LIST
> joelaf@joelaf:~/repo/linux-master$ make olddefconfig
>
> DEBUG_LOCK_ALLOC shows up:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock_all .config
> CONFIG_DEBUG_LOCK_ALLOC=y
>
> So does PROVE_RCU options:
> joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> CONFIG_PROVE_RCU=y
> CONFIG_PROVE_RCU_LIST=y
>
> Now, force disable DEBUG_LOCK_ALLOC:
>
> joelaf@joelaf:~/repo/linux-master$ ./scripts/config -d CONFIG_DEBUG_LOCK_ALLOC
>
> joelaf@joelaf:~/repo/linux-master$ make olddefconfig
>
> Options are still enabled:
>
> joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> CONFIG_DEBUG_LOCK_ALLOC=y
> joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> CONFIG_PROVE_RCU=y
> CONFIG_PROVE_RCU_LIST=y
Also, appreciate if you could Ack the fix ;-) (assuming the newlines in
commit message are fixed).
thanks,
- Joel
On Tue, Aug 13, 2019 at 09:40:14AM -0400, Joel Fernandes wrote:
> On Tue, Aug 13, 2019 at 09:39:05AM -0400, Joel Fernandes wrote:
> [snip]
> > > > drivers/base/core.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 32cf83d1c744..c22271577c84 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
> > > > up_read(&device_links_lock);
> > > > }
> > > >
> > > > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > > int device_links_read_lock_held(void)
> > > > {
> > > > - return lock_is_held(&device_links_lock);
> > > > + return lock_is_held(&(device_links_lock.dep_map));
> > > > }
> > > > +#endif
> > >
> > > I don't know what the original code looks like here, but I'm guessing
> > > that some .h file will need to be fixed up as you are just preventing
> > > this function from ever being present without that option enabled?
> >
> > No, it doesn't. I already thought about that and it is not an issue. I know
> > this may be confusing because the patch I am fixing is not yet in mainline
> > but in -rcu dev branch, however I did CC you on that RCU patch before but
> > understandably it is not in the series so it was harder to review.
> >
> > Let me explain, the lock checking facility that this patch uses is here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=28875945ba98d1b47a8a706812b6494d165bb0a0
> >
> > If you see, the CONFIG_PROVE_RCU_LIST defines an alternate __list_check_rcu()
> > which is just a NOOP if CONFIG_PROVE_RCU_LIST=n.
> >
> > CONFIG_PROVE_RCU_LIST depends on CONFIG_PROVE_RCU which is def_bool on
> > CONFIG_PROVE_LOCKING which selects CONFIG_DEBUG_LOCK_ALLOC.
> >
> > So there cannot be a scenario where CONFIG_PROVE_RCU_LIST is enabled but
> > CONFIG_DEBUG_LOCK_ALLOC is disabled.
> >
> > To verify this, one could clone the RCU tree's dev branch and do this:
> >
> > Initially PROVE_RCU_LIST is not in config:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> >
> > Neither is DEBUG_LOCK_ALLOC:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> > # CONFIG_DEBUG_LOCK_ALLOC is not set
> >
> > Enable all configs:
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_RCU_EXPERT
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_LOCKING
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU_LIST
> > joelaf@joelaf:~/repo/linux-master$ make olddefconfig
> >
> > DEBUG_LOCK_ALLOC shows up:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock_all .config
> > CONFIG_DEBUG_LOCK_ALLOC=y
> >
> > So does PROVE_RCU options:
> > joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> > CONFIG_PROVE_RCU=y
> > CONFIG_PROVE_RCU_LIST=y
> >
> > Now, force disable DEBUG_LOCK_ALLOC:
> >
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -d CONFIG_DEBUG_LOCK_ALLOC
> >
> > joelaf@joelaf:~/repo/linux-master$ make olddefconfig
> >
> > Options are still enabled:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> > CONFIG_DEBUG_LOCK_ALLOC=y
> > joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> > CONFIG_PROVE_RCU=y
> > CONFIG_PROVE_RCU_LIST=y
>
> Also, appreciate if you could Ack the fix ;-) (assuming the newlines in
> commit message are fixed).
I don't know enough here to ack it, given that I can't even remember the
original patch...
If you think it's ok, that's fine with me, you can deal with the fallout
if it fails later :)
thanks,
greg k-h