Date: Tue, 18 Jul 2000 22:15:45 +0200 (CEST) From: =?ISO-8859-1?Q?G=E9rard_Roudier?= <groudier@club-internet.fr> To: Andrew Gallatin <gallatin@cs.duke.edu> Cc: Bernd Walter <ticso@cicely8.cicely.de>, dg@root.com, mjacob@feral.com, Mike Smith <msmith@FreeBSD.ORG>, freebsd-alpha@FreeBSD.ORG Subject: Re: fxp0 hangs on a PC164 using STABLE Message-ID: <Pine.LNX.4.10.10007182135430.1638-100000@linux.local> In-Reply-To: <14707.48450.371071.494543@grasshopper.cs.duke.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 17 Jul 2000, Andrew Gallatin wrote:
=20
> Bernd Walter writes:
> > On Fri, Jul 14, 2000 at 05:23:21PM -0400, Andrew Gallatin wrote:
> > > As for the bug. I can duplicate it I run both the sender & receiver
> > > fairly hard, like 2 100Mb/tcp connections running in opposite
> > > directions. When I do this, it looks like the xmit side locks up b=
ut=20
> > > the recv. side is at least partially OK. And I'm still getting stat=
s
> > > updates.=20
> >=20
> > Looks like you are on the right way.
> > I can download a big file to the host without any problems but cvsup g=
ets
> > the interface stoped quite soon.
>=20
> The problem is that the status & command field share a 32-bit word.
> The programming API of the eepro apparently requires that you update
> the status field of a transmit slot that you've given to the card.
> Since alphas can only operate on 32-bit chunks of memory, both the
> status & command fields are loaded into memory & operated on in
> registers when the following line of C is executed:
>=20
> =09=09/*
> =09=09 * Advance the end of list forward.
> =09=09 */
> =09=09sc->cbl_last->cb_command &=3D ~FXP_CB_COMMAND_S;
I am not an Alpha expert, but my reading of the Alpha Architecture
Handbook rev. 4 let me think that not too old Alpha Systems should allow
atomic WORD and BYTE memory access from agents on the system BUS. CPUs
with AMASK bit 0 set support proper intructions for doing such kind of
access.
If the C compiler does not want to generate the corresponding code, then=20
assembly is to be used. The assembly will force compiler barrier, but if=20
C can be used, may-be a volatile cast should be used to force such a=20
barrier.
You may also want to add a memory barrier in places where some strong=20
ordering regarding how LOADs and STOREs are carried out to the system=20
BUS is expected.
> The race is caused by the card DMA'ing up the status at just the wrong
> time -- after it has been loaded into a register & before it has been
> written back. The old value of the status is written back, clobbering
> the status the card just DMA'ed up. The fact that the card has sent
> this frame is missed & the transmit engine appears to hang.
>=20
> I think I have a fix. It feels like overkill, but if the programming
> interface of the chip does not allow for something better why not just
> turn off DMA while we update cb_command?
>=20
> Can anybody having problems with fxp's please try the appended patch?
> Without this patch, I have a test case that can lock the xmitter in <
> 5 seconds 100% of the time. With the patch, I've been unable to get
> it to lock up using this test case.
>=20
> Can a PCI expert comment on its safety? Mike?
As a PCI non-expert, I may comment on its apparent brokenness. ;-)
Clearing the PCI BUS MASTER ENABLE bit is generally not a documented way
to tell a `running' PCI device to suspend DMA and wait for the bit to be
set again prior to continuing operations. This is likely to confuse most
PCI devices, in my opinion.
I hope I haven't been too wrong. :-)
G=E9rard.
> Index: pci/if_fxp.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> RCS file: /home/ncvs/src/sys/pci/if_fxp.c,v
> retrieving revision 1.77.2.4
> diff -u -r1.77.2.4 if_fxp.c
> --- pci/if_fxp.c=092000/07/13 07:53:06=091.77.2.4
> +++ pci/if_fxp.c=092000/07/18 02:07:04
> @@ -527,6 +527,8 @@
> =20
> =09callout_handle_init(&sc->stat_ch);
> =20
> +=09sc->dev =3D dev;
> +
> =09s =3D splimp();
> =20
> =09/*
> @@ -535,6 +537,7 @@
> =09val =3D pci_read_config(dev, PCIR_COMMAND, 2);
> =09val |=3D (PCIM_CMD_MEMEN|PCIM_CMD_BUSMASTEREN);
> =09pci_write_config(dev, PCIR_COMMAND, val, 2);
> +=09sc->cmd =3D val;
> =20
> =09/*
> =09 * Map control/status registers.
> @@ -960,6 +963,7 @@
> {
> =09struct fxp_softc *sc =3D ifp->if_softc;
> =09struct fxp_cb_tx *txp;
> +=09int s;
> =20
> =09/*
> =09 * See if we need to suspend xmit until the multicast filter
> @@ -1052,10 +1056,24 @@
> =09=09}
> =09=09txp->tx_threshold =3D tx_threshold;
> =09
> +#ifdef __alpha__
> +=09=09/*
> +=09=09 * Prevent the card from DMA'ing up the status while
> +=09=09 * we update the command field. This could cause
> +=09=09 * us to overwrite the completion status.
> +=09=09 */
> +=09=09s =3D splhigh();
> +=09=09pci_write_config(sc->dev, PCIR_COMMAND,=20
> +=09=09 (sc->cmd & ~PCIM_CMD_BUSMASTEREN), 2);
> +#endif /*alpha*/
> =09=09/*
> =09=09 * Advance the end of list forward.
> =09=09 */
> =09=09sc->cbl_last->cb_command &=3D ~FXP_CB_COMMAND_S;
> +#ifdef __alpha__
> +=09=09pci_write_config(sc->dev, PCIR_COMMAND, sc->cmd, 2);
> +=09=09splx(s);
> +#endif /*alpha*/
> =09=09sc->cbl_last =3D txp;
> =20
> =09=09/*
> Index: pci/if_fxpvar.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> RCS file: /home/ncvs/src/sys/pci/if_fxpvar.h,v
> retrieving revision 1.9.2.1
> diff -u -r1.9.2.1 if_fxpvar.h
> --- pci/if_fxpvar.h=092000/03/29 02:02:39=091.9.2.1
> +++ pci/if_fxpvar.h=092000/07/18 02:03:50
> @@ -48,6 +48,7 @@
> =09struct resource *mem;=09=09/* resource descriptor for registers */
> =09struct resource *irq;=09=09/* resource descriptor for interrupt */
> =09void *ih;=09=09=09/* interrupt handler cookie */
> +=09device_t dev;=09=09=09/* newbus device pointer */
> #endif /* __NetBSD__ */
> =09bus_space_tag_t sc_st;=09=09/* bus space tag */
> =09bus_space_handle_t sc_sh;=09/* bus space handle */
> @@ -68,6 +69,7 @@
> =09int phy_primary_device;=09=09/* device type of primary PHY */
> =09int phy_10Mbps_only;=09=09/* PHY is 10Mbps-only device */
> =09int eeprom_size;=09=09/* size of serial EEPROM */
> +=09u_long cmd;=09=09=09/* contents of pci CMD register */
> };
> =20
> /* Macros to ease CSR access. */
>=20
>=20
> Cheers,
>=20
> Drew
> -------------------------------------------------------------------------=
-----
> Andrew Gallatin, Sr Systems Programmer=09http://www.cs.duke.edu/~gallatin
> Duke University=09=09=09=09Email: gallatin@cs.duke.edu
> Department of Computer Science=09=09Phone: (919) 660-6590
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-alpha" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.LNX.4.10.10007182135430.1638-100000>