From owner-freebsd-firewire@FreeBSD.ORG  Tue Jan  6 05:19:00 2009
Return-Path: <owner-freebsd-firewire@FreeBSD.ORG>
Delivered-To: freebsd-firewire@FreeBSD.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34])
	by hub.freebsd.org (Postfix) with ESMTP id B1745106564A;
	Tue,  6 Jan 2009 05:19:00 +0000 (UTC)
	(envelope-from freebsd@sopwith.solgatos.com)
Received: from sopwith.solgatos.com
	(pool-71-117-207-61.ptldor.fios.verizon.net [71.117.207.61])
	by mx1.freebsd.org (Postfix) with ESMTP id B76108FC14;
	Tue,  6 Jan 2009 05:18:59 +0000 (UTC)
	(envelope-from freebsd@sopwith.solgatos.com)
Received: by sopwith.solgatos.com (Postfix, from userid 66)
	id 97D6FB650; Mon,  5 Jan 2009 05:12:47 -0800 (PST)
Received: from localhost by sopwith.solgatos.com (8.8.8/6.24)
	id BAA21475; Tue, 6 Jan 2009 01:52:29 GMT
Message-Id: <200901060152.BAA21475@sopwith.solgatos.com>
To: bug-followup@FreeBSD.org, freebsd-firewire@FreeBSD.org
In-reply-to: Your message of "Mon, 05 Jan 2009 10:56:37 PST."
	<1231181797.21260.8.camel@localhost.localdomain> 
Date: Mon, 05 Jan 2009 17:52:29 +0000
From: Dieter <freebsd@sopwith.solgatos.com>
Cc: 
Subject: Re: kern/118093: [firewire] firewire bus reset hogs CPU,
	causing data to be lost 
X-BeenThere: freebsd-firewire@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: Firewire support in FreeBSD <freebsd-firewire.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-firewire>, 
	<mailto:freebsd-firewire-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-firewire>
List-Post: <mailto:freebsd-firewire@freebsd.org>
List-Help: <mailto:freebsd-firewire-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-firewire>, 
	<mailto:freebsd-firewire-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 06 Jan 2009 05:19:01 -0000

In message <1231181797.21260.8.camel@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?