Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 5 Jan 2009 19:00:11 GMT
From:      Sean Bruno <sean.bruno@dsl-only.net>
To:        freebsd-bugs@FreeBSD.org
Subject:   Re: kern/118093: [firewire] firewire bus reset hogs CPU, causing data to be lost
Message-ID:  <200901051900.n05J0BDl060894@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/118093; it has been noted by GNATS.

From: Sean Bruno <sean.bruno@dsl-only.net>
To: bug-followup@FreeBSD.org, freebsd@sopwith.solgatos.com,  freebsd-firewire@freebsd.org
Cc: scottl@freebsd.org
Subject: Re: kern/118093: [firewire] firewire bus reset hogs CPU, causing
 data to be lost
Date: Mon, 05 Jan 2009 10:56:37 -0800

 --=-bdk1tSakYWl4EHnChn2C
 Content-Type: text/plain
 Content-Transfer-Encoding: 7bit
 
 Well ... this has really sent me down the rabbit hole the last couple of
 days.
 
 There is a need to audit all locking in the firewire stack right now and
 I have started that task.
 
 Essentially, threads, callouts, interrupts and task queues are all
 jumping around causing context to be switched from one thread to
 another.  It's kind of bad in there and I need to sort it out.
 
 I'm working with a stable/7 tree, so I've started a patch for it.  This
 patch has quite a few printf -> device_printf changes in it, so try to 
 ignore thost for now.
 
 The meat of the patch is a judicious implementation of FW_GLOCK() in
 certain code areas.  Note that sometimes the code is just trying to get
 the lock and then drops it immediately.  This is not very optimal, but
 it does the trick.
 
 I'm still seeing a high level of broken log messages
 in /var/log/messages but this may help the issue you were seeing.  Give
 it a spin.
 
 Sean
 
 --=-bdk1tSakYWl4EHnChn2C
 Content-Disposition: attachment; filename="firewire.diff"
 Content-Type: text/x-patch; name="firewire.diff"; charset="UTF-8"
 Content-Transfer-Encoding: 7bit
 
 Index: fwohci_pci.c
 ===================================================================
 --- fwohci_pci.c	(revision 186781)
 +++ fwohci_pci.c	(working copy)
 @@ -337,11 +337,7 @@
  
  	err = bus_setup_intr(self, sc->irq_res,
  			INTR_TYPE_NET | INTR_MPSAFE,
 -#if FWOHCI_INTFILT
 -		     fwohci_filt, NULL, sc, &sc->ih);
 -#else
  		     NULL, (driver_intr_t *) fwohci_intr, sc, &sc->ih);
 -#endif
  #if defined(__DragonFly__) || __FreeBSD_version < 500000
  	/* XXX splcam() should mask this irq for sbp.c*/
  	err = bus_setup_intr(self, sc->irq_res, INTR_TYPE_CAM,
 Index: firewire.c
 ===================================================================
 --- firewire.c	(revision 186781)
 +++ firewire.c	(working copy)
 @@ -1240,8 +1240,8 @@
  	fp->mode.common.tcode |= FWTCODE_PHY;
  
  	if (firewire_debug)
 -		printf("send phy_config root_node=%d gap_count=%d\n",
 -						root_node, gap_count);
 +		device_printf(fc->dev, "%s: phy_config root_node=%d gap_count=%d\n",
 +				__func__, root_node, gap_count);
  	fw_asyreq(fc, -1, xfer);
  }
  
 @@ -1285,7 +1285,7 @@
  	self_id = &fc->topology_map->self_id[0];
  	for(i = 0; i < fc->sid_cnt; i ++){
  		if (sid[1] != ~sid[0]) {
 -			printf("fw_sidrcv: invalid self-id packet\n");
 +			device_printf(fc->bdev, "%s: invalid self-id packet\n", __func__);
  			sid += 2;
  			continue;
  		}
 @@ -1331,7 +1331,6 @@
  		self_id++;
  		fc->topology_map->self_id_count ++;
  	}
 -	device_printf(fc->bdev, "%d nodes", fc->max_node + 1);
  	/* CRC */
  	fc->topology_map->crc = fw_crc16(
  			(uint32_t *)&fc->topology_map->generation,
 @@ -1350,16 +1349,16 @@
  	bcopy(p, &CSRARC(fc, SPED_MAP + 8), (fc->speed_map->crc_len - 1)*4);
  
  	fc->max_hop = fc->max_node - i_branch;
 -	printf(", maxhop <= %d", fc->max_hop);
 +	device_printf(fc->bdev, "%s: %d nodes, maxhop <= %d\n",
 +			__func__, fc->max_node + 1, fc->max_hop);
  		
  	if(fc->irm == -1 ){
 -		printf(", Not found IRM capable node");
 +		device_printf(fc->bdev, "%s: No IRM capable node", __func__);
  	}else{
 -		printf(", cable IRM = %d", fc->irm);
 -		if (fc->irm == fc->nodeid)
 -			printf(" (me)");
 +		device_printf(fc->bdev, "%s: IRM capable node = %d, %s\n",
 +				 __func__, fc->irm,
 +				(fc->irm == fc->nodeid) ? " (me)" : "");
  	}
 -	printf("\n");
  
  	if (try_bmr && (fc->irm != -1) && (CSRARC(fc, BUS_MGR_ID) == 0x3f)) {
  		if (fc->irm == fc->nodeid) {
 @@ -1388,6 +1387,7 @@
  	struct fw_device *fwdev;
  
  	s = splfw();
 +	FW_GLOCK(fc);
  	fc->status = FWBUSEXPLORE;
  
  	/* Invalidate all devices, just after bus reset. */
 @@ -1396,6 +1396,7 @@
  			fwdev->status = FWDEVINVAL;
  			fwdev->rcnt = 0;
  		}
 +	FW_GUNLOCK(fc);
  	splx(s);
  
  	wakeup((void *)fc);
 @@ -1647,20 +1648,27 @@
  
  	fc = (struct firewire_comm *)arg;
  
 -	mtx_lock(&fc->wait_lock);
 +	/*mtx_lock(&fc->wait_lock);*/
 +	FW_GLOCK(fc);
  	while (fc->status != FWBUSDETACH) {
 +		FW_GUNLOCK(fc);
  		if (fc->status == FWBUSEXPLORE) {
 -			mtx_unlock(&fc->wait_lock);
 +			/*mtx_unlock(&fc->wait_lock);*/
  			fw_explore(fc);
  			fc->status = FWBUSEXPDONE;
 -			if (firewire_debug)
 -				printf("bus_explore done\n");
 +			if (firewire_debug) {
 +				device_printf(fc->dev,"%s: bus_explore done\n",
 +						__func__);
 +			}
  			fw_attach_dev(fc);
 -			mtx_lock(&fc->wait_lock);
 +			/*mtx_lock(&fc->wait_lock);*/
  		}
 -		msleep((void *)fc, &fc->wait_lock, PWAIT|PCATCH, "-", 0);
 +		FW_GLOCK(fc);
 +		/*msleep((void *)fc, &fc->wait_lock, PWAIT|PCATCH, "-", 0);*/
 +		msleep((void *)fc, FW_GMTX(fc), PWAIT|PCATCH, "-", 0);
  	}
 -	mtx_unlock(&fc->wait_lock);
 +	/*mtx_unlock(&fc->wait_lock);*/
 +	FW_GUNLOCK(fc);
  	kthread_exit(0);
  }
  
 Index: fwohci.c
 ===================================================================
 --- fwohci.c	(revision 186781)
 +++ fwohci.c	(working copy)
 @@ -1003,7 +1003,8 @@
  	if (maxdesc < db_tr->dbcnt) {
  		maxdesc = db_tr->dbcnt;
  		if (firewire_debug)
 -			device_printf(sc->fc.dev, "maxdesc: %d\n", maxdesc);
 +			device_printf(sc->fc.dev, "%s: maxdesc: %d\n",
 +					__func__, maxdesc);
  	}
  	/* last db */
  	LAST_DB(db_tr, db);
 @@ -1024,7 +1025,8 @@
  	if(db_tr != dbch->bottom){
  		goto txloop;
  	} else {
 -		device_printf(sc->fc.dev, "fwohci_start: lack of db_trq\n");
 +		device_printf(sc->fc.dev, "%s: lack of db_trq\n",
 +				__func__);
  		dbch->flags |= FWOHCI_DBCH_FULL;
  	}
  kick:
 @@ -1036,8 +1038,8 @@
  		OWRITE(sc, OHCI_DMACTL(off), OHCI_CNTL_DMA_WAKE);
  	} else {
  		if (firewire_debug)
 -			device_printf(sc->fc.dev, "start AT DMA status=%x\n",
 -					OREAD(sc, OHCI_DMACTL(off)));
 +			device_printf(sc->fc.dev, "%s: start AT DMA status=%x\n",
 +					__func__, OREAD(sc, OHCI_DMACTL(off)));
  		OWRITE(sc, OHCI_DMACMD(off), dbch->top->bus_addr | fsegment);
  		OWRITE(sc, OHCI_DMACTL(off), OHCI_CNTL_DMA_RUN);
  		dbch->xferq.flag |= FWXFERQ_RUNNING;
 @@ -1848,7 +1850,7 @@
  		/* Disable bus reset interrupt until sid recv. */
  		OWRITE(sc, FWOHCI_INTMASKCLR,  OHCI_INT_PHY_BUS_R);
  	
 -		device_printf(fc->dev, "BUS reset\n");
 +		device_printf(fc->dev, "%s: BUS reset\n", __func__);
  		OWRITE(sc, FWOHCI_INTMASKCLR,  OHCI_INT_CYC_LOST);
  		OWRITE(sc, OHCI_LNKCTLCLR, OHCI_CNTL_CYCSRC);
  
 @@ -1885,10 +1887,10 @@
  		plen = OREAD(sc, OHCI_SID_CNT);
  
  		fc->nodeid = node_id & 0x3f;
 -		device_printf(fc->dev, "node_id=0x%08x, gen=%d, ",
 -			node_id, (plen >> 16) & 0xff);
 +		device_printf(fc->dev, "%s: node_id=0x%08x, gen=%d\n",
 +			__func__, node_id, (plen >> 16) & 0xff);
  		if (!(node_id & OHCI_NODE_VALID)) {
 -			printf("Bus reset failure\n");
 +			device_printf(fc->dev, "%s: Bus reset failure\n", __func__);
  			goto sidout;
  		}
  
 @@ -1896,11 +1898,11 @@
  		sc->cycle_lost = 0;
  		OWRITE(sc, FWOHCI_INTMASK,  OHCI_INT_CYC_LOST);
  		if ((node_id & OHCI_NODE_ROOT) && !nocyclemaster) {
 -			printf("CYCLEMASTER mode\n");
 +			device_printf(fc->dev, "%s: CYCLEMASTER mode\n", __func__);
  			OWRITE(sc, OHCI_LNKCTL,
  				OHCI_CNTL_CYCMTR | OHCI_CNTL_CYCTIMER);
  		} else {
 -			printf("non CYCLEMASTER mode\n");
 +			device_printf(fc->dev, "%s: non CYCLEMASTER mode\n", __func__);
  			OWRITE(sc, OHCI_LNKCTLCLR, OHCI_CNTL_CYCMTR);
  			OWRITE(sc, OHCI_LNKCTL, OHCI_CNTL_CYCTIMER);
  		}
 @@ -1922,6 +1924,8 @@
  	u_int i;
  	struct firewire_comm *fc = (struct firewire_comm *)sc;
  
 +	FW_GLOCK(fc);
 +	FW_GUNLOCK(fc);
  	if (stat & OHCI_INT_DMA_IR) {
  		irstat = atomic_readandclear_int(&sc->irstat);
  		for(i = 0; i < fc->nisodma ; i++){
 @@ -1969,8 +1973,8 @@
  			OWRITE(sc, OHCI_LNKCTLCLR, OHCI_CNTL_CYCTIMER);
  #endif
  			OWRITE(sc, FWOHCI_INTMASKCLR,  OHCI_INT_CYC_LOST);
 -			device_printf(fc->dev, "too many cycle lost, "
 -			 "no cycle master presents?\n");
 +			device_printf(fc->dev, "%s: too many cycle lost, "
 +			 "no cycle master presents?\n", __func__);
  		}
  	}
  	if (stat & OHCI_INT_DMA_ATRQ) {
 @@ -1986,7 +1990,7 @@
  		device_printf(fc->dev, "unrecoverable error\n");
  	}
  	if (stat & OHCI_INT_PHY_INT) {
 -		device_printf(fc->dev, "phy int\n");
 +		device_printf(fc->dev, "%s: phy int\n", __func__);
  	}
  
  	return;
 @@ -1997,6 +2001,8 @@
  {
  	struct fwohci_softc *sc = (struct fwohci_softc *)arg;
  
 +	FW_GLOCK(&sc->fc);
 +	FW_GUNLOCK(&sc->fc);
  	fw_busreset(&sc->fc, FWBUSRESET);
  	OWRITE(sc, OHCI_CROMHDR, ntohl(sc->fc.config_rom[0]));
  	OWRITE(sc, OHCI_BUS_OPT, ntohl(sc->fc.config_rom[2]));
 @@ -2011,6 +2017,8 @@
  	int i, plen;
  
  
 +	FW_GLOCK(fc);
 +	FW_GUNLOCK(fc);
  	plen = OREAD(sc, OHCI_SID_CNT);
  
  	if (plen & OHCI_SID_ERR) {
 @@ -2062,7 +2070,16 @@
  {
  	uint32_t stat, irstat, itstat;
  
 +	/*
 +	 * Try and grab the global lock
 +	 * for a moment to keep us from
 +	 * firing while we are still 
 +	 * setting the interrupt condition
 +	 * on the bus
 +	 */
 +	FW_GLOCK(&sc->fc);
  	stat = OREAD(sc, FWOHCI_INTSTAT);
 +	FW_GUNLOCK(&sc->fc);
  	if (stat == 0xffffffff) {
  		device_printf(sc->fc.dev, 
  			"device physically ejected?\n");
 @@ -2091,25 +2108,15 @@
  	return (FILTER_HANDLED);
  }
  
 -int
 -fwohci_filt(void *arg)
 +void
 +fwohci_intr(void *arg)
  {
  	struct fwohci_softc *sc = (struct fwohci_softc *)arg;
  
 -	if (!(sc->intmask & OHCI_INT_EN)) {
 -		/* polling mode */
 -		return (FILTER_STRAY);
 -	}
 -	return (fwohci_check_stat(sc));
 +	fwohci_check_stat(sc);
  }
  
  void
 -fwohci_intr(void *arg)
 -{
 -	fwohci_filt(arg);
 -}
 -
 -void
  fwohci_poll(struct firewire_comm *fc, int quick, int count)
  {
  	struct fwohci_softc *sc = (struct fwohci_softc *)fc;
 @@ -2464,6 +2471,7 @@
  	struct fwohci_softc *sc;
  	uint32_t fun;
  
 +	FW_GLOCK(fc);
  	device_printf(fc->dev, "Initiate bus reset\n");
  	sc = (struct fwohci_softc *)fc;
  
 @@ -2487,6 +2495,7 @@
  	fun |= FW_PHY_ISBR | FW_PHY_RHB;
  	fun = fwphy_wrdata(sc, FW_PHY_ISBR_REG, fun);
  #endif
 +	FW_GUNLOCK(fc);
  }
  
  void
 Index: fwohcivar.h
 ===================================================================
 --- fwohcivar.h	(revision 186781)
 +++ fwohcivar.h	(working copy)
 @@ -37,12 +37,6 @@
  
  #include <sys/taskqueue.h>
  
 -#if defined(__DragonFly__) ||  __FreeBSD_version < 700043
 -#define FWOHCI_INTFILT	0
 -#else
 -#define FWOHCI_INTFILT	1
 -#endif
 -
  typedef struct fwohci_softc {
  	struct firewire_comm fc;
  	bus_space_tag_t bst;
 @@ -84,7 +78,6 @@
  } fwohci_softc_t;
  
  void fwohci_intr (void *arg);
 -int fwohci_filt (void *arg);
  int fwohci_init (struct fwohci_softc *, device_t);
  void fwohci_poll (struct firewire_comm *, int, int);
  void fwohci_reset (struct fwohci_softc *, device_t);
 
 --=-bdk1tSakYWl4EHnChn2C--
 



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