kern/118093: [firewire] firewire bus reset hogs CPU, causing data to be lost

[Dieter]

The following reply was made to PR kern/118093; it has been noted by GNATS.

From: Dieter <freebsd at sopwith.solgatos.com>
To: bug-followup at FreeBSD.org, freebsd-firewire at FreeBSD.org
Cc:  
Subject: Re: kern/118093: [firewire] firewire bus reset hogs CPU, causing data to be lost 
Date: Mon, 05 Jan 2009 17:52:29 +0000

 In message <1231181797.21260.8.camel at localhost.localdomain>, Sean Bruno writes:
 
 > @@ -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);
 
 If the (null these days) spl calls have been replaced with FW_GLOCK/FW_GUNLOCK
 calls, can the spl calls be removed now?
 
 > @@ -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++){
 
 This needs to be reviewed by someone who understands what is protected by
 getting a lock and then immediately releasing it.  I have no clue.
 
 > @@ -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__);
 >  		}
 
 Should this be "too many cycles lost, " and  "no cycle master present?\n"
 
 >  	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);
 >  }
 
 Does the lock need to protect the printf?