From owner-cvs-all@FreeBSD.ORG  Tue May  6 13:57:47 2003
Return-Path: <owner-cvs-all@FreeBSD.ORG>
Delivered-To: cvs-all@freebsd.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP id D6AE837B401
	for <cvs-all@FreeBSD.org>; Tue,  6 May 2003 13:57:47 -0700 (PDT)
Received: from rootlabs.com (root.org [67.118.192.226])
	by mx1.FreeBSD.org (Postfix) with SMTP id 6D1BC43FCB
	for <cvs-all@FreeBSD.org>; Tue,  6 May 2003 13:57:46 -0700 (PDT)
	(envelope-from nate@rootlabs.com)
Received: (qmail 17884 invoked by uid 1000); 6 May 2003 20:57:47 -0000
Date: Tue, 6 May 2003 13:57:47 -0700 (PDT)
From: Nate Lawson <nate@root.org>
To: Olivier Houchard <cognet@FreeBSD.org>
In-Reply-To: <20030506020032.8318437B478@hub.freebsd.org>
Message-ID: <20030506133223.V29676@root.org>
References: <20030506020032.8318437B478@hub.freebsd.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
cc: cvs-src@FreeBSD.org
cc: src-committers@FreeBSD.org
cc: cvs-all@FreeBSD.org
Subject: Re: cvs commit: src/sys/pci if_sis.c
X-BeenThere: cvs-all@freebsd.org
X-Mailman-Version: 2.1.1
Precedence: list
List-Id: CVS commit messages for the entire tree <cvs-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/cvs-all>
List-Post: <mailto:cvs-all@freebsd.org>
List-Help: <mailto:cvs-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 06 May 2003 20:57:48 -0000

On Mon, 5 May 2003, Olivier Houchard wrote:
>   Modified files:
>     sys/pci              if_sis.c
>   Log:
>   Don't call timeout() in sis_tick(), this is done earlier by mii_tick(), and it
>   leads to a panic at unload time, as we own 2 instances of callout and
>   untimeout() only one.
>   Will I'm there, remove a call to callout_handler_init(), one is enough.
>
>   Reviewed by:    wpaul

Comments below.

> --- src/sys/pci/if_sis.c:1.74	Mon Apr 21 11:34:04 2003
> +++ src/sys/pci/if_sis.c	Mon May  5 19:00:01 2003
> @@ -29,7 +29,7 @@
>   * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
>   * THE POSSIBILITY OF SUCH DAMAGE.
>   *
> - * $FreeBSD: /repoman/r/ncvs/src/sys/pci/if_sis.c,v 1.74 2003/04/21 18:34:04 imp Exp $
> + * $FreeBSD: /repoman/r/ncvs/src/sys/pci/if_sis.c,v 1.75 2003/05/06 02:00:01 cognet Exp $
>   */
>
>  /*
> @@ -58,7 +58,7 @@
>   */
>
>  #include <sys/cdefs.h>
> -__FBSDID("$FreeBSD: /repoman/r/ncvs/src/sys/pci/if_sis.c,v 1.74 2003/04/21 18:34:04 imp Exp $");
> +__FBSDID("$FreeBSD: /repoman/r/ncvs/src/sys/pci/if_sis.c,v 1.75 2003/05/06 02:00:01 cognet Exp $");
>
>  #include <sys/param.h>
>  #include <sys/systm.h>

Why do we have both the RCS id and FBSDID?  I thought only one was enough.

> @@ -1374,8 +1374,6 @@
>  		goto fail;
>  	}
>
> -	callout_handle_init(&sc->sis_stat_ch);
> -
>  	/*
>  	 * Call MI attach routine.
>  	 */

I would rather you deleted the first call to callout_handle_init() instead
(after the "Inform the world" comment).  Reasons include poor style for
the first one and uniformity with other sys/pci/if_* drivers which do the
init right before ether_ifattach().

> @@ -1747,8 +1745,6 @@
>  		if (ifp->if_snd.ifq_head != NULL)
>  			sis_start(ifp);
>  	}
> -
> -	sc->sis_stat_ch = timeout(sis_tick, sc, hz);
>
>  	SIS_UNLOCK(sc);
>

Can you check the other sys/pci/if_* devices to see if they have the same
bug?  I think they all re-add the timeout at the end of *_tick().

-Nate