Date: Tue, 6 May 2003 13:57:47 -0700 (PDT) From: Nate Lawson <nate@root.org> To: Olivier Houchard <cognet@FreeBSD.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/pci if_sis.c Message-ID: <20030506133223.V29676@root.org> In-Reply-To: <20030506020032.8318437B478@hub.freebsd.org> References: <20030506020032.8318437B478@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030506133223.V29676>