[KJ]remove SPIN_LOCK_UNLOCKED

[KJ]remove SPIN_LOCK_UNLOCKED

[Milind Arun Choudhary]

"use spin_lock_init instead of SPIN_LOCK_UNLOCKED"


i'm a bit confused with SPIN_LOCK_UNLOCKED removal
 I just did the below mentioned change
but then came to know that  spin_lock_init is defined as

# define spin_lock_init(lock)                                   \
        do { *(lock) = SPIN_LOCK_UNLOCKED; } while (0)

so what difference does it make..
eventually replacing SPIN_LOCK_UNLOCKED with itself

or is it just that this one [SPIN_LOCK_UNLOCKED] also needs 
to be replaced with __SPIN_LOCK_UNLOCKED?

after thought with CONFIG_DEBUG_SPINLOCK does make a diff


let me know if this is fine ..


 aerdrv.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index db6ad8e..6846fb4 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -157,7 +157,7 @@ static struct aer_rpc* aer_alloc_rpc(struct pcie_device *dev)
 	 * Initialize Root lock access, e_lock, to Root Error Status Reg,
 	 * Root Error ID Reg, and Root error producer/consumer index.
 	 */
-	rpc->e_lock = SPIN_LOCK_UNLOCKED;
+	spin_lock_init(&rpc->e_lock);
 
 	rpc->rpd = dev;
 	INIT_WORK(&rpc->dpc_handler, aer_isr);


-- 
Milind Arun Choudhary

Re: [KJ]remove SPIN_LOCK_UNLOCKED

[Jan Engelhardt]

On Apr 10 2007 23:46, Milind Arun Choudhary wrote:

>"use spin_lock_init instead of SPIN_LOCK_UNLOCKED"

Fact is, we cannot remove SPIN_LOCK_UNLOCKED. It's needed for
variables outside functions:

static spinlock_t foobar = SPIN_LOCK_UNLOCKED;




>let me know if this is fine ..

not for me to comment :)


> aerdrv.c |    2 +-
> 1 files changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
>index db6ad8e..6846fb4 100644
>--- a/drivers/pci/pcie/aer/aerdrv.c
>+++ b/drivers/pci/pcie/aer/aerdrv.c
>@@ -157,7 +157,7 @@ static struct aer_rpc* aer_alloc_rpc(struct pcie_device *dev)
> 	 * Initialize Root lock access, e_lock, to Root Error Status Reg,
> 	 * Root Error ID Reg, and Root error producer/consumer index.
> 	 */
>-	rpc->e_lock = SPIN_LOCK_UNLOCKED;
>+	spin_lock_init(&rpc->e_lock);
> 
> 	rpc->rpd = dev;
> 	INIT_WORK(&rpc->dpc_handler, aer_isr);
>
>
>-- 
>Milind Arun Choudhary
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>

Jan
-- 

Re: [KJ]remove SPIN_LOCK_UNLOCKED

[Roland Dreier]

 > Fact is, we cannot remove SPIN_LOCK_UNLOCKED. It's needed for
 > variables outside functions:
 > 
 > static spinlock_t foobar = SPIN_LOCK_UNLOCKED;

DEFINE_SPINLOCK() is provided to define variables that way.

 - R.

Re: [KJ]remove SPIN_LOCK_UNLOCKED

[Robert P. J. Day]

On Tue, 10 Apr 2007, Jan Engelhardt wrote:

>
> On Apr 10 2007 23:46, Milind Arun Choudhary wrote:
>
> >"use spin_lock_init instead of SPIN_LOCK_UNLOCKED"
>
> Fact is, we cannot remove SPIN_LOCK_UNLOCKED. It's needed for
> variables outside functions:
>
> static spinlock_t foobar = SPIN_LOCK_UNLOCKED;

but that's where you would use the more explicit
__RAW_SPIN_LOCK_UNLOCKED, no?  AFAIK, you really can remove the macro
SPIN_LOCK_UNLOCKED in its entirety.

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

Re: [KJ]remove SPIN_LOCK_UNLOCKED

[Jan Engelhardt]

On Apr 10 2007 17:25, Robert P. J. Day wrote:
>On Tue, 10 Apr 2007, Jan Engelhardt wrote:
>> On Apr 10 2007 23:46, Milind Arun Choudhary wrote:
>>
>> >"use spin_lock_init instead of SPIN_LOCK_UNLOCKED"
>>
>> Fact is, we cannot remove SPIN_LOCK_UNLOCKED. It's needed for
>> variables outside functions:
>>
>> static spinlock_t foobar = SPIN_LOCK_UNLOCKED;
>
>but that's where you would use the more explicit
>__RAW_SPIN_LOCK_UNLOCKED, no?  AFAIK, you really can remove the macro
>SPIN_LOCK_UNLOCKED in its entirety.

I don't remember LDD speaking about __RAW_*. (And other than not
having looked into the code to date, I don't know the difference.)


Jan
-- 

Re: [KJ]remove SPIN_LOCK_UNLOCKED

[Roland Dreier]

 > >but that's where you would use the more explicit
 > >__RAW_SPIN_LOCK_UNLOCKED, no?  AFAIK, you really can remove the macro
 > >SPIN_LOCK_UNLOCKED in its entirety.
 > 
 > I don't remember LDD speaking about __RAW_*. (And other than not
 > having looked into the code to date, I don't know the difference.)

Don't worry about the __RAW_SPIN_LOCK_UNLOCKED stuff, that's obviously
not for generic code to use.  The right answer (as I said before) is
to use DEFINE_SPINLOCK().

 - R.

Re: [KJ]remove SPIN_LOCK_UNLOCKED

[Robert P. J. Day]

On Tue, 10 Apr 2007, Roland Dreier wrote:

>  > >but that's where you would use the more explicit
>  > >__RAW_SPIN_LOCK_UNLOCKED, no?  AFAIK, you really can remove the macro
>  > >SPIN_LOCK_UNLOCKED in its entirety.
>  >
>  > I don't remember LDD speaking about __RAW_*. (And other than not
>  > having looked into the code to date, I don't know the difference.)
>
> Don't worry about the __RAW_SPIN_LOCK_UNLOCKED stuff, that's
> obviously not for generic code to use.  The right answer (as I said
> before) is to use DEFINE_SPINLOCK().

that works fine if you're defining a single spinlock, but what do you
do in cases like this:

arch/sparc/lib/atomic32.c:      [0 ... (ATOMIC_HASH_SIZE-1)] = SPIN_LOCK_UNLOCKED

that is, when you're assigning an array of them?  you still need some
kind of generic, unnamed spinlock in those circumstances, no?

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

Re: [KJ]remove SPIN_LOCK_UNLOCKED

[Roland Dreier]

 > > Don't worry about the __RAW_SPIN_LOCK_UNLOCKED stuff, that's
 > > obviously not for generic code to use.  The right answer (as I said
 > > before) is to use DEFINE_SPINLOCK().
 > 
 > that works fine if you're defining a single spinlock, but what do you
 > do in cases like this:
 > 
 > arch/sparc/lib/atomic32.c:      [0 ... (ATOMIC_HASH_SIZE-1)] = SPIN_LOCK_UNLOCKED
 > 
 > that is, when you're assigning an array of them?  you still need some
 > kind of generic, unnamed spinlock in those circumstances, no?

Wow, I didn't realize there was code doing that.  I guess for that
handful of cases, you indeed would probably want to convert them to
raw_spinlock_t and use __RAW_SPIN_LOCK_UNLOCKED.  But in the vast
majority of cases, DEFINE_SPINLOCK() is the right think to do.

 - R.

Re: [KJ] remove SPIN_LOCK_UNLOCKED

[Matthew Wilcox]

On Tue, Apr 10, 2007 at 05:45:07PM -0400, Robert P. J. Day wrote:
> that works fine if you're defining a single spinlock, but what do you
> do in cases like this:
> 
> arch/sparc/lib/atomic32.c:      [0 ... (ATOMIC_HASH_SIZE-1)] = SPIN_LOCK_UNLOCKED
> 
> that is, when you're assigning an array of them?  you still need some
> kind of generic, unnamed spinlock in those circumstances, no?

That's a special case for architecture-only code.  It's not to be used
by drivers.

Re: [KJ] remove SPIN_LOCK_UNLOCKED

[Milind Arun Choudhary]

On 4/11/07, Matthew Wilcox <matthew@wil.cx> wrote:
> On Tue, Apr 10, 2007 at 05:45:07PM -0400, Robert P. J. Day wrote:
> > that works fine if you're defining a single spinlock, but what do you
> > do in cases like this:
> >
> > arch/sparc/lib/atomic32.c:      [0 ... (ATOMIC_HASH_SIZE-1)] = SPIN_LOCK_UNLOCKED
> >
> > that is, when you're assigning an array of them?  you still need some
> > kind of generic, unnamed spinlock in those circumstances, no?
>
> That's a special case for architecture-only code.  It's not to be used
> by drivers.
as per my understanding, [which i should have keyed in earlier]
different places where SPIN_LOCK_UNLOCKED currently appears are

1. static spinlock_t foobar = SPIN_LOCK_UNLOCKED;
needs to be replaced bye DEFINE_SPINLOCK

e.g  linux-core/drm_memory_debug.h
-static spinlock_t drm_mem_lock = SPIN_LOCK_UNLOCKED;
+static DEFINE_SPINLOCK(drm_mem_lock);

there are very few occurrences left in the tree i see

2. allocating a data structure dynamically
& initializing the spinlock embedded within

use spin_lock_init()
e.g   linux-core/via_dmablit.c

-               blitq->blit_lock = SPIN_LOCK_UNLOCKED;
+               spin_lock_init(&blitq->blit_lock);

3. static initialization of structure members
struct foo bar ={
.
.
.
.lock = SPIN_LOCK_UNLOCKED,
.
}
use
struct foo bar ={
.
.
.
.lock = __SPIN_LOCK_UNLOCKED(bar.lock),
.
}

e.g arch/i386/kernel/traps.c
-		.lock =			SPIN_LOCK_UNLOCKED,
+		.lock =			__SPIN_LOCK_UNLOCKED(die.lock),

plenty of these are still there
may be some patches queued


4. arrays of spinlocks
e.g arch/cris/arch-v32/kernel/smp.c

-spinlock_t cris_atomic_locks[] = { [0 ... LOCK_COUNT - 1] =
SPIN_LOCK_UNLOCKED};
+raw_spinlock_t cris_atomic_locks[] = { [0 ... LOCK_COUNT - 1] =
__RAW_SPIN_LOCK_UNLOCKED};


my question is still there in the original post
about spin_lock_init()

CMIIW
Refer:
http://lkml.org/lkml/2005/6/20/47
http://lkml.org/lkml/2007/1/16/90
http://lists.openwall.net/linux-kernel/2007/02/01/258
-- 
Milind Arun Choudhary

Re: [KJ] remove SPIN_LOCK_UNLOCKED

[Robert P. J. Day]

On Tue, 10 Apr 2007, Matthew Wilcox wrote:

> On Tue, Apr 10, 2007 at 05:45:07PM -0400, Robert P. J. Day wrote:
> > that works fine if you're defining a single spinlock, but what do you
> > do in cases like this:
> >
> > arch/sparc/lib/atomic32.c:      [0 ... (ATOMIC_HASH_SIZE-1)] = SPIN_LOCK_UNLOCKED
> >
> > that is, when you're assigning an array of them?  you still need
> > some kind of generic, unnamed spinlock in those circumstances, no?
>
> That's a special case for architecture-only code.  It's not to be
> used by drivers.

be that as it may, it still means you need to take it into account
whenever someone says they want to entirely remove the
SPIN_LOCK_UNLOCKED macro from the source tree, as suggested in
Documentation/spinlocks.txt.

if you do that removal, you can always replace SPIN_LOCK_UNLOCKED with
its current definition of __SPIN_LOCK_UNLOCKED(old_style_spin_init),
or what have you.  but you would obviously have to replace it with
*something* that represents an unnamed spinlock if SPIN_LOCK_UNLOCKED
goes away.

rday

p.s.  just FYI:

$ grep -r "\.\.\..*SPIN_LOCK_UNLOCKED" *
arch/sparc/lib/atomic32.c:      [0 ... (ATOMIC_HASH_SIZE-1)] = SPIN_LOCK_UNLOCKED
arch/cris/arch-v32/kernel/smp.c:spinlock_t cris_atomic_locks[] = { [0 ... LOCK_COUNT - 1] = SPIN_LOCK_UNLOCKED};
arch/parisc/lib/bitops.c:       [0 ... (ATOMIC_HASH_SIZE-1)]  = __RAW_SPIN_LOCK_UNLOCKED
arch/mips/kernel/gdb-stub.c:    [0 ... NR_CPUS-1] = __RAW_SPIN_LOCK_UNLOCKED,
arch/powerpc/platforms/iseries/htab.c:  { [0 ... 63] = SPIN_LOCK_UNLOCKED};

so, as matthew says, it's clearly not for drivers.

-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================