[PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Guenter Roeck]

spin_is_locked() always returns false for uniprocessor configurations,
so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
purpose and must be used instead.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/firmware/efi/vars.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index f0a4364..6f3623c 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -481,7 +481,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove);
  */
 static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
 {
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	list_del(&entry->list);
 	spin_unlock_irq(&__efivars->lock);
@@ -507,7 +507,7 @@ int __efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -667,7 +667,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 	int strsize1, strsize2;
 	bool found = false;
 
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	list_for_each_entry_safe(entry, n, head, list) {
 		strsize1 = ucs2_strsize(name, 1024);
@@ -739,7 +739,7 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
-- 
1.9.1

[PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Guenter Roeck]

spin_is_locked() always returns false for uniprocessor configurations,
so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
purpose and must be used instead.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index f0a4364..6f3623c 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -481,7 +481,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove);
  */
 static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
 {
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	list_del(&entry->list);
 	spin_unlock_irq(&__efivars->lock);
@@ -507,7 +507,7 @@ int __efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -667,7 +667,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 	int strsize1, strsize2;
 	bool found = false;
 
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	list_for_each_entry_safe(entry, n, head, list) {
 		strsize1 = ucs2_strsize(name, 1024);
@@ -739,7 +739,7 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	WARN_ON(!spin_is_locked(&__efivars->lock));
+	WARN_ON_SMP(!spin_is_locked(&__efivars->lock));
 
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
-- 
1.9.1

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Matt Fleming]

On Sun, 10 Aug, at 08:54:13PM, Guenter Roeck wrote:
> spin_is_locked() always returns false for uniprocessor configurations,
> so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
> purpose and must be used instead.
 
Good catch, though I worry that WARN_ON_SMP() doesn't seem to be a very
common pattern,

arch/x86/pci/i386.c:	WARN_ON_SMP(!spin_is_locked(&pcibios_fwaddrmap_lock));
drivers/gpu/drm/drm_irq.c:	WARN_ON_SMP(!spin_is_locked(&dev->event_lock));
include/asm-generic/bug.h: * WARN_ON_SMP() is for cases that the warning is either
include/asm-generic/bug.h: *	WARN_ON_SMP(!zoot->bar);
include/asm-generic/bug.h: * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
include/asm-generic/bug.h: * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
include/asm-generic/bug.h:# define WARN_ON_SMP(x)			WARN_ON(x)
include/asm-generic/bug.h: * Use of ({0;}) because WARN_ON_SMP(x) may be used either as
include/asm-generic/bug.h:# define WARN_ON_SMP(x)			({0;})
kernel/futex.c:	if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr))

and people must want to do this kind of checking all the time.

How about lockdep_assert_held()? That seems to be much more popular.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Matt Fleming]

On Sun, 10 Aug, at 08:54:13PM, Guenter Roeck wrote:
> spin_is_locked() always returns false for uniprocessor configurations,
> so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
> purpose and must be used instead.
 
Good catch, though I worry that WARN_ON_SMP() doesn't seem to be a very
common pattern,

arch/x86/pci/i386.c:	WARN_ON_SMP(!spin_is_locked(&pcibios_fwaddrmap_lock));
drivers/gpu/drm/drm_irq.c:	WARN_ON_SMP(!spin_is_locked(&dev->event_lock));
include/asm-generic/bug.h: * WARN_ON_SMP() is for cases that the warning is either
include/asm-generic/bug.h: *	WARN_ON_SMP(!zoot->bar);
include/asm-generic/bug.h: * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
include/asm-generic/bug.h: * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
include/asm-generic/bug.h:# define WARN_ON_SMP(x)			WARN_ON(x)
include/asm-generic/bug.h: * Use of ({0;}) because WARN_ON_SMP(x) may be used either as
include/asm-generic/bug.h:# define WARN_ON_SMP(x)			({0;})
kernel/futex.c:	if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr))

and people must want to do this kind of checking all the time.

How about lockdep_assert_held()? That seems to be much more popular.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Guenter Roeck]

On 08/13/2014 07:18 AM, Matt Fleming wrote:
> On Sun, 10 Aug, at 08:54:13PM, Guenter Roeck wrote:
>> spin_is_locked() always returns false for uniprocessor configurations,
>> so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
>> purpose and must be used instead.
>
> Good catch, though I worry that WARN_ON_SMP() doesn't seem to be a very
> common pattern,
>
> arch/x86/pci/i386.c:	WARN_ON_SMP(!spin_is_locked(&pcibios_fwaddrmap_lock));
> drivers/gpu/drm/drm_irq.c:	WARN_ON_SMP(!spin_is_locked(&dev->event_lock));
> include/asm-generic/bug.h: * WARN_ON_SMP() is for cases that the warning is either
> include/asm-generic/bug.h: *	WARN_ON_SMP(!zoot->bar);
> include/asm-generic/bug.h: * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
> include/asm-generic/bug.h: * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
> include/asm-generic/bug.h:# define WARN_ON_SMP(x)			WARN_ON(x)
> include/asm-generic/bug.h: * Use of ({0;}) because WARN_ON_SMP(x) may be used either as
> include/asm-generic/bug.h:# define WARN_ON_SMP(x)			({0;})
> kernel/futex.c:	if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
>
> and people must want to do this kind of checking all the time.
>
> How about lockdep_assert_held()? That seems to be much more popular.
>

WARN_ON and WARN_ON_SMP are unconditional. lockdep_assert_held()
is only active if lockdep debugging is enabled. Not knowing the code,
nor the reason why the unconditional method was chosen, I prefer
to refrain from functional changes and limit myself to bug fixes.

Also, if and how the code can be optimized is another question,
which in my opinion should be separate from a bug fix.

Anyway, this specific bug doesn't affect me at this time;
I only noticed the problem since mainline currently crashes
on several non-SMP platforms due to the same problem. So
I don't mind if this patch is not or not immediately applied
until the discussion about how to best fix it is complete.

Guenter

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Guenter Roeck]

On 08/13/2014 07:18 AM, Matt Fleming wrote:
> On Sun, 10 Aug, at 08:54:13PM, Guenter Roeck wrote:
>> spin_is_locked() always returns false for uniprocessor configurations,
>> so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
>> purpose and must be used instead.
>
> Good catch, though I worry that WARN_ON_SMP() doesn't seem to be a very
> common pattern,
>
> arch/x86/pci/i386.c:	WARN_ON_SMP(!spin_is_locked(&pcibios_fwaddrmap_lock));
> drivers/gpu/drm/drm_irq.c:	WARN_ON_SMP(!spin_is_locked(&dev->event_lock));
> include/asm-generic/bug.h: * WARN_ON_SMP() is for cases that the warning is either
> include/asm-generic/bug.h: *	WARN_ON_SMP(!zoot->bar);
> include/asm-generic/bug.h: * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
> include/asm-generic/bug.h: * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
> include/asm-generic/bug.h:# define WARN_ON_SMP(x)			WARN_ON(x)
> include/asm-generic/bug.h: * Use of ({0;}) because WARN_ON_SMP(x) may be used either as
> include/asm-generic/bug.h:# define WARN_ON_SMP(x)			({0;})
> kernel/futex.c:	if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
>
> and people must want to do this kind of checking all the time.
>
> How about lockdep_assert_held()? That seems to be much more popular.
>

WARN_ON and WARN_ON_SMP are unconditional. lockdep_assert_held()
is only active if lockdep debugging is enabled. Not knowing the code,
nor the reason why the unconditional method was chosen, I prefer
to refrain from functional changes and limit myself to bug fixes.

Also, if and how the code can be optimized is another question,
which in my opinion should be separate from a bug fix.

Anyway, this specific bug doesn't affect me at this time;
I only noticed the problem since mainline currently crashes
on several non-SMP platforms due to the same problem. So
I don't mind if this patch is not or not immediately applied
until the discussion about how to best fix it is complete.

Guenter

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Matt Fleming]

On Wed, 13 Aug, at 07:37:37AM, Guenter Roeck wrote:
> 
> WARN_ON and WARN_ON_SMP are unconditional. lockdep_assert_held()
> is only active if lockdep debugging is enabled. Not knowing the code,
> nor the reason why the unconditional method was chosen, I prefer
> to refrain from functional changes and limit myself to bug fixes.
 
As the author of that code, I feel confident telling you that the
unconditional method was used because the author is a boob. The code
isn't so important that we need to unconditionally check the locks, and
indeed it's possible to run into all sorts of problems when you don't
use the standard lock-checking functions - the non-SMP crash being a
good example.

> Also, if and how the code can be optimized is another question,
> which in my opinion should be separate from a bug fix.
> 
> Anyway, this specific bug doesn't affect me at this time;
> I only noticed the problem since mainline currently crashes
> on several non-SMP platforms due to the same problem. So
> I don't mind if this patch is not or not immediately applied
> until the discussion about how to best fix it is complete.

Would you mind sending a patch swapping WARN_ON(!spin_is_locked()) for
lockdep_assert_held() ? That's still a bug fix (and your non-SMP
breakage rationale still makes sense), but I think it's a more
preferable fix.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Matt Fleming]

On Wed, 13 Aug, at 07:37:37AM, Guenter Roeck wrote:
> 
> WARN_ON and WARN_ON_SMP are unconditional. lockdep_assert_held()
> is only active if lockdep debugging is enabled. Not knowing the code,
> nor the reason why the unconditional method was chosen, I prefer
> to refrain from functional changes and limit myself to bug fixes.
 
As the author of that code, I feel confident telling you that the
unconditional method was used because the author is a boob. The code
isn't so important that we need to unconditionally check the locks, and
indeed it's possible to run into all sorts of problems when you don't
use the standard lock-checking functions - the non-SMP crash being a
good example.

> Also, if and how the code can be optimized is another question,
> which in my opinion should be separate from a bug fix.
> 
> Anyway, this specific bug doesn't affect me at this time;
> I only noticed the problem since mainline currently crashes
> on several non-SMP platforms due to the same problem. So
> I don't mind if this patch is not or not immediately applied
> until the discussion about how to best fix it is complete.

Would you mind sending a patch swapping WARN_ON(!spin_is_locked()) for
lockdep_assert_held() ? That's still a bug fix (and your non-SMP
breakage rationale still makes sense), but I think it's a more
preferable fix.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Theodore Ts'o]

On Wed, Aug 13, 2014 at 04:02:01PM +0100, Matt Fleming wrote:
> On Wed, 13 Aug, at 07:37:37AM, Guenter Roeck wrote:
> > 
> > WARN_ON and WARN_ON_SMP are unconditional. lockdep_assert_held()
> > is only active if lockdep debugging is enabled. Not knowing the code,
> > nor the reason why the unconditional method was chosen, I prefer
> > to refrain from functional changes and limit myself to bug fixes.
>  
> As the author of that code, I feel confident telling you that the
> unconditional method was used because the author is a boob. The code
> isn't so important that we need to unconditionally check the locks, and
> indeed it's possible to run into all sorts of problems when you don't
> use the standard lock-checking functions - the non-SMP crash being a
> good example.

If you want to actually force a BUG_ON if the spinlock is not taken,
even for non-debug kernels, you can use assert_spin_locked().  This
translates to a BUG_ON(!raw_spin_is_locked(x)) on SMP kernels, and a
no-op on UP kernels.

If you're confident in your testing that any problems would be
discovered before you push your patches to linus (and you actually use
lockdep in your testing :-), then lockdep_assert_held() doesn't add
any overhead if !lockdep, and so it might be a better choice for you.

							- Ted

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Theodore Ts'o]

On Wed, Aug 13, 2014 at 04:02:01PM +0100, Matt Fleming wrote:
> On Wed, 13 Aug, at 07:37:37AM, Guenter Roeck wrote:
> > 
> > WARN_ON and WARN_ON_SMP are unconditional. lockdep_assert_held()
> > is only active if lockdep debugging is enabled. Not knowing the code,
> > nor the reason why the unconditional method was chosen, I prefer
> > to refrain from functional changes and limit myself to bug fixes.
>  
> As the author of that code, I feel confident telling you that the
> unconditional method was used because the author is a boob. The code
> isn't so important that we need to unconditionally check the locks, and
> indeed it's possible to run into all sorts of problems when you don't
> use the standard lock-checking functions - the non-SMP crash being a
> good example.

If you want to actually force a BUG_ON if the spinlock is not taken,
even for non-debug kernels, you can use assert_spin_locked().  This
translates to a BUG_ON(!raw_spin_is_locked(x)) on SMP kernels, and a
no-op on UP kernels.

If you're confident in your testing that any problems would be
discovered before you push your patches to linus (and you actually use
lockdep in your testing :-), then lockdep_assert_held() doesn't add
any overhead if !lockdep, and so it might be a better choice for you.

							- Ted

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Wed, Aug 13, 2014 at 03:18:36PM +0100, Matt Fleming wrote:
> On Sun, 10 Aug, at 08:54:13PM, Guenter Roeck wrote:
> > spin_is_locked() always returns false for uniprocessor configurations,
> > so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
> > purpose and must be used instead.
>  
> 
> How about lockdep_assert_held()? That seems to be much more popular.

lockdep_assert_held() not only checks if the lock is taken, it also
verifies its taken by the current context, so its a stronger assertion.

And, as a bonus, it doesn't emit _any_ code on a !DEBUG build ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] firmware: Do not use WARN_ON(!spin_is_locked())

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Wed, Aug 13, 2014 at 03:18:36PM +0100, Matt Fleming wrote:
> On Sun, 10 Aug, at 08:54:13PM, Guenter Roeck wrote:
> > spin_is_locked() always returns false for uniprocessor configurations,
> > so do not use WARN_ON with it. WARN_ON_SMP() exists for that very
> > purpose and must be used instead.
>  
> 
> How about lockdep_assert_held()? That seems to be much more popular.

lockdep_assert_held() not only checks if the lock is taken, it also
verifies its taken by the current context, so its a stronger assertion.

And, as a bonus, it doesn't emit _any_ code on a !DEBUG build ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]