shared config registers and locking

shared config registers and locking

[Benjamin Herrenschmidt]

Hi !

On my work in porting emac over to arch/powerpc (and make it work on SMP
platforms since there's at least one coming, possibly too), I ended up
with a problem with things like the workarounds for the EMAC loss of RX
clock (CONFIG_IBM_EMAC_PHY_RX_CLK_FIX) that I think uncovers a more
generic problem about access global system wide configuration registers
in a race free way.

On UP configuration, there is no real problem: local_irq_disable/enable
around the code tweaking those bits (read/modify/write) is enough and
everybody is happy. That's what the current emac code does.

However, on SMP, we need spinlocking (or a semaphore, but for simple
register accesses like that, spinlocks are probably better).

So there are several possible approaches here...

One is to stay fine grained and have every bit that need locking have
it's own accessors and locking mecanisms (ugh). For example, 40x and 44x
could provide some "subsystem" specific to some of the config DCRs here
exposing APIs to toggle some of the clock bits with proper locking in
there.

It might be the cleanest way but I also find that a bit painful ...

One other approach I had in mind is to simplify the problem to: There is
a certain amount of global configuration register on any machine,
typically embedded gear, and thus we provide a single spinlock (the
global config lock ?) that can be used by all powerpc archs & drivers to
protect each other when flipping bits in such registers.

That is simpler, but that still requires that we are a bit careful that

 - We only use that for very short lived things (typically
read/modify/write cycles to set/clear bits in a register)

 - We make sure that all drivers that access a given register do it with
that lock held, that is we have some agreement on a given platform/CPU
family to what registers are to be covered by that lock

And thus, as a consequence of the above, if a given subsystem needs a
bit more efficient/smart or long lived locking, it should not use that
facility but instead provide a specific subsystem of code with external
interfaces.

Any comments ?

The result would be something like:

global_config_lock(flags);

global_config_unlock(flags);

(It has the semantics of spin_lock_irqsave/restore)

Cheers,
Ben.

Re: shared config registers and locking

[Benjamin Herrenschmidt]

> On my work in porting emac over to arch/powerpc (and make it work on SMP
> platforms since there's at least one coming, possibly too), I ended up
> with a problem with things like the workarounds for the EMAC loss of RX
> clock (CONFIG_IBM_EMAC_PHY_RX_CLK_FIX) that I think uncovers a more
> generic problem about access global system wide configuration registers
> in a race free way.

BTW, Eugene, in the specific case of this RX clock problem causing the
transmitter to stall...

I've had this problem with sungem in the past and a few other drivers
have to deal with it as well. The general approach seems to be a bit
different, and if you think it can work for EMAC, I'll implement it that
way in the driver I'm working on.

The idea is rather to try to find magic bits to force the chip clocks on
when there is no link, simply ack the fact that when there is no link,
nothing gets transmitted and thus ignore tx timeouts. Doing
netif_carrier_off() (which is the right thing to do when the link is
down anyway) should cause the net stack to ignore them.

Now, some chips (like GEM) have a problem recovering when the link
finally comes back up and thus can have the transmitter stuck. That's
why drivers like this will simply reset the TX side when the link is
back up.

That should be simpler and avoid the need for the workaround, don't you
think ?

Cheers,
Ben.

Re: shared config registers and locking

[Eugene Surovegin]

On Wed, Dec 06, 2006 at 04:16:29PM +1100, Benjamin Herrenschmidt wrote:
> > On my work in porting emac over to arch/powerpc (and make it work on SMP
> > platforms since there's at least one coming, possibly too), I ended up
> > with a problem with things like the workarounds for the EMAC loss of RX
> > clock (CONFIG_IBM_EMAC_PHY_RX_CLK_FIX) that I think uncovers a more
> > generic problem about access global system wide configuration registers
> > in a race free way.
> 
> BTW, Eugene, in the specific case of this RX clock problem causing the
> transmitter to stall...
> 
> I've had this problem with sungem in the past and a few other drivers
> have to deal with it as well. The general approach seems to be a bit
> different, and if you think it can work for EMAC, I'll implement it that
> way in the driver I'm working on.
> 
> The idea is rather to try to find magic bits to force the chip clocks on
> when there is no link, simply ack the fact that when there is no link,
> nothing gets transmitted and thus ignore tx timeouts. Doing
> netif_carrier_off() (which is the right thing to do when the link is
> down anyway) should cause the net stack to ignore them.
> 
> Now, some chips (like GEM) have a problem recovering when the link
> finally comes back up and thus can have the transmitter stuck. That's
> why drivers like this will simply reset the TX side when the link is
> back up.
> 
> That should be simpler and avoid the need for the workaround, don't you
> think ?

No, it's not simpler. This just does not handle the problem this 
workaround was implemented for.

You don't understand what this workaround does. It has nothing to do 
with TX path at all. It deals with RX one and the fact that in that 
particular MAC design whole RX domain is clocked from this clock, 
meaning some parts of EMAC just simply stop working. For some modes 
(e.g. SMII) this problem will not arise as this clock is always 
present, there are other cases when it's not needed (like sane PHY).

For example, how do you handle PHY detection if you cannot talk to the 
PHY?

Ben, my suggestion, before you start "forking" and "cleaning up" this 
code, understand what it does.

-- 
Eugene

Re: shared config registers and locking

[Benjamin Herrenschmidt]

> No, it's not simpler. This just does not handle the problem this 
> workaround was implemented for.

Quite possibly I didn't fully understand it as there is little
explanation in the code about what is going on except that the RX clock
from the PHY is dropping and that is causing TX timeouts...

> You don't understand what this workaround does. It has nothing to do 
> with TX path at all. It deals with RX one and the fact that in that 
> particular MAC design whole RX domain is clocked from this clock, 
> meaning some parts of EMAC just simply stop working.

Ok. Up to what extent ? Enough so that MDIO stops working ?

> For some modes (e.g. SMII) this problem will not arise as this clock is always 
> present, there are other cases when it's not needed (like sane PHY).
>
> For example, how do you handle PHY detection if you cannot talk to the 
> PHY?

Ok, so MDIO is going down... Do you have some examples of which PHYs are
causing this ? (for my own curiosity/education)

> Ben, my suggestion, before you start "forking" and "cleaning up" this 
> code, understand what it does.

That is gratuituously mean.

I'm forking it to make everybody's (including yours) life easy as I had
to do major changes to the driver to handle SMP safety, OF platform
probing, and other things like that and I'm trying to avoid disrupting
the existing working driver while I am doing that.

This is more a "branch" than a "fork" per-se and is, I think, the
appropriate thing to do when changes of that magnitude are needed.

I'm not planning to do more "cleanups" than the obvious things that will
naturally result from the changes, like removing ifdef's whenever
possible as the driver becomes multiplatform capable, etc...

Your attack about not understanding what it does is low, since it
concern specifically a workaround for a problem that isn't clearly
explained in the code, and  I was specifically asking you about it
before changing things exactly because I wasn't sure about my
understanding of it !

Besides, I don't understand your bitterness about the situation as, as I
said, I'm trying hard to do things in a way that will be easier to deal
with for everybody, and I've tried to discuss my choices with you on IRC
pretty much every time, to be mostly received with harsh words if not
insults.

Now, regarding that clocking problem, I wonder if it is related to Apple
disabling the automatic low power mode on some PHYs on GEM ... I think
that mode causes that stopping of the RX clock when the link is down and
this is disrupting the chip. (Just wondering if it's a similar isssue).

Now back to this workaround, there is still the issue that you noticed
that on some chips, the only way to tweak the clocks is globally for all
EMACs (440GX). Does that mean that on such a setup, removing the link
after probe will be fatal to the driver ? I suppose switching to
internal clocks of all EMACs is not an option as it will disrupt
activity on all the other ones....

Ben.

Re: shared config registers and locking

[Roger Larsson]

On Wednesday 06 December 2006 06:10, Benjamin Herrenschmidt wrote:
>
> On UP configuration, there is no real problem: local_irq_disable/enable
> around the code tweaking those bits (read/modify/write) is enough and
> everybody is happy. That's what the current emac code does.
>
> However, on SMP, we need spinlocking (or a semaphore, but for simple
> register accesses like that, spinlocks are probably better).
>
It is actually simple.

You should use
spin_lock_irq_save()

include/linux/spinlock.h
#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
#define spin_lock_irqsave(lock, flags)  flags = _spin_lock_irqsave(lock)
- - -
#else
#define spin_lock_irqsave(lock, flags)  _spin_lock_irqsave(lock, flags)
- - -
#endif

spinlock_api_smp.h:
	unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
spinlock_api_up.h:
	/*
	 * In the UP-nondebug case there's no real locking going on, so the
	 * only thing we have to do is to keep the preempt counts and irq
	 * flags straight, to supress compiler warnings of unused lock
	 * variables, and to add the proper checker annotations:
	 */

	#define _spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
	#define __LOCK_IRQSAVE(lock, flags) \
		do { local_irq_save(flags); __LOCK(lock); } while (0)
	#define __LOCK(lock) \
		do { preempt_disable(); __acquire(lock); (void)(lock); } while (0)
		-> disable preemption if compiled with preemption
		-> static checker annotation
		-> make compiler happy, use variable lock

So by using spin_lock_irqsave always you won't get anything more than 
local_irq_save when compiling on an UP box with kernel preemption disabled.

You could of cause use spin_lock_irq(lock) if you do not have nested locks -
it is not the lock that is the problem but the unlock.

#define local_irq_disable()     __asm__ __volatile__("cli": : :"memory")
#define local_irq_enable()      __asm__ __volatile__("sti": : :"memory")

local_irq_disable()
call
	local_irq_disable()
	local_irq_enable()
do_something(); // Is IRQ enabled or disabled here? Was that intended?
local_irq_enable()


Documentation to read:
	linux2.6/Documentation/spinlocks.txt
	linux2.6/Documentation/io_ordering.txt
	linux2.6/Documentation/preempt-locking.txt

/RogerL

Re: shared config registers and locking

[Benjamin Herrenschmidt]

On Wed, 2006-12-06 at 22:57 +0100, Roger Larsson wrote:
> On Wednesday 06 December 2006 06:10, Benjamin Herrenschmidt wrote:
> >
> > On UP configuration, there is no real problem: local_irq_disable/enable
> > around the code tweaking those bits (read/modify/write) is enough and
> > everybody is happy. That's what the current emac code does.
> >
> > However, on SMP, we need spinlocking (or a semaphore, but for simple
> > register accesses like that, spinlocks are probably better).
> >
> It is actually simple.
> 
> You should use
> spin_lock_irq_save()

Thanks you very much for telling me about spinlocks ... I wonder how I
did all these years ... :-)

Now, more seriously, of course the thing will boil down to a spinlock.
My point was what do you guys think about having one shared spinlock
exposed by the powerpc platform code for use by all those little bits
here or there that whack global configuration registers like clock
control things, etc... that we have all over the place on embedded
processors.

Since I don't like exposing a naked data structure though, rather than
exposing the spinlock itself, I was proposing to expose an API of two
functions (just in case we want to do something fancy) though they could
well end up being inline and thus boil down to the same...

Ben.

Re: shared config registers and locking

[Michael Galassi]

>Now, more seriously, of course the thing will boil down to a spinlock.
>My point was what do you guys think about having one shared spinlock
>exposed by the powerpc platform code for use by all those little bits
>here or there that whack global configuration registers like clock
>control things, etc... that we have all over the place on embedded
>processors.

GACK!  Many better engineers than any of us have spent countless hours
breaking down locks.  A lock takes nearly no space at all, you're not
proposing to lock any less often, so multiple locks take no less time,
the only thing you stand to gain by protecting multiple data structures
with one lock is the possibility of lock contention.  Please reconsider.

I can envision situations in which you might want to have a lock
protecting more than one item in code which is local to your project,
company, group, whatever, feel free to do do so in that circumstance
but limit this practice to individual, carefully thought out, *LOCAL*
situations, don't allow that to propagate.  Please?  Pretty please?

>Since I don't like exposing a naked data structure though, rather than
>exposing the spinlock itself, I was proposing to expose an API of two
>functions (just in case we want to do something fancy) though they could
>well end up being inline and thus boil down to the same...

Linux needs another api just as much as we need another war in the
middle east or a third term of our commander in chief.  Just say no.

It turns out that any place you have any business manipulating a lock
associated with some data you're probably doing so in preparation to
manipulate that data.  By using a simple, well known interface such as
spin_lock_irqsave/spin_unlock_irqrestore you are actually making clear
what your code is doing, you are alerting the reader that you've
considered locking issues.  This is a good thing.

-michael

Re: shared config registers and locking

[Benjamin Herrenschmidt]

> GACK!  Many better engineers than any of us have spent countless hours
> breaking down locks.  A lock takes nearly no space at all, you're not
> proposing to lock any less often, so multiple locks take no less time,
> the only thing you stand to gain by protecting multiple data structures
> with one lock is the possibility of lock contention.  Please reconsider.

Wait wait wait.... I'm perfectly aware of lock contention issues and I'm
not proposing to have a giant lock for everything in the system.

I was proposing something very clearly aimed at configuration type
things that are generally accessed a bit at boot time and then every
time the phase of the moon changes, in which case, it's just a pain to
have to use fine grained locking and one shared lock is plenty enough.

The specific example that made me think about it was some clock control
registers on 4xx that the EMAC driver is whacking every now and then (on
link state change, no lock contention to be worried about here), though
they could be used by other drivers too (or platform code). Since those
registers are a bit different from one 4xx to the next too, and since I
now have a non-4xx platform that needs to use EMAC and has similar clock
control bits I might have to toggle there too, I'm basically ending up
with a global spinlock that I can't precisely define better than "global
clock control lock" ....

Now, what is best there ? To have 4xx expose such a lock and define
precisely what set of registers it covers and require any non-4xx
platform that uses the EMAC driver to provide a similar lock ?
 
Now, you have the right to disagree with me, I was mostly asking what
the mood was about it, I don't have any firm opinion on wether it's a
good or a bad idea just yet... so I count your vote as a "no" :-)

Another approach would be to remove the code from EMAC and have it
instead use the "feature call" thingy to call into platform code to do
the magic clock tweaking (with appropriate locking) but I don't like
much that idea as all 4xx platforms pretty much would have to duplicate
that code, and I prefer keeping some of that magic in the EMAC driver
itself. 

Now, regarding exposing a function or just a spinlock, well, you might
well be right that exposing a spinlock is better, I have no very firm
preference there neither... just a slight bias toward exporting
functions rather than variables.

Ben.

Re: shared config registers and locking

[Roger Larsson]

On Thursday 07 December 2006 00:34, you wrote:
> > GACK!  Many better engineers than any of us have spent countless hours
> > breaking down locks.  A lock takes nearly no space at all, you're not
> > proposing to lock any less often, so multiple locks take no less time,
> > the only thing you stand to gain by protecting multiple data structures
> > with one lock is the possibility of lock contention.  Please reconsider.
>
> Wait wait wait.... I'm perfectly aware of lock contention issues and I'm
> not proposing to have a giant lock for everything in the system.
>
> I was proposing something very clearly aimed at configuration type
> things that are generally accessed a bit at boot time and then every
> time the phase of the moon changes, in which case, it's just a pain to
> have to use fine grained locking and one shared lock is plenty enough.
>
> The specific example that made me think about it was some clock control
> registers on 4xx that the EMAC driver is whacking every now and then (on
> link state change, no lock contention to be worried about here), though
> they could be used by other drivers too (or platform code). Since those
> registers are a bit different from one 4xx to the next too, and since I
> now have a non-4xx platform that needs to use EMAC and has similar clock
> control bits I might have to toggle there too, I'm basically ending up
> with a global spinlock that I can't precisely define better than "global
> clock control lock" ....
>

Do all EMAC implementations have this problem?

- - -

> Another approach would be to remove the code from EMAC and have it
> instead use the "feature call" thingy to call into platform code to do
> the magic clock tweaking (with appropriate locking) but I don't like
> much that idea as all 4xx platforms pretty much would have to duplicate
> that code, and I prefer keeping some of that magic in the EMAC driver
> itself.

I usually preferre this approach. Since the magic from one point of view 
(EMAC) is normal configuration from another.

Good candidates from EMACs(!) point of view...
	EMAC_RX_CLK_TX(int idx)
	EMAC_RX_CLK_DEFAULT(int idx)
But form clock config it is probably closer to this.
	CLK_DEFAULT(int idx)
	...

Another approach might be to factor out the modification code only,
keeping the original locking code for comparation...
	static inline modify_dcr(reg, set_mask, reset_mask)
	{
		unsigned long flags;
		local_irq_save(flags);
		mtdcr(reg, mfdcr(reg) & ~reset_mask | set_mask ))
		local_irq_restore(flags);
	}
and
	static inline SDR_MODIFY(...)

#if defined(CONFIG_405EP)
#define EMAC_RX_CLK_TX(idx) modify_dcr(0xf3, 1<< (idx), 0)
#else
#define EMAC_RX_CLK_TX(idx) SDR_MODIFY(DCRN_SDR_MFR, (0x08000000 >> idx), 0)
#endif

/RogerL

Re: shared config registers and locking

[Benjamin Herrenschmidt]

> > The specific example that made me think about it was some clock control
> > registers on 4xx that the EMAC driver is whacking every now and then (on
> > link state change, no lock contention to be worried about here), though
> > they could be used by other drivers too (or platform code). Since those
> > registers are a bit different from one 4xx to the next too, and since I
> > now have a non-4xx platform that needs to use EMAC and has similar clock
> > control bits I might have to toggle there too, I'm basically ending up
> > with a global spinlock that I can't precisely define better than "global
> > clock control lock" ....
> >
> 
> Do all EMAC implementations have this problem?

No, only some of them. 

> Another approach might be to factor out the modification code only,
> keeping the original locking code for comparation...
> 	static inline modify_dcr(reg, set_mask, reset_mask)
> 	{
> 		unsigned long flags;
> 		local_irq_save(flags);
> 		mtdcr(reg, mfdcr(reg) & ~reset_mask | set_mask ))
> 		local_irq_restore(flags);
> 	}
> and
> 	static inline SDR_MODIFY(...)

Since we need a spinlock here (the whole point of the exercise is to
make it SMP and -rt safe), that means having a global spinlock used by
modify_dcr / SDR_MODIFY which boils down to something not far from my
proposal.

> #if defined(CONFIG_405EP)
> #define EMAC_RX_CLK_TX(idx) modify_dcr(0xf3, 1<< (idx), 0)
> #else
> #define EMAC_RX_CLK_TX(idx) SDR_MODIFY(DCRN_SDR_MFR, (0x08000000 >> idx), 0)
> #endif

The ifdef's are going away in my rework in favor of something like

	if (emac_has_feature(dev, EMAC_FTR_NEED_405EP_CLK_FIX))
		xxxx

So that we can build support for multiple models in one driver if we
want. If we don't, the above if will turn into if (0) / if (1) depending
on what we build for and thus the compiler will do the right thing and
optimize out unused code. That's the approach we use already for CPU and
firmware features and it works well.

Ben.

Re: shared config registers and locking

[Roger Larsson]

On Thursday 07 December 2006 03:47, Benjamin Herrenschmidt wrote:
> > > The specific example that made me think about it was some clock control
> > > registers on 4xx that the EMAC driver is whacking every now and then
> > > (on link state change, no lock contention to be worried about here),
> > > though they could be used by other drivers too (or platform code).
> > > Since those registers are a bit different from one 4xx to the next too,
> > > and since I now have a non-4xx platform that needs to use EMAC and has
> > > similar clock control bits I might have to toggle there too, I'm
> > > basically ending up with a global spinlock that I can't precisely
> > > define better than "global clock control lock" ....
> >
> > Do all EMAC implementations have this problem?
>
> No, only some of them.
>
> > Another approach might be to factor out the modification code only,
> > keeping the original locking code for comparation...
> > 	static inline modify_dcr(reg, set_mask, reset_mask)
> > 	{
> > 		unsigned long flags;
> > 		local_irq_save(flags);
> > 		mtdcr(reg, mfdcr(reg) & ~reset_mask | set_mask ))
> > 		local_irq_restore(flags);
> > 	}
> > and
> > 	static inline SDR_MODIFY(...)
>
> Since we need a spinlock here (the whole point of the exercise is to
> make it SMP and -rt safe), that means having a global spinlock used by
> modify_dcr / SDR_MODIFY which boils down to something not far from my
> proposal.

That is correct. BUT I do like my other approach better.
Add functions with names that work on for example clocks.
By doing that there will be no need to comment on what is done, only why.

Modify all users to use those functions (you will need to modify all users
for all approaches). ALL modification on clock config registers will be 
required to use the modify_dcr internally - then it might be a lot better to 
add a cleaner interface directly.

And that interface would take care of the differences in how the actual
modify clock between 405 and 440 (use of dcr or sdr).

>
> > #if defined(CONFIG_405EP)
> > #define EMAC_RX_CLK_TX(idx) modify_dcr(0xf3, 1<< (idx), 0)
> > #else
> > #define EMAC_RX_CLK_TX(idx) SDR_MODIFY(DCRN_SDR_MFR, (0x08000000 >> idx),
> > 0) #endif
>
> The ifdef's are going away in my rework in favor of something like
>
> 	if (emac_has_feature(dev, EMAC_FTR_NEED_405EP_CLK_FIX))
> 		xxxx
>
> So that we can build support for multiple models in one driver if we
> want. If we don't, the above if will turn into if (0) / if (1) depending
> on what we build for and thus the compiler will do the right thing and
> optimize out unused code. That's the approach we use already for CPU and
> firmware features and it works well.

Nice!

/RogerL