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

Dieter <[email protected]>
Newsgroups gmane.os.freebsd.devel.firewire
Message-ID <[email protected]>
In message <[email protected]>, 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?
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-firewire
To unsubscribe, send any mail to "[email protected]"
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.