Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 21 Jan 2009 20:48:43 -1000 (HST)
From:      Jeff Roberson <jroberson@jroberson.net>
To:        John Baldwin <jhb@FreeBSD.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r187576 - in head/sys/dev: ppbus ppc
Message-ID:  <20090121204615.H983@desktop>
In-Reply-To: <200901212310.n0LNA6cM093944@svn.freebsd.org>
References:  <200901212310.n0LNA6cM093944@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 21 Jan 2009, John Baldwin wrote:

> Author: jhb
> Date: Wed Jan 21 23:10:06 2009
> New Revision: 187576
> URL: http://svn.freebsd.org/changeset/base/187576
>
> Log:
>  Add locking to ppc and ppbus and mark the whole lot MPSAFE:

Looks like there might be some kinks still:

ppc0: <Parallel port> port 0x378-0x37f,0x778-0x77f irq 7 drq 3 on acpi0
ppc0: SMC-like chipset (ECP/EPP/PS2/NIBBLE) in COMPATIBLE mode
ppc0: FIFO with 16/16/9 bytes threshold
ppc0: [ITHREAD]
ppbus0: <Parallel port bus> on ppc0
panic: mutex ppc0 not owned at ../../../dev/ppc/ppc.c:1983
cpuid = 0
KDB: enter: panic
[thread pid 0 tid 100000 ]
Stopped at      kdb_enter+0x3d: movq    $0,0x652ea8(%rip)
_mtx_assert() at _mtx_assert+0xdc
ppc_write_ivar() at ppc_write_ivar+0x6e
ppbus_attach() at ppbus_attach+0x14b

Thanks,
Jeff

>  - To avoid having a bunch of locks that end up always getting acquired as
>    a group, give each ppc(4) device a mutex which it shares with all the
>    child devices including ppbus(4), lpt(4), plip(4), etc.  This mutex
>    is then used for all the locking.
>  - Rework the interrupt handling stuff yet again.  Now ppbus drivers setup
>    their interrupt handler during attach and tear it down during detach
>    like most other drivers.  ppbus(4) only invokes the interrupt handler
>    of the device that currently owns the bus (if any) when an interrupt
>    occurs, however.  Also, interrupt handlers in general now accept their
>    softc pointers as their argument rather than the device_t.  Another
>    feature of the ppbus interrupt handlers is that they are called with
>    the parent ppc device's lock already held.  This minimizes the number
>    of lock operations during an interrupt.
>  - Mark plip(4), lpt(4), pcfclock(4), ppi(4), vpo(4) MPSAFE.
>  - lpbb(4) uses the ppc lock instead of Giant.
>  - Other plip(4) changes:
>    - Add a mutex to protect the global tables in plip(4) and free them on
>      module unload.
>    - Add a detach routine.
>    - Split out the init/stop code from the ioctl routine into separate
>      functions.
>  - Other lpt(4) changes:
>    - Use device_printf().
>    - Use a dedicated callout for the lptout timer.
>    - Allocate the I/O buffers at attach and detach rather than during
>      open and close as this simplifies the locking at the cost of
>      1024+32 bytes when the driver is attached.
>  - Other ppi(4) changes:
>    - Use an sx lock to serialize open and close.
>    - Remove unused HADBUS flag.
>    - Add a detach routine.
>    - Use a malloc'd buffer for each read and write to avoid races with
>      concurrent read/write.
>  - Other pps(4) changes:
>    - Use a callout rather than a callout handle with timeout().
>    - Conform to the new ppbus requirements (regular mutex, non-filter
>      interrupt handler).  pps(4) is probably going to have to become a
>      standalone driver that doesn't use ppbus(4) to satisfy it's
>      requirements for low latency as a result.
>    - Use an sx lock to serialize open and close.
>  - Other vpo(4) changes:
>    - Use the parent ppc device's lock to create the CAM sim instead of
>      Giant.
>  - Other ppc(4) changes:
>    - Fix ppc_isa's detach method to detach instead of calling attach.
>
>  Tested by:	  no one :-(
>
> Modified:
>  head/sys/dev/ppbus/if_plip.c
>  head/sys/dev/ppbus/immio.c
>  head/sys/dev/ppbus/lpbb.c
>  head/sys/dev/ppbus/lpt.c
>  head/sys/dev/ppbus/pcfclock.c
>  head/sys/dev/ppbus/ppb_1284.c
>  head/sys/dev/ppbus/ppb_base.c
>  head/sys/dev/ppbus/ppb_msq.c
>  head/sys/dev/ppbus/ppbconf.c
>  head/sys/dev/ppbus/ppbconf.h
>  head/sys/dev/ppbus/ppi.c
>  head/sys/dev/ppbus/pps.c
>  head/sys/dev/ppbus/vpo.c
>  head/sys/dev/ppbus/vpoio.c
>  head/sys/dev/ppc/ppc.c
>  head/sys/dev/ppc/ppc_acpi.c
>  head/sys/dev/ppc/ppc_isa.c
>  head/sys/dev/ppc/ppc_pci.c
>  head/sys/dev/ppc/ppc_puc.c
>  head/sys/dev/ppc/ppcreg.h
>  head/sys/dev/ppc/ppcvar.h
>
> Modified: head/sys/dev/ppbus/if_plip.c
> ==============================================================================
> --- head/sys/dev/ppbus/if_plip.c	Wed Jan 21 21:48:46 2009	(r187575)
> +++ head/sys/dev/ppbus/if_plip.c	Wed Jan 21 23:10:06 2009	(r187576)
> @@ -152,8 +152,12 @@ struct lp_data {
> 	int		sc_iferrs;
>
> 	struct resource *res_irq;
> +	void		*sc_intr_cookie;
> };
>
> +static struct mtx lp_tables_lock;
> +MTX_SYSINIT(lp_tables, &lp_tables_lock, "plip tables", MTX_DEF);
> +
> /* Tables for the lp# interface */
> static u_char *txmith;
> #define	txmitl (txmith + (1 * LPIPTBLSIZE))
> @@ -170,13 +174,41 @@ static int lpinittables(void);
> static int lpioctl(struct ifnet *, u_long, caddr_t);
> static int lpoutput(struct ifnet *, struct mbuf *, struct sockaddr *,
> 	struct rtentry *);
> +static void lpstop(struct lp_data *);
> static void lp_intr(void *);
> +static int lp_module_handler(module_t, int, void *);
>
> #define	DEVTOSOFTC(dev) \
> 	((struct lp_data *)device_get_softc(dev))
>
> static devclass_t lp_devclass;
>
> +static int
> +lp_module_handler(module_t mod, int what, void *arg)
> +{
> +
> +	switch (what) {
> +	case MOD_UNLOAD:
> +		mtx_lock(&lp_tables_lock);
> +		if (txmith != NULL) {
> +			free(txmith, M_DEVBUF);
> +			txmith = NULL;
> +		}
> +		if (ctxmith != NULL) {
> +			free(ctxmith, M_DEVBUF);
> +			ctxmith = NULL;
> +		}
> +		mtx_unlock(&lp_tables_lock);
> +		break;
> +	case MOD_LOAD:
> +	case MOD_QUIESCE:
> +		break;
> +	default:
> +		return (EOPNOTSUPP);
> +	}
> +	return (0);
> +}
> +
> static void
> lp_identify(driver_t *driver, device_t parent)
> {
> @@ -201,7 +233,7 @@ lp_attach(device_t dev)
> {
> 	struct lp_data *lp = DEVTOSOFTC(dev);
> 	struct ifnet *ifp;
> -	int rid = 0;
> +	int error, rid = 0;
>
> 	lp->sc_dev = dev;
>
> @@ -224,8 +256,7 @@ lp_attach(device_t dev)
> 	ifp->if_softc = lp;
> 	if_initname(ifp, device_get_name(dev), device_get_unit(dev));
> 	ifp->if_mtu = LPMTU;
> -	ifp->if_flags = IFF_SIMPLEX | IFF_POINTOPOINT | IFF_MULTICAST |
> -	    IFF_NEEDSGIANT;
> +	ifp->if_flags = IFF_SIMPLEX | IFF_POINTOPOINT | IFF_MULTICAST;
> 	ifp->if_ioctl = lpioctl;
> 	ifp->if_output = lpoutput;
> 	ifp->if_hdrlen = 0;
> @@ -235,8 +266,39 @@ lp_attach(device_t dev)
>
> 	bpfattach(ifp, DLT_NULL, sizeof(u_int32_t));
>
> +	/*
> +	 * Attach our interrupt handler.  It is only called while we
> +	 * own the ppbus.
> +	 */
> +	error = bus_setup_intr(dev, lp->res_irq, INTR_TYPE_NET | INTR_MPSAFE,
> +	    NULL, lp_intr, lp, &lp->sc_intr_cookie);
> +	if (error) {
> +		bpfdetach(ifp);
> +		if_detach(ifp);
> +		bus_release_resource(dev, SYS_RES_IRQ, 0, lp->res_irq);
> +		device_printf(dev, "Unable to register interrupt handler\n");
> +		return (error);
> +	}
> +
> 	return (0);
> }
> +
> +static int
> +lp_detach(device_t dev)
> +{
> +	struct lp_data *sc = device_get_softc(dev);
> +	device_t ppbus = device_get_parent(dev);
> +
> +	ppb_lock(ppbus);
> +	lpstop(sc);
> +	ppb_unlock(ppbus);
> +	bpfdetach(sc->sc_ifp);
> +	if_detach(sc->sc_ifp);
> +	bus_teardown_intr(dev, sc->res_irq, sc->sc_intr_cookie);
> +	bus_release_resource(dev, SYS_RES_IRQ, 0, sc->res_irq);
> +	return (0);
> +}
> +
> /*
>  * Build the translation tables for the LPIP (BSD unix) protocol.
>  * We don't want to calculate these nasties in our tight loop, so we
> @@ -247,17 +309,22 @@ lpinittables(void)
> {
> 	int i;
>
> +	mtx_lock(&lp_tables_lock);
> 	if (txmith == NULL)
> 		txmith = malloc(4 * LPIPTBLSIZE, M_DEVBUF, M_NOWAIT);
>
> -	if (txmith == NULL)
> +	if (txmith == NULL) {
> +		mtx_unlock(&lp_tables_lock);
> 		return (1);
> +	}
>
> 	if (ctxmith == NULL)
> 		ctxmith = malloc(4 * LPIPTBLSIZE, M_DEVBUF, M_NOWAIT);
>
> -	if (ctxmith == NULL)
> +	if (ctxmith == NULL) {
> +		mtx_unlock(&lp_tables_lock);
> 		return (1);
> +	}
>
> 	for (i = 0; i < LPIPTBLSIZE; i++) {
> 		ctxmith[i] = (i & 0xF0) >> 4;
> @@ -272,10 +339,61 @@ lpinittables(void)
> 		trecvh[i] = ((~i) & 0x80) | ((i & 0x38) << 1);
> 		trecvl[i] = (((~i) & 0x80) >> 4) | ((i & 0x38) >> 3);
> 	}
> +	mtx_unlock(&lp_tables_lock);
>
> 	return (0);
> }
>
> +static void
> +lpstop(struct lp_data *sc)
> +{
> +	device_t ppbus = device_get_parent(sc->sc_dev);
> +
> +	ppb_assert_locked(ppbus);
> +	ppb_wctr(ppbus, 0x00);
> +	sc->sc_ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE);
> +	free(sc->sc_ifbuf, M_DEVBUF);
> +	sc->sc_ifbuf = NULL;
> +
> +	/* IFF_UP is not set, try to release the bus anyway */
> +	ppb_release_bus(ppbus, sc->sc_dev);
> +}
> +
> +static int
> +lpinit_locked(struct ifnet *ifp)
> +{
> +	struct lp_data *sc = ifp->if_softc;
> +	device_t dev = sc->sc_dev;
> +	device_t ppbus = device_get_parent(dev);
> +	int error;
> +
> +	ppb_assert_locked(ppbus);
> +	error = ppb_request_bus(ppbus, dev, PPB_DONTWAIT);
> +	if (error)
> +		return (error);
> +
> +	/* Now IFF_UP means that we own the bus */
> +	ppb_set_mode(ppbus, PPB_COMPATIBLE);
> +
> +	if (lpinittables()) {
> +		ppb_release_bus(ppbus, dev);
> +		return (ENOBUFS);
> +	}
> +
> +	sc->sc_ifbuf = malloc(sc->sc_ifp->if_mtu + MLPIPHDRLEN,
> +	    M_DEVBUF, M_NOWAIT);
> +	if (sc->sc_ifbuf == NULL) {
> +		ppb_release_bus(ppbus, dev);
> +		return (ENOBUFS);
> +	}
> +
> +	ppb_wctr(ppbus, IRQENABLE);
> +
> +	ifp->if_drv_flags |= IFF_DRV_RUNNING;
> +	ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
> +	return (0);
> +}
> +
> /*
>  * Process an ioctl request.
>  */
> @@ -288,7 +406,6 @@ lpioctl(struct ifnet *ifp, u_long cmd, c
> 	struct ifaddr *ifa = (struct ifaddr *)data;
> 	struct ifreq *ifr = (struct ifreq *)data;
> 	u_char *ptr;
> -	void *ih;
> 	int error;
>
> 	switch (cmd) {
> @@ -301,67 +418,32 @@ lpioctl(struct ifnet *ifp, u_long cmd, c
> 		ifp->if_flags |= IFF_UP;
> 		/* FALLTHROUGH */
> 	case SIOCSIFFLAGS:
> +		error = 0;
> +		ppb_lock(ppbus);
> 		if ((!(ifp->if_flags & IFF_UP)) &&
> -		    (ifp->if_drv_flags & IFF_DRV_RUNNING)) {
> -
> -			ppb_wctr(ppbus, 0x00);
> -			ifp->if_drv_flags &= ~IFF_DRV_RUNNING;
> -
> -			/* IFF_UP is not set, try to release the bus anyway */
> -			ppb_release_bus(ppbus, dev);
> -			break;
> -		}
> -		if (((ifp->if_flags & IFF_UP)) &&
> -		    (!(ifp->if_drv_flags & IFF_DRV_RUNNING))) {
> -
> -			/* XXX
> -			 * Should the request be interruptible?
> -			 */
> -			if ((error = ppb_request_bus(ppbus, dev, PPB_WAIT |
> -			    PPB_INTR)))
> -				return (error);
> +		    (ifp->if_drv_flags & IFF_DRV_RUNNING))
> +			lpstop(sc);
> +		else if (((ifp->if_flags & IFF_UP)) &&
> +		    (!(ifp->if_drv_flags & IFF_DRV_RUNNING)))
> +			error = lpinit_locked(ifp);
> +		ppb_unlock(ppbus);
> +		return (error);
>
> -			/* Now IFF_UP means that we own the bus */
> -			ppb_set_mode(ppbus, PPB_COMPATIBLE);
> -
> -			if (lpinittables()) {
> -				ppb_release_bus(ppbus, dev);
> -				return (ENOBUFS);
> -			}
> -
> -			sc->sc_ifbuf = malloc(sc->sc_ifp->if_mtu + MLPIPHDRLEN,
> -			    M_DEVBUF, M_WAITOK);
> -			if (sc->sc_ifbuf == NULL) {
> -				ppb_release_bus(ppbus, dev);
> +	case SIOCSIFMTU:
> +		ppb_lock(ppbus);
> +		if (ifp->if_drv_flags & IFF_DRV_RUNNING) {
> +			ptr = malloc(ifr->ifr_mtu + MLPIPHDRLEN, M_DEVBUF,
> +			    M_NOWAIT);
> +			if (ptr == NULL) {
> +				ppb_unlock(ppbus);
> 				return (ENOBUFS);
> 			}
> -
> -			/*
> -			 * Attach our interrupt handler.  It is
> -			 * detached later when the bus is released.
> -			 */
> -			if ((error = bus_setup_intr(dev, sc->res_irq,
> -			    INTR_TYPE_NET, NULL, lp_intr, dev, &ih))) {
> -				ppb_release_bus(ppbus, dev);
> -				return (error);
> -			}
> -
> -			ppb_wctr(ppbus, IRQENABLE);
> -			ifp->if_drv_flags |= IFF_DRV_RUNNING;
> -		}
> -		break;
> -
> -	case SIOCSIFMTU:
> -		ptr = sc->sc_ifbuf;
> -		sc->sc_ifbuf = malloc(ifr->ifr_mtu + MLPIPHDRLEN, M_DEVBUF,
> -		    M_NOWAIT);
> -		if (sc->sc_ifbuf == NULL) {
> +			if (sc->sc_ifbuf)
> +				free(sc->sc_ifbuf, M_DEVBUF);
> 			sc->sc_ifbuf = ptr;
> -			return (ENOBUFS);
> 		}
> -		if (ptr)
> -			free(ptr, M_DEVBUF);
> 		sc->sc_ifp->if_mtu = ifr->ifr_mtu;
> +		ppb_unlock(ppbus);
> 		break;
>
> 	case SIOCGIFMTU:
> @@ -417,14 +499,14 @@ clpinbyte(int spin, device_t ppbus)
> {
> 	u_char c, cl;
>
> -	while((ppb_rstr(ppbus) & CLPIP_SHAKE))
> +	while ((ppb_rstr(ppbus) & CLPIP_SHAKE))
> 		if (!--spin) {
> 			return (-1);
> 		}
> 	cl = ppb_rstr(ppbus);
> 	ppb_wdtr(ppbus, 0x10);
>
> -	while(!(ppb_rstr(ppbus) & CLPIP_SHAKE))
> +	while (!(ppb_rstr(ppbus) & CLPIP_SHAKE))
> 		if (!--spin) {
> 			return (-1);
> 		}
> @@ -445,16 +527,14 @@ lptap(struct ifnet *ifp, struct mbuf *m)
> static void
> lp_intr(void *arg)
> {
> -	device_t dev = (device_t)arg;
> -	device_t ppbus = device_get_parent(dev);
> -	struct lp_data *sc = DEVTOSOFTC(dev);
> -	int len, s, j;
> +	struct lp_data *sc = arg;
> +	device_t ppbus = device_get_parent(sc->sc_dev);
> +	int len, j;
> 	u_char *bp;
> 	u_char c, cl;
> 	struct mbuf *top;
>
> -	s = splhigh();
> -
> +	ppb_assert_locked(ppbus);
> 	if (sc->sc_ifp->if_flags & IFF_LINK0) {
>
> 		/* Ack. the request */
> @@ -500,13 +580,15 @@ lp_intr(void *arg)
> 		top = m_devget(sc->sc_ifbuf + CLPIPHDRLEN, len, 0, sc->sc_ifp,
> 		    0);
> 		if (top) {
> +			ppb_unlock(ppbus);
> 			if (bpf_peers_present(sc->sc_ifp->if_bpf))
> 				lptap(sc->sc_ifp, top);
>
> 			/* mbuf is free'd on failure. */
> 			netisr_queue(NETISR_IP, top);
> +			ppb_lock(ppbus);
> 		}
> -		goto done;
> +		return;
> 	}
> 	while ((ppb_rstr(ppbus) & LPIP_SHAKE)) {
> 		len = sc->sc_ifp->if_mtu + LPIPHDRLEN;
> @@ -517,7 +599,7 @@ lp_intr(void *arg)
> 			ppb_wdtr(ppbus, 8);
>
> 			j = LPMAXSPIN2;
> -			while((ppb_rstr(ppbus) & LPIP_SHAKE))
> +			while ((ppb_rstr(ppbus) & LPIP_SHAKE))
> 				if (!--j)
> 					goto err;
>
> @@ -550,14 +632,16 @@ lp_intr(void *arg)
> 		top = m_devget(sc->sc_ifbuf + LPIPHDRLEN, len, 0, sc->sc_ifp,
> 		    0);
> 		if (top) {
> +			ppb_unlock(ppbus);
> 			if (bpf_peers_present(sc->sc_ifp->if_bpf))
> 				lptap(sc->sc_ifp, top);
>
> 			/* mbuf is free'd on failure. */
> 			netisr_queue(NETISR_IP, top);
> +			ppb_lock(ppbus);
> 		}
> 	}
> -	goto done;
> +	return;
>
> err:
> 	ppb_wdtr(ppbus, 0);
> @@ -575,9 +659,6 @@ err:
> 		sc->sc_ifp->if_drv_flags &= ~IFF_DRV_RUNNING;
> 		sc->sc_iferrs = 0;
> 	}
> -
> -done:
> -	splx(s);
> }
>
> static __inline int
> @@ -602,7 +683,7 @@ lpoutput(struct ifnet *ifp, struct mbuf
> 	struct lp_data *sc = ifp->if_softc;
> 	device_t dev = sc->sc_dev;
> 	device_t ppbus = device_get_parent(dev);
> -	int s, err;
> +	int err;
> 	struct mbuf *mm;
> 	u_char *cp = "\0\0";
> 	u_char chksum = 0;
> @@ -611,19 +692,18 @@ lpoutput(struct ifnet *ifp, struct mbuf
>
> 	/* We need a sensible value if we abort */
> 	cp++;
> -	ifp->if_drv_flags |= IFF_DRV_RUNNING;
> +	ppb_lock(ppbus);
> +	ifp->if_drv_flags |= IFF_DRV_OACTIVE;
>
> 	err = 1;		/* assume we're aborting because of an error */
>
> -	s = splhigh();
> -
> 	/* Suspend (on laptops) or receive-errors might have taken us offline */
> 	ppb_wctr(ppbus, IRQENABLE);
>
> 	if (ifp->if_flags & IFF_LINK0) {
> 		if (!(ppb_rstr(ppbus) & CLPIP_SHAKE)) {
> 			lprintf("&");
> -			lp_intr(dev);
> +			lp_intr(sc);
> 		}
>
> 		/* Alert other end to pending packet */
> @@ -681,6 +761,7 @@ lpoutput(struct ifnet *ifp, struct mbuf
> 		err = 0;			/* No errors */
>
> 	nend:
> +		ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
> 		if (err)  {			/* if we didn't timeout... */
> 			ifp->if_oerrors++;
> 			lprintf("X");
> @@ -695,15 +776,15 @@ lpoutput(struct ifnet *ifp, struct mbuf
>
> 		if (!(ppb_rstr(ppbus) & CLPIP_SHAKE)) {
> 			lprintf("^");
> -			lp_intr(dev);
> +			lp_intr(sc);
> 		}
> -		(void) splx(s);
> +		ppb_unlock(ppbus);
> 		return (0);
> 	}
>
> 	if (ppb_rstr(ppbus) & LPIP_SHAKE) {
> 		lprintf("&");
> -		lp_intr(dev);
> +		lp_intr(sc);
> 	}
>
> 	if (lpoutbyte(0x08, LPMAXSPIN1, ppbus))
> @@ -726,6 +807,7 @@ end:
> 	--cp;
> 	ppb_wdtr(ppbus, txmitl[*cp] ^ 0x17);
>
> +	ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
> 	if (err)  {			/* if we didn't timeout... */
> 		ifp->if_oerrors++;
> 		lprintf("X");
> @@ -740,10 +822,10 @@ end:
>
> 	if (ppb_rstr(ppbus) & LPIP_SHAKE) {
> 		lprintf("^");
> -		lp_intr(dev);
> +		lp_intr(sc);
> 	}
>
> -	(void) splx(s);
> +	ppb_unlock(ppbus);
> 	return (0);
> }
>
> @@ -752,6 +834,7 @@ static device_method_t lp_methods[] = {
> 	DEVMETHOD(device_identify,	lp_identify),
> 	DEVMETHOD(device_probe,		lp_probe),
> 	DEVMETHOD(device_attach,	lp_attach),
> +	DEVMETHOD(device_detach,	lp_detach),
>
> 	{ 0, 0 }
> };
> @@ -762,5 +845,5 @@ static driver_t lp_driver = {
> 	sizeof(struct lp_data),
> };
>
> -DRIVER_MODULE(plip, ppbus, lp_driver, lp_devclass, 0, 0);
> +DRIVER_MODULE(plip, ppbus, lp_driver, lp_devclass, lp_module_handler, 0);
> MODULE_DEPEND(plip, ppbus, 1, 1, 1);
>
> Modified: head/sys/dev/ppbus/immio.c
> ==============================================================================
> --- head/sys/dev/ppbus/immio.c	Wed Jan 21 21:48:46 2009	(r187575)
> +++ head/sys/dev/ppbus/immio.c	Wed Jan 21 23:10:06 2009	(r187576)
> @@ -606,6 +606,7 @@ imm_attach(struct vpoio_data *vpo)
> 	/*
> 	 * Initialize mode dependent in/out microsequences
> 	 */
> +	ppb_lock(ppbus);
> 	if ((error = ppb_request_bus(ppbus, vpo->vpo_dev, PPB_WAIT)))
> 	    goto error;
>
> @@ -632,6 +633,7 @@ imm_attach(struct vpoio_data *vpo)
>
> 	ppb_release_bus(ppbus, vpo->vpo_dev);
>  error:
> +	ppb_unlock(ppbus);
> 	return (error);
> }
>
>
> Modified: head/sys/dev/ppbus/lpbb.c
> ==============================================================================
> --- head/sys/dev/ppbus/lpbb.c	Wed Jan 21 21:48:46 2009	(r187575)
> +++ head/sys/dev/ppbus/lpbb.c	Wed Jan 21 23:10:06 2009	(r187576)
> @@ -103,16 +103,16 @@ lpbb_callback(device_t dev, int index, c
> 	case IIC_REQUEST_BUS:
> 		/* request the ppbus */
> 		how = *(int *)data;
> -		mtx_lock(&Giant);
> +		ppb_lock(ppbus);
> 		error = ppb_request_bus(ppbus, dev, how);
> -		mtx_unlock(&Giant);
> +		ppb_unlock(ppbus);
> 		break;
>
> 	case IIC_RELEASE_BUS:
> 		/* release the ppbus */
> -		mtx_lock(&Giant);
> +		ppb_lock(ppbus);
> 		error = ppb_release_bus(ppbus, dev);
> -		mtx_unlock(&Giant);
> +		ppb_unlock(ppbus);
> 		break;
>
> 	default:
> @@ -129,25 +129,38 @@ lpbb_callback(device_t dev, int index, c
> #define ALIM    0x20
> #define I2CKEY  0x50
>
> +/* Reset bus by setting SDA first and then SCL. */
> +static void
> +lpbb_reset_bus(device_t dev)
> +{
> +	device_t ppbus = device_get_parent(dev);
> +
> +	ppb_assert_locked(ppbus);
> +	ppb_wdtr(ppbus, (u_char)~SDA_out);
> +	ppb_wctr(ppbus, (u_char)(ppb_rctr(ppbus) | SCL_out));
> +}
> +
> static int
> lpbb_getscl(device_t dev)
> {
> +	device_t ppbus = device_get_parent(dev);
> 	int rval;
>
> -	mtx_lock(&Giant);
> -	rval = ((ppb_rstr(device_get_parent(dev)) & SCL_in) == SCL_in);
> -	mtx_unlock(&Giant);
> +	ppb_lock(ppbus);
> +	rval = ((ppb_rstr(ppbus) & SCL_in) == SCL_in);
> +	ppb_unlock(ppbus);
> 	return (rval);
> }
>
> static int
> lpbb_getsda(device_t dev)
> {
> +	device_t ppbus = device_get_parent(dev);
> 	int rval;
>
> -	mtx_lock(&Giant);
> -	rval = ((ppb_rstr(device_get_parent(dev)) & SDA_in) == SDA_in);
> -	mtx_unlock(&Giant);
> +	ppb_lock(ppbus);
> +	rval = ((ppb_rstr(ppbus) & SDA_in) == SDA_in);
> +	ppb_unlock(ppbus);
> 	return (rval);
> }
>
> @@ -156,12 +169,12 @@ lpbb_setsda(device_t dev, char val)
> {
> 	device_t ppbus = device_get_parent(dev);
>
> -	mtx_lock(&Giant);
> +	ppb_lock(ppbus);
> 	if (val == 0)
> 		ppb_wdtr(ppbus, (u_char)SDA_out);
> 	else
> 		ppb_wdtr(ppbus, (u_char)~SDA_out);
> -	mtx_unlock(&Giant);
> +	ppb_unlock(ppbus);
> }
>
> static void
> @@ -169,12 +182,12 @@ lpbb_setscl(device_t dev, unsigned char
> {
> 	device_t ppbus = device_get_parent(dev);
>
> -	mtx_lock(&Giant);
> +	ppb_lock(ppbus);
> 	if (val == 0)
> 		ppb_wctr(ppbus, (u_char)(ppb_rctr(ppbus) & ~SCL_out));
> 	else
> 		ppb_wctr(ppbus, (u_char)(ppb_rctr(ppbus) | SCL_out));
> -	mtx_unlock(&Giant);
> +	ppb_unlock(ppbus);
> }
>
> static int
> @@ -182,23 +195,24 @@ lpbb_detect(device_t dev)
> {
> 	device_t ppbus = device_get_parent(dev);
>
> +	ppb_lock(ppbus);
> 	if (ppb_request_bus(ppbus, dev, PPB_DONTWAIT)) {
> +		ppb_unlock(ppbus);
> 		device_printf(dev, "can't allocate ppbus\n");
> 		return (0);
> 	}
>
> -	/* reset bus */
> -	lpbb_setsda(dev, 1);
> -	lpbb_setscl(dev, 1);
> +	lpbb_reset_bus(dev);
>
> 	if ((ppb_rstr(ppbus) & I2CKEY) ||
> 		((ppb_rstr(ppbus) & ALIM) != ALIM)) {
> -
> 		ppb_release_bus(ppbus, dev);
> +		ppb_unlock(ppbus);
> 		return (0);
> 	}
>
> 	ppb_release_bus(ppbus, dev);
> +	ppb_unlock(ppbus);
>
> 	return (1);
> }
> @@ -208,18 +222,17 @@ lpbb_reset(device_t dev, u_char speed, u
> {
> 	device_t ppbus = device_get_parent(dev);
>
> -	mtx_lock(&Giant);
> +	ppb_lock(ppbus);
> 	if (ppb_request_bus(ppbus, dev, PPB_DONTWAIT)) {
> +		ppb_unlock(ppbus);
> 		device_printf(dev, "can't allocate ppbus\n");
> 		return (0);
> 	}
>
> -	/* reset bus */
> -	lpbb_setsda(dev, 1);
> -	lpbb_setscl(dev, 1);
> +	lpbb_reset_bus(dev);
>
> 	ppb_release_bus(ppbus, dev);
> -	mtx_unlock(&Giant);
> +	ppb_unlock(ppbus);
>
> 	return (IIC_ENOADDR);
> }
>
> Modified: head/sys/dev/ppbus/lpt.c
> ==============================================================================
> --- head/sys/dev/ppbus/lpt.c	Wed Jan 21 21:48:46 2009	(r187575)
> +++ head/sys/dev/ppbus/lpt.c	Wed Jan 21 23:10:06 2009	(r187576)
> @@ -105,9 +105,9 @@ static int volatile lptflag = 1;
> #define	BUFSTATSIZE	32
>
> struct lpt_data {
> -	device_t dev;
> -	struct cdev *cdev;
> -	struct cdev *cdev_bypass;
> +	device_t sc_dev;
> +	struct cdev *sc_cdev;
> +	struct cdev *sc_cdev_bypass;
> 	short	sc_state;
> 	/* default case: negative prime, negative ack, handshake strobe,
> 	   prime once */
> @@ -130,9 +130,10 @@ struct lpt_data {
> #define	LP_ENABLE_IRQ	0x04	/* enable IRQ on open */
> #define	LP_ENABLE_EXT	0x10	/* we shall use advanced mode when possible */
> 	u_char	sc_backoff ;	/* time to call lptout() again */
> +	struct callout sc_timer;
>
> -	struct resource *intr_resource;	/* interrupt resource */
> -	void *intr_cookie;		/* interrupt registration cookie */
> +	struct resource *sc_intr_resource;	/* interrupt resource */
> +	void	*sc_intr_cookie;		/* interrupt cookie */
> };
>
> #define	LPT_NAME	"lpt"		/* our official name */
> @@ -144,8 +145,7 @@ static int	lpt_detect(device_t dev);
> #define	DEVTOSOFTC(dev) \
> 	((struct lpt_data *)device_get_softc(dev))
>
> -static void lptintr(device_t dev);
> -static void lpt_intr(void *arg);	/* without spls */
> +static void lptintr(void *arg);
>
> static devclass_t lpt_devclass;
>
> @@ -183,7 +183,6 @@ static	d_ioctl_t	lptioctl;
>
> static struct cdevsw lpt_cdevsw = {
> 	.d_version =	D_VERSION,
> -	.d_flags =	D_NEEDGIANT,
> 	.d_open =	lptopen,
> 	.d_close =	lptclose,
> 	.d_read =	lptread,
> @@ -199,13 +198,17 @@ lpt_request_ppbus(device_t dev, int how)
> 	struct lpt_data *sc = DEVTOSOFTC(dev);
> 	int error;
>
> +	/*
> +	 * We might already have the bus for a write(2) after an interrupted
> +	 * write(2) call.
> +	 */
> +	ppb_assert_locked(ppbus);
> 	if (sc->sc_state & HAVEBUS)
> 		return (0);
>
> -	/* we have the bus only if the request succeded */
> -	if ((error = ppb_request_bus(ppbus, dev, how)) == 0)
> +	error = ppb_request_bus(ppbus, dev, how);
> +	if (error == 0)
> 		sc->sc_state |= HAVEBUS;
> -
> 	return (error);
> }
>
> @@ -216,9 +219,12 @@ lpt_release_ppbus(device_t dev)
> 	struct lpt_data *sc = DEVTOSOFTC(dev);
> 	int error = 0;
>
> -	if ((error = ppb_release_bus(ppbus, dev)) == 0)
> -		sc->sc_state &= ~HAVEBUS;
> -
> +	ppb_assert_locked(ppbus);
> +	if (sc->sc_state & HAVEBUS) {
> +		error = ppb_release_bus(ppbus, dev);
> +		if (error == 0)
> +			sc->sc_state &= ~HAVEBUS;
> +	}
> 	return (error);
> }
>
> @@ -306,24 +312,25 @@ lpt_detect(device_t dev)
>
> 	status = 1;				/* assume success */
>
> +	ppb_lock(ppbus);
> 	if ((error = lpt_request_ppbus(dev, PPB_DONTWAIT))) {
> -		printf(LPT_NAME ": cannot alloc ppbus (%d)!\n", error);
> -		status = 0;
> -		goto end_probe;
> +		ppb_unlock(ppbus);
> +		device_printf(dev, "cannot alloc ppbus (%d)!\n", error);
> +		return (0);
> 	}
>
> 	for (i = 0; i < 18 && status; i++)
> 		if (!lpt_port_test(ppbus, testbyte[i], 0xff)) {
> 			status = 0;
> -			goto end_probe;
> +			break;
> 		}
>
> -end_probe:
> 	/* write 0's to control and data ports */
> 	ppb_wdtr(ppbus, 0);
> 	ppb_wctr(ppbus, 0);
>
> 	lpt_release_ppbus(dev);
> +	ppb_unlock(ppbus);
>
> 	return (status);
> }
> @@ -363,21 +370,33 @@ lpt_attach(device_t dev)
> 	int error;
>
> 	sc->sc_primed = 0;	/* not primed yet */
> +	ppb_init_callout(ppbus, &sc->sc_timer, 0);
>
> +	ppb_lock(ppbus);
> 	if ((error = lpt_request_ppbus(dev, PPB_DONTWAIT))) {
> -		printf(LPT_NAME ": cannot alloc ppbus (%d)!\n", error);
> +		ppb_unlock(ppbus);
> +		device_printf(dev, "cannot alloc ppbus (%d)!\n", error);
> 		return (0);
> 	}
>
> 	ppb_wctr(ppbus, LPC_NINIT);
> -
> -	/* check if we can use interrupt, should be done by ppc stuff */
> -	lprintf(("oldirq %x\n", sc->sc_irq));
> +	ppb_unlock(ppbus);
> +	lpt_release_ppbus(dev);
>
> 	/* declare our interrupt handler */
> -	sc->intr_resource = bus_alloc_resource_any(dev, SYS_RES_IRQ, &rid,
> +	sc->sc_intr_resource = bus_alloc_resource_any(dev, SYS_RES_IRQ, &rid,
> 	    RF_SHAREABLE);
> -	if (sc->intr_resource) {
> +	if (sc->sc_intr_resource) {
> +		error = bus_setup_intr(dev, sc->sc_intr_resource,
> +		    INTR_TYPE_TTY | INTR_MPSAFE, NULL, lptintr, sc,
> +		    &sc->sc_intr_cookie);
> +		if (error) {
> +			bus_release_resource(dev, SYS_RES_IRQ, rid,
> +			    sc->sc_intr_resource);
> +			device_printf(dev,
> +			    "Unable to register interrupt handler\n");
> +			return (error);
> +		}
> 		sc->sc_irq = LP_HAS_IRQ | LP_USE_IRQ | LP_ENABLE_IRQ;
> 		device_printf(dev, "Interrupt-driven port\n");
> 	} else {
> @@ -386,17 +405,17 @@ lpt_attach(device_t dev)
> 	}
> 	lprintf(("irq %x\n", sc->sc_irq));
>
> -	lpt_release_ppbus(dev);
> -
> -	sc->dev = dev;
> -	sc->cdev = make_dev(&lpt_cdevsw, unit,
> +	sc->sc_inbuf = malloc(BUFSIZE, M_DEVBUF, M_WAITOK);
> +	sc->sc_statbuf = malloc(BUFSTATSIZE, M_DEVBUF, M_WAITOK);
> +	sc->sc_dev = dev;
> +	sc->sc_cdev = make_dev(&lpt_cdevsw, unit,
> 	    UID_ROOT, GID_WHEEL, 0600, LPT_NAME "%d", unit);
> -	sc->cdev->si_drv1 = sc;
> -	sc->cdev->si_drv2 = 0;
> -	sc->cdev_bypass = make_dev(&lpt_cdevsw, unit,
> +	sc->sc_cdev->si_drv1 = sc;
> +	sc->sc_cdev->si_drv2 = 0;
> +	sc->sc_cdev_bypass = make_dev(&lpt_cdevsw, unit,
> 	    UID_ROOT, GID_WHEEL, 0600, LPT_NAME "%d.ctl", unit);
> -	sc->cdev_bypass->si_drv1 = sc;
> -	sc->cdev_bypass->si_drv2 = (void *)LP_BYPASS;
> +	sc->sc_cdev_bypass->si_drv1 = sc;
> +	sc->sc_cdev_bypass->si_drv2 = (void *)LP_BYPASS;
> 	return (0);
> }
>
> @@ -404,15 +423,21 @@ static int
> lpt_detach(device_t dev)
> {
> 	struct lpt_data *sc = DEVTOSOFTC(dev);
> +	device_t ppbus = device_get_parent(dev);
>
> -	destroy_dev(sc->cdev);
> -	destroy_dev(sc->cdev_bypass);
> +	destroy_dev(sc->sc_cdev);
> +	destroy_dev(sc->sc_cdev_bypass);
> +	ppb_lock(ppbus);
> 	lpt_release_ppbus(dev);
> -	if (sc->intr_resource != 0) {
> -		BUS_TEARDOWN_INTR(device_get_parent(dev), dev,
> -		    sc->intr_resource, sc->intr_cookie);
> -		bus_release_resource(dev, SYS_RES_IRQ, 0, sc->intr_resource);
> +	ppb_unlock(ppbus);
> +	callout_drain(&sc->sc_timer);
> +	if (sc->sc_intr_resource != NULL) {
> +		bus_teardown_intr(dev, sc->sc_intr_resource,
> +		    sc->sc_intr_cookie);
> +		bus_release_resource(dev, SYS_RES_IRQ, 0, sc->sc_intr_resource);
> 	}
> +	free(sc->sc_inbuf, M_DEVBUF);
> +	free(sc->sc_statbuf, M_DEVBUF);
>
> 	return (0);
> }
> @@ -420,18 +445,17 @@ lpt_detach(device_t dev)
> static void
> lptout(void *arg)
> {
> -	device_t dev = (device_t)arg;
> -	struct lpt_data *sc = DEVTOSOFTC(dev);
> -#ifdef LPT_DEBUG
> +	struct lpt_data *sc = arg;
> +	device_t dev = sc->sc_dev;
> 	device_t ppbus = device_get_parent(dev);
> -#endif
>
> +	ppb_assert_locked(ppbus);
> 	lprintf(("T %x ", ppb_rstr(ppbus)));
> 	if (sc->sc_state & OPEN) {
> 		sc->sc_backoff++;
> 		if (sc->sc_backoff > hz/LPTOUTMAX)
> 			sc->sc_backoff = sc->sc_backoff > hz/LPTOUTMAX;
> -		timeout(lptout, (caddr_t)dev, sc->sc_backoff);
> +		callout_reset(&sc->sc_timer, sc->sc_backoff, lptout, sc);
> 	} else
> 		sc->sc_state &= ~TOUT;
>
> @@ -442,7 +466,7 @@ lptout(void *arg)
> 	 * Avoid possible hangs due to missed interrupts
> 	 */
> 	if (sc->sc_xfercnt) {
> -		lptintr(dev);
> +		lptintr(sc);
> 	} else {
> 		sc->sc_state &= ~OBUSY;
> 		wakeup(dev);
> @@ -458,17 +482,19 @@ lptout(void *arg)
> static	int
> lptopen(struct cdev *dev, int flags, int fmt, struct thread *td)
> {
> -	int s;
> 	int trys, err;
> 	struct lpt_data *sc = dev->si_drv1;
> -	device_t lptdev = sc->dev;
> +	device_t lptdev = sc->sc_dev;
> 	device_t ppbus = device_get_parent(lptdev);
>
> 	if (!sc)
> 		return (ENXIO);
>
> +	ppb_lock(ppbus);
> 	if (sc->sc_state) {
> -		lprintf((LPT_NAME ": still open %x\n", sc->sc_state));
> +		lprintf(("%s: still open %x\n", device_get_nameunit(lptdev),
> +		    sc->sc_state));
> +		ppb_unlock(ppbus);
> 		return(EBUSY);
> 	} else
> 		sc->sc_state |= LPTINIT;
> @@ -478,6 +504,7 @@ lptopen(struct cdev *dev, int flags, int
> 	/* Check for open with BYPASS flag set. */
> 	if (sc->sc_flags & LP_BYPASS) {
> 		sc->sc_state = OPEN;
> +		ppb_unlock(ppbus);
> 		return(0);
> 	}
>
> @@ -485,11 +512,12 @@ lptopen(struct cdev *dev, int flags, int
> 	if ((err = lpt_request_ppbus(lptdev, PPB_WAIT|PPB_INTR)) != 0) {
> 		/* give it a chance to try later */
> 		sc->sc_state = 0;
> +		ppb_unlock(ppbus);
> 		return (err);
> 	}
>
> -	s = spltty();
> -	lprintf((LPT_NAME " flags 0x%x\n", sc->sc_flags));
> +	lprintf(("%s flags 0x%x\n", device_get_nameunit(lptdev),
> +	    sc->sc_flags));
>
> 	/* set IRQ status according to ENABLE_IRQ flag
> 	 */
> @@ -514,21 +542,21 @@ lptopen(struct cdev *dev, int flags, int
> 	do {
> 		/* ran out of waiting for the printer */
> 		if (trys++ >= LPINITRDY*4) {
> -			splx(s);
> 			sc->sc_state = 0;
> 			lprintf(("status %x\n", ppb_rstr(ppbus)));
>
> 			lpt_release_ppbus(lptdev);
> +			ppb_unlock(ppbus);
> 			return (EBUSY);
> 		}
>
> 		/* wait 1/4 second, give up if we get a signal */
> -		if (tsleep(lptdev, LPPRI|PCATCH, "lptinit", hz/4) !=
> -		    EWOULDBLOCK) {
> +		if (ppb_sleep(ppbus, lptdev, LPPRI | PCATCH, "lptinit",
> +		    hz / 4) != EWOULDBLOCK) {
> 			sc->sc_state = 0;
> -			splx(s);
>
> 			lpt_release_ppbus(lptdev);
> +			ppb_unlock(ppbus);
> 			return (EBUSY);
> 		}
>
> @@ -548,22 +576,20 @@ lptopen(struct cdev *dev, int flags, int
> 	ppb_wctr(ppbus, sc->sc_control);
>
> 	sc->sc_state = OPEN;
> -	sc->sc_inbuf = malloc(BUFSIZE, M_DEVBUF, M_WAITOK);
> -	sc->sc_statbuf = malloc(BUFSTATSIZE, M_DEVBUF, M_WAITOK);
> 	sc->sc_xfercnt = 0;
> -	splx(s);
> -
> -	/* release the ppbus */
> -	lpt_release_ppbus(lptdev);
>
> 	/* only use timeout if using interrupt */
> 	lprintf(("irq %x\n", sc->sc_irq));
> 	if (sc->sc_irq & LP_USE_IRQ) {
> 		sc->sc_state |= TOUT;
> -		timeout(lptout, (caddr_t)lptdev,
> -			 (sc->sc_backoff = hz/LPTOUTINITIAL));
> +		sc->sc_backoff = hz / LPTOUTINITIAL;
> +		callout_reset(&sc->sc_timer, sc->sc_backoff, lptout, sc);
> 	}
>
> +	/* release the ppbus */
> +	lpt_release_ppbus(lptdev);
> +	ppb_unlock(ppbus);
> +
> 	lprintf(("opened.\n"));
> 	return(0);
> }
> @@ -578,17 +604,21 @@ static	int
> lptclose(struct cdev *dev, int flags, int fmt, struct thread *td)
> {
> 	struct lpt_data *sc = dev->si_drv1;
> -	device_t lptdev = sc->dev;
>
> *** DIFF OUTPUT TRUNCATED AT 1000 LINES ***
>



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