Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

[Ben Hutchings <ben@decadent.org.uk>]

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

On Thu, 2010-06-24 at 14:05 +0200, Steffen Klassert wrote:
> Hi.
> 
> On Thu, Jun 24, 2010 at 12:55:41AM +0100, Ben Hutchings wrote:
> > This avoids scheduling in atomic context and also means that IRQs
> > will only be deferred for relatively short periods of time.
> > 
> > Previously discussed in:
> > http://article.gmane.org/gmane.linux.network/155024
> > 
> > Reported-by: Arne Nordmark <nordmark@mech.kth.se>
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32]
> > ---
> >  drivers/net/3c59x.c |   66 ++++++++++++++++++++++++++++++---------------------
> >  1 files changed, 39 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
> > index beddef9..f4a3fb1 100644
> > --- a/drivers/net/3c59x.c
> > +++ b/drivers/net/3c59x.c
> > @@ -644,9 +644,15 @@ struct vortex_private {
> >  	u16 deferred;						/* Resend these interrupts when we
> >  										 * bale from the ISR */
> >  	u16 io_size;						/* Size of PCI region (for release_region) */
> > -	spinlock_t lock;					/* Serialise access to device & its vortex_private */
> > -	struct mii_if_info mii;				/* MII lib hooks/info */
> > -	int window;					/* Register window */
> > +
> > +	/* Serialises access to hardware other than MII and variables below.
> > +	 * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */
> > +	spinlock_t lock;
> > +
> > +	spinlock_t mii_lock;		/* Serialises access to MII */
> > +	struct mii_if_info mii;		/* MII lib hooks/info */
> > +	spinlock_t window_lock;		/* Serialises access to windowed regs */
> 
> You should initialize the new locks properly with spin_lock_init().

Oops, yes, obviously.

> > +	int window;			/* Register window */
> >  };
> >  
> >  static void window_set(struct vortex_private *vp, int window)
> > @@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window)
> >  static u ## size							\
> >  window_read ## size(struct vortex_private *vp, int window, int addr)	\
> >  {									\
> > +	unsigned long flags;						\
> > +	u ## size ret;							\
> > +	spin_lock_irqsave(&vp->window_lock, flags);			\
> >  	window_set(vp, window);						\
> > -	return ioread ## size(vp->ioaddr + addr);			\
> > +	ret = ioread ## size(vp->ioaddr + addr);			\
> > +	spin_unlock_irqrestore(&vp->window_lock, flags);		\
> > +	return ret;							\
> >  }									\
> >  static void								\
> >  window_write ## size(struct vortex_private *vp, u ## size value,	\
> >  		     int window, int addr)				\
> >  {									\
> > +	unsigned long flags;						\
> > +	spin_lock_irqsave(&vp->window_lock, flags);			\
> >  	window_set(vp, window);						\
> >  	iowrite ## size(value, vp->ioaddr + addr);			\
> > +	spin_unlock_irqrestore(&vp->window_lock, flags);		\
> >  }
> 
> This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many
> places where this is not necessary at all. For example during device probe and
> device open, window_read/window_write are called multiple times, each time
> disabling the interrupts. I'd suggest to have unlocked, locked and irqsave
> versions of window_read/window_write and use them in appropriate places.

So what?  These are not speed-critical.  The fast-path functions do
acquire the lock just once.

> >  DEFINE_WINDOW_IO(8)
> >  DEFINE_WINDOW_IO(16)
> > @@ -1784,7 +1798,6 @@ vortex_timer(unsigned long data)
> >  		pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo);
> >  	}
> >  
> > -	disable_irq_lockdep(dev->irq);
> >  	media_status = window_read16(vp, 4, Wn4_Media);
> >  	switch (dev->if_port) {
> >  	case XCVR_10baseT:  case XCVR_100baseTx:  case XCVR_100baseFx:
> > @@ -1805,10 +1818,7 @@ vortex_timer(unsigned long data)
> >  	case XCVR_MII: case XCVR_NWAY:
> >  		{
> >  			ok = 1;
> > -			/* Interrupts are already disabled */
> > -			spin_lock(&vp->lock);
> >  			vortex_check_media(dev, 0);
> > -			spin_unlock(&vp->lock);
> >  		}
> >  		break;
> >  	  default:					/* Other media types handled by Tx timeouts. */
> > @@ -1827,6 +1837,8 @@ vortex_timer(unsigned long data)
> >  	if (!ok) {
> >  		unsigned int config;
> >  
> > +		spin_lock_irq(&vp->lock);
> 
> This can still happen every 5 seconds if the NIC has no link beat and
> medialock is not set. So what about defering this locked codepath to
> a workqueue, or moving the whole vortex_timer to a delayed workqueue?
> In this case we don't need to disable all the interrups on the cpu, we
> could still use disable_irq then.

This locked section is now very short - 5 or 6 register read/writes and
no delays.  We might even be able to get away without locking here as
the only software state this accesses is dev->if_port and I don't think
it can race with anything except SIOCGIFMAP (which seems harmless).

Ben.

> The rest looks quite good to me.
> 
> Thanks,
> 
> Steffen
> 

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]