Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 7 Feb 2007 01:18:57 +0300
From:      Oleg Bulyzhin <oleg@FreeBSD.org>
To:        Bruce Evans <bde@zeta.org.au>
Cc:        Robin Gruyters <r.gruyters@yirdis.nl>, freebsd-net@FreeBSD.org
Subject:   Re: [Fwd: Re: bge Ierr rate increase from 5.3R -> 6.1R]
Message-ID:  <20070206221857.GA66675@lath.rinet.ru>
In-Reply-To: <20070207003539.I31484@besplex.bde.org>
References:  <20070125170532.c9c2374hkwk4oc4k@server.yirdis.net> <20070205232800.GA45487@lath.rinet.ru> <20070207003539.I31484@besplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Feb 07, 2007 at 01:31:39AM +1100, Bruce Evans wrote:
> On Tue, 6 Feb 2007, Oleg Bulyzhin wrote:
> 
> >Sorry for the late reply - i was AFK for some time and didnt read mails.
> >Patch against 6.2R attached, please let me know does it helps or not.
> 
> I didn't test the old version of this since I have too many local changes
> in my non-6.x versions.  I might test the 6.x version.
> 
> I use jdp's quicker fix.  It works fine for detecting cable unplug
> and replug, but link detection is still very bad at boot time and
> after down/up (seems to be worse for down/up than unplug/replug?).
> Link detection in -current generally seems to be much worse than
> in 5.2.  On some systems I use two ping -c2's early in the boot to
> wait for the link to actually be up.  The first ping tends to fail
> and the second tends to work, both long after the lonk claims to
> be up.  Then other network activity still takes too long to start.
> Without the pings, an "ntpdate -b" early in the boot fails about
> half the time and gives messed up timing activity when it fails,
> and initial nfs mounts tak 30-60 seconds.  Later after down/up and
> waiting for the "up" message, ttcp -u usually fails to connect the
> first time and then works normally with no failure or connection
> delay the second time.

Could you please give some more details? I'm intersted in:
- chip version
- are you using 'auto' media or fixed one?
- have you tried verbose boot? (MAC's link state (bge_link) reported with it).
- is there recipe how to trigger erroneous behaviour? i'll try it on mine
  bge cards. (i have bcm5721 5700 & 5701, but i didnt notice errors in
  link handling with -current driver (and i had problems with 5.x driver))

The fact 'ping' workaround does help looks like lost interrupt.

> 
> % Index: sys/dev/bge/if_bgereg.h
> % ===================================================================
> % RCS file: /home/ncvs/src/sys/dev/bge/if_bgereg.h,v
> % retrieving revision 1.36.2.8.2.1
> % diff -u -r1.36.2.8.2.1 if_bgereg.h
> % --- sys/dev/bge/if_bgereg.h	21 Dec 2006 21:53:54 -0000	1.36.2.8.2.1
> % +++ sys/dev/bge/if_bgereg.h	5 Feb 2007 23:23:22 -0000
> % @@ -2460,8 +2460,13 @@
> %  	uint32_t		bge_tx_buf_ratio;
> %  	int			bge_if_flags;
> %  	int			bge_txcnt;
> % -	int			bge_link;	/* link state */
> % -	int			bge_link_evt;	/* pending link event */
> % +	uint32_t		bge_sts;
> % +#define	BGE_STS_LINK		0x00000001	/* MAC link status */
> % +#define	BGE_STS_LINK_EVT	0x00000002	/* pending link 
> event */
> % +#define	BGE_STS_AUTOPOLL	0x00000004	/* PHY auto-polling  
> */
> % +#define BGE_STS_BIT(sc, x)	((sc)->bge_sts & (x))
> % +#define BGE_STS_SETBIT(sc, x)	((sc)->bge_sts |= (x))
> % +#define BGE_STS_CLRBIT(sc, x)	((sc)->bge_sts &= ~(x))
> 
> Style bugs:
> - inconsistents tabs.  bge mostly consitently doesn't follow KNF for the
>   tab after #define.

mea culpa.

> - I don't like obfuscating bit testing and setting using macros.  The
>   above macros are quite different from the BGE and PCI SETBIT and CLRBIT
>   macros -- the latter operate on hardware bits and do something useful
>   by hiding the details of the hardware accesses, and aren't assoicated
>   with bit testing macros.  Apart from the above, testing of hardware bits
>   and testing an setting of software bits is always done using direct
>   "&" and "|" operations in bge.

I'm going to agree with you. (I was hesitating about using macros here and
your arguments look reasonable.)

> 
> % Index: sys/dev/bge/if_bge.c
> % ===================================================================
> % RCS file: /home/ncvs/src/sys/dev/bge/if_bge.c,v
> % retrieving revision 1.91.2.18.2.1
> % diff -u -r1.91.2.18.2.1 if_bge.c
> % --- sys/dev/bge/if_bge.c	21 Dec 2006 21:53:54 -0000	1.91.2.18.2.1
> % +++ sys/dev/bge/if_bge.c	5 Feb 2007 23:23:29 -0000
> % @@ -2645,12 +2648,12 @@
> % 
> %  	/* Note link event. It will be processed by POLL_AND_CHECK_STATUS 
> cmd */
> %  	if (statusword & BGE_STATFLAG_LINKSTATE_CHANGED)
> % -		sc->bge_link_evt++;
> % +		BGE_STS_SETBIT(sc, BGE_STS_LINK_EVT);
> % 
> %  	if (cmd == POLL_AND_CHECK_STATUS)
> %  		if ((sc->bge_asicrev == BGE_ASICREV_BCM5700 &&
> %  		    sc->bge_chipid != BGE_CHIPID_BCM5700_B2) ||
> % -		    sc->bge_link_evt || sc->bge_tbi)
> % +		    BGE_STS_BIT(sc, BGE_STS_LINK_EVT) || sc->bge_tbi)
> %  			bge_link_upd(sc);
> 
> I suspected this is the cause of slowness for polling in idle (see previous
> mail), but since it doesn't do any PCI access directly it should be fast
> enough.
> The macros make it unclear that it doesn't do any PCI accesses
> directly.
Agree.
> 
> % @@ -2699,7 +2702,7 @@
> % 
> %  	if ((sc->bge_asicrev == BGE_ASICREV_BCM5700 &&
> %  	    sc->bge_chipid != BGE_CHIPID_BCM5700_B2) ||
> % -	    statusword || sc->bge_link_evt)
> % +	    statusword || BGE_STS_BIT(sc, BGE_STS_LINK_EVT))
> %  		bge_link_upd(sc);
> % 
> %  	if (ifp->if_drv_flags & IFF_DRV_RUNNING) {
> 
> The software link status handling causes a problem in the interrupt
> handler.  To avoid pessimizing the case of shared interrupts, the
> interrupt handler should be able to read the status word from the
> status block and return without doing anything if the interrupt is not
> for it.  This can be done without acquiring the driver lock (since the
> driver lock is neither necessary nor sufficient for accessing the
> status block).  Software link status gets in the way of this, since
> accessing it requires the driver lock.
You are right, but at the moment, bge_intr() is locked and does not care
about shared interrupts.  If we are going to fix this i would vote for 
using taskqueue api - in this case we can test 'link event' flag inside
locked taskqueue thread.

>(Note that `statusword' in the
> above is now bogusly named.  It used to be the status word from the
> status block but is now the mac status for a PCI register.  It is still
> from the status block in similar code in bge_poll().  Not pessimizing
> the case of shared interrupts requires using the status block again,
> and then the read from the PCI register might become a dummy.)
We can not avoid pci register read before syncing status block - we should
flush data posted at pci bridge, otherwise we can 'miss' interrupt.

> 
> Elsewhere, you added code to force interrupt after link status changes.
> I think this is to get the interrupt handler to look at the software
> link status in the above.  When I first saw it, I thought that forcing
> an interrupt is needed in more places.
Forced interrupt used for TBI cards since they do not have PHY auto-polling.

> 
> There are also complications to work around hardware bugs in reporting
> the link status.  Maybe the complications can be avoided by pushing
> them to the tick routine.  My version doesn't worry about them except
> in a commment and returns early without locking if the status block
> doesn't claim to have been updated.
> 
> Bruce

-- 
Oleg.

================================================================
=== Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- oleg@rinet.ru ===
================================================================




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070206221857.GA66675>