2007-09-27 21:36:18

by Roel Kluin

[permalink] [raw]
Subject: [PATCH] spin_lock_unlocked cleanups

Replace some SPIN_LOCK_UNLOCKED with DEFINE_SPINLOCK

Signed-off-by: Roel Kluin <[email protected]>
---
diff --git a/arch/mips/pci/ops-pmcmsp.c b/arch/mips/pci/ops-pmcmsp.c
index 09fa007..059eade 100644
--- a/arch/mips/pci/ops-pmcmsp.c
+++ b/arch/mips/pci/ops-pmcmsp.c
@@ -206,7 +206,7 @@ static void pci_proc_init(void)
}
#endif /* CONFIG_PROC_FS && PCI_COUNTERS */

-spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
+DEFINE_SPINLOCK(bpci_lock);

/*****************************************************************************
*
diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index d5fd390..cd2766e 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c
@@ -34,7 +34,7 @@
#include <asm/mmu.h>
#include <asm/spu.h>

-static spinlock_t slice_convert_lock = SPIN_LOCK_UNLOCKED;
+static DEFINE_SPINLOCK(slice_convert_lock);


#ifdef DEBUG
diff --git a/drivers/char/watchdog/bfin_wdt.c b/drivers/char/watchdog/bfin_wdt.c
index 309d279..31dc7a6 100644
--- a/drivers/char/watchdog/bfin_wdt.c
+++ b/drivers/char/watchdog/bfin_wdt.c
@@ -71,7 +71,7 @@ static int nowayout = WATCHDOG_NOWAYOUT;
static struct watchdog_info bfin_wdt_info;
static unsigned long open_check;
static char expect_close;
-static spinlock_t bfin_wdt_spinlock = SPIN_LOCK_UNLOCKED;
+static DEFINE_SPINLOCK(bfin_wdt_spinlock);

/**
* bfin_wdt_keepalive - Keep the Userspace Watchdog Alive
diff --git a/drivers/ieee1394/ieee1394_core.c b/drivers/ieee1394/ieee1394_core.c
index 98fd985..36c747b 100644
--- a/drivers/ieee1394/ieee1394_core.c
+++ b/drivers/ieee1394/ieee1394_core.c
@@ -488,7 +488,7 @@ void hpsb_selfid_complete(struct hpsb_host *host, int phyid, int isroot)
highlevel_host_reset(host);
}

-static spinlock_t pending_packets_lock = SPIN_LOCK_UNLOCKED;
+static DEFINE_SPINLOCK(pending_packets_lock);

/**
* hpsb_packet_sent - notify core of sending a packet
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 83e76b3..94fd78f 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -15,9 +15,9 @@
#include "sysfs.h"

DEFINE_MUTEX(sysfs_mutex);
-spinlock_t sysfs_assoc_lock = SPIN_LOCK_UNLOCKED;
+DEFINE_SPINLOCK(sysfs_assoc_lock);

-static spinlock_t sysfs_ino_lock = SPIN_LOCK_UNLOCKED;
+static DEFINE_SPINLOCK(sysfs_ino_lock);
static DEFINE_IDA(sysfs_ino_ida);

/**


2007-09-28 08:17:51

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Thu, 2007-09-27 at 23:36 +0200, roel wrote:
> Replace some SPIN_LOCK_UNLOCKED with DEFINE_SPINLOCK
>
> Signed-off-by: Roel Kluin <[email protected]>

Acked-by: Thomas Gleixner <[email protected]>

Andy, Randy,

can we please add this to checkpatch.pl ?

> -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> +DEFINE_SPINLOCK(bpci_lock);

This code was introduced in June 2007, almost two years after the first
big DEFINE_SPINLOCK cleanup. Sigh.

Thanks,

tglx




2007-09-28 08:28:23

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:

> can we please add this to checkpatch.pl ?
>
> > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > +DEFINE_SPINLOCK(bpci_lock);

That check is already in checkpatch. Problem is that hardly anyone
runs the thing.

I think we're ready to wire checkpatch up to a email robot which monitors
the mailing lists and sends people nastygrams. I bet that'll be popular ;)

(I'd love it if it could detect wordwrapped and tab-expanded patches, too.
You wouldn't _believe_...)

2007-09-28 08:30:59

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups


* Andrew Morton <[email protected]> wrote:

> On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:
>
> > can we please add this to checkpatch.pl ?
> >
> > > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > > +DEFINE_SPINLOCK(bpci_lock);
>
> That check is already in checkpatch. Problem is that hardly anyone
> runs the thing.

i automatically run it for every patch i submit or push out via git.

> I think we're ready to wire checkpatch up to a email robot which
> monitors the mailing lists and sends people nastygrams. I bet that'll
> be popular ;)

heh ;-) It could be automated for patches that are sent out with a
Signed-off-by [or a Reviewed-by] line. If you send a SoB patch that is
broken, prepare to get a nastygram. (Initially i'd suggest the nastygram
to Cc: to a different email list, not lkml.)

Ingo

2007-09-28 08:32:49

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, 2007-09-28 at 01:26 -0700, Andrew Morton wrote:
> On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:
>
> > can we please add this to checkpatch.pl ?
> >
> > > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > > +DEFINE_SPINLOCK(bpci_lock);
>
> That check is already in checkpatch. Problem is that hardly anyone
> runs the thing.

Sigh, I forgot that perl is write only. :)

> I think we're ready to wire checkpatch up to a email robot which monitors
> the mailing lists and sends people nastygrams. I bet that'll be popular ;)

We should wire it up to git-commit as well. A lot of that comes in via
git subsystems.

> (I'd love it if it could detect wordwrapped and tab-expanded patches, too.
> You wouldn't _believe_...)

I know ...

tglx


2007-09-28 08:54:14

by Andy Whitcroft

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, Sep 28, 2007 at 01:26:56AM -0700, Andrew Morton wrote:
> On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:
>
> > can we please add this to checkpatch.pl ?
> >
> > > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > > +DEFINE_SPINLOCK(bpci_lock);
>
> That check is already in checkpatch. Problem is that hardly anyone
> runs the thing.
>
> I think we're ready to wire checkpatch up to a email robot which monitors
> the mailing lists and sends people nastygrams. I bet that'll be popular ;)

That shouldn't be too hard. checkpatch has been subscribed since birth
but short circuiting the replies to me only.

I guess the main question is whether to reply-all or reply just to the
sender when commenting on patches. Perhaps for the sanity of the rest
of the world, just the sender makes most sense.

> (I'd love it if it could detect wordwrapped and tab-expanded patches, too.
> You wouldn't _believe_...)

It should pick up both of these, the word-wrapping is already there as
we detect lines within patch segments which don't start '[ +-]', the
tab-expanded should be picked up as every line would be "don't use
spaces use tabs for indent".

-apw

2007-09-28 08:56:50

by Andy Whitcroft

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, Sep 28, 2007 at 10:32:38AM +0200, Thomas Gleixner wrote:
> On Fri, 2007-09-28 at 01:26 -0700, Andrew Morton wrote:
> > On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:
> >
> > > can we please add this to checkpatch.pl ?
> > >
> > > > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > > > +DEFINE_SPINLOCK(bpci_lock);
> >
> > That check is already in checkpatch. Problem is that hardly anyone
> > runs the thing.
>
> Sigh, I forgot that perl is write only. :)
>
> > I think we're ready to wire checkpatch up to a email robot which monitors
> > the mailing lists and sends people nastygrams. I bet that'll be popular ;)
>
> We should wire it up to git-commit as well. A lot of that comes in via
> git subsystems.

The problem with git-commit is who's repo to add the hook to. I did
attempt to do this by picking up each of linus' main releases and then
using the git blame engine to attribute each "failure" to a particular
commit. The plan then would be to send a nasty-gram to the committer
about violations there-in.

I'll try and find some time to get this bit polished and at least
emailing me.

-apw

2007-09-28 08:57:33

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, 28 Sep 2007 10:30:37 +0200 Ingo Molnar <[email protected]> wrote:

>
> * Andrew Morton <[email protected]> wrote:
>
> > On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:
> >
> > > can we please add this to checkpatch.pl ?
> > >
> > > > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > > > +DEFINE_SPINLOCK(bpci_lock);
> >
> > That check is already in checkpatch. Problem is that hardly anyone
> > runs the thing.
>
> i automatically run it for every patch i submit or push out via git.

you're hardly anyone ;)

> > I think we're ready to wire checkpatch up to a email robot which
> > monitors the mailing lists and sends people nastygrams. I bet that'll
> > be popular ;)
>
> heh ;-) It could be automated for patches that are sent out with a
> Signed-off-by [or a Reviewed-by] line. If you send a SoB patch that is
> broken, prepare to get a nastygram. (Initially i'd suggest the nastygram
> to Cc: to a different email list, not lkml.)

I was thinking it would reply to the sender only.

I have this vision of dragging my sorry butt to the keyboard in the morning
to be greeted by the usual shower of tab-replaced, space-stuffed
wordwrappery, except now each one is followed ten minutes later by a fixed up
version.

One can dream.

2007-09-28 09:06:19

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, 2007-09-28 at 09:56 +0100, Andy Whitcroft wrote:
> > > I think we're ready to wire checkpatch up to a email robot which monitors
> > > the mailing lists and sends people nastygrams. I bet that'll be popular ;)
> >
> > We should wire it up to git-commit as well. A lot of that comes in via
> > git subsystems.
>
> The problem with git-commit is who's repo to add the hook to. I did
> attempt to do this by picking up each of linus' main releases and then
> using the git blame engine to attribute each "failure" to a particular
> commit. The plan then would be to send a nasty-gram to the committer
> about violations there-in.
>
> I'll try and find some time to get this bit polished and at least
> emailing me.

The question is, whether we can convince the git developers to integrate
it. When a commit happens and checkpatch.pl is in scripts/, then run the
patch through it before doing the actual commit.

tglx



2007-09-28 09:07:44

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, 28 Sep 2007 09:53:47 +0100 Andy Whitcroft <[email protected]> wrote:

> On Fri, Sep 28, 2007 at 01:26:56AM -0700, Andrew Morton wrote:
> > On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:
> >
> > > can we please add this to checkpatch.pl ?
> > >
> > > > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > > > +DEFINE_SPINLOCK(bpci_lock);
> >
> > That check is already in checkpatch. Problem is that hardly anyone
> > runs the thing.
> >
> > I think we're ready to wire checkpatch up to a email robot which monitors
> > the mailing lists and sends people nastygrams. I bet that'll be popular ;)
>
> That shouldn't be too hard. checkpatch has been subscribed since birth
> but short circuiting the replies to me only.
>
> I guess the main question is whether to reply-all or reply just to the
> sender when commenting on patches. Perhaps for the sanity of the rest
> of the world, just the sender makes most sense.

For sure.

> > (I'd love it if it could detect wordwrapped and tab-expanded patches, too.
> > You wouldn't _believe_...)
>
> It should pick up both of these, the word-wrapping is already there as
> we detect lines within patch segments which don't start '[ +-]', the
> tab-expanded should be picked up as every line would be "don't use
> spaces use tabs for indent".

OK.

Often patches are wordwrapped only in the header:

--- old/drivers/ata/libata-sff.c 2007-04-26 12:02:46.000000000 -0400
+++ linux/drivers/ata/libata-sff.c 2007-04-29 08:29:27.000000000 -0400
@@ -413,6 +413,24 @@
ap->ops->irq_on(ap);

comes through as

--- old/drivers/ata/libata-sff.c 2007-04-26
12:02:46.000000000 -0400
+++ linux/drivers/ata/libata-sff.c 2007-04-29
08:29:27.000000000 -0400
@@ -413,6 +413,24 @@
ap->ops->irq_on(ap);

and the rest of the patch is good.

<tests it>

Yup, fooled you ;)

2007-09-28 17:32:26

by mark gross

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, Sep 28, 2007 at 01:26:56AM -0700, Andrew Morton wrote:
> On Fri, 28 Sep 2007 10:17:30 +0200 Thomas Gleixner <[email protected]> wrote:
>
> > can we please add this to checkpatch.pl ?
> >
> > > -spinlock_t bpci_lock = SPIN_LOCK_UNLOCKED;
> > > +DEFINE_SPINLOCK(bpci_lock);
>
> That check is already in checkpatch. Problem is that hardly anyone
> runs the thing.
>
> I think we're ready to wire checkpatch up to a email robot which monitors
> the mailing lists and sends people nastygrams. I bet that'll be popular ;)

One could make check patch create a signature hashing a check patch key
and the patch one could put in the post like a signed-off-by: thing.

checkpatch-sig : 2f818bcf0c2333a461affc4a170814f23adf2e08

this puts the burden on the sender to run the thing.

--mgross

2007-09-28 21:15:33

by Sean

[permalink] [raw]
Subject: Re: [PATCH] spin_lock_unlocked cleanups

On Fri, 28 Sep 2007 11:06:09 +0200
Thomas Gleixner <[email protected]> wrote:

> > The problem with git-commit is who's repo to add the hook to. I did
> > attempt to do this by picking up each of linus' main releases and then
> > using the git blame engine to attribute each "failure" to a particular
> > commit. The plan then would be to send a nasty-gram to the committer
> > about violations there-in.

Wouldn't it be easier to pass each commit through checkpatch and
email the committer if there is a problem? Each commit can be viewed
as a standalone patch afterall; what does blame add?

> The question is, whether we can convince the git developers to integrate
> it. When a commit happens and checkpatch.pl is in scripts/, then run the
> patch through it before doing the actual commit.

Definitely the way to go. I'm pretty sure the Git guys would agree to
distribute checkpatch.pl along with the existing pre-commit hook. So
at least enabling checkpatch would be trivial for those convinced to
use it.

Sean