From owner-cvs-all@FreeBSD.ORG Wed Feb 21 17:50:50 2007 Return-Path: X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B0135170392; Wed, 21 Feb 2007 17:50:50 +0000 (UTC) (envelope-from gallatin@cs.duke.edu) Received: from duke.cs.duke.edu (duke.cs.duke.edu [152.3.140.1]) by mx1.freebsd.org (Postfix) with ESMTP id 6F7D013C4B8; Wed, 21 Feb 2007 17:50:50 +0000 (UTC) (envelope-from gallatin@cs.duke.edu) Received: from grasshopper.cs.duke.edu (grasshopper.cs.duke.edu [152.3.145.30]) by duke.cs.duke.edu (8.14.0/8.14.0) with ESMTP id l1LHonvR016020 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 21 Feb 2007 12:50:49 -0500 (EST) Received: (from gallatin@localhost) by grasshopper.cs.duke.edu (8.12.9p2/8.12.9/Submit) id l1LHoipQ020329; Wed, 21 Feb 2007 12:50:44 -0500 (EST) (envelope-from gallatin) From: Andrew Gallatin MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <17884.34420.308021.423716@grasshopper.cs.duke.edu> Date: Wed, 21 Feb 2007 12:50:44 -0500 (EST) To: Luigi Rizzo In-Reply-To: <20070221092332.A90766@xorpc.icir.org> References: <200702151721.l1FHLWno019525@repoman.freebsd.org> <20070221121302.A20229@grasshopper.cs.duke.edu> <20070221092332.A90766@xorpc.icir.org> X-Mailer: VM 6.75 under 21.1 (patch 12) "Channel Islands" XEmacs Lucid Cc: cvs-src@FreeBSD.org, Luigi Rizzo , src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/arm/xscale/ixp425 ixp425_npe.c src/sys/dev/ipw if_ipw.c if_ipwvar.h src/sys/dev/isp isp_freebsd.h src/sys/dev/iwi if_iwi.c if_iwivar.h src/sys/dev/mxge if_mxge.c src/sys/kern subr_firmware.c src/sys/sys firmware.h src/sys/tools fw_stub.awk X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Feb 2007 17:50:50 -0000 Luigi Rizzo writes: > On Wed, Feb 21, 2007 at 12:13:02PM -0500, Andrew Gallatin wrote: > > Luigi Rizzo [luigi@FreeBSD.org] wrote: > > > > > Cleanup and document the implementation of firmware(9) based on > > > a version that i posted earlier on the -current mailing list, > > > and subsequent feedback received. > > > > > > > At least for me, firmware(9) has been broken ever since the kld_mtx > > was replaced with an sx lock last June. The problem is that there is > > an exclusive lock of kld_sx taken when loading a driver, and then > > firmware_get() triggers another xlock of it, leading to a deadlock: > ... > > > > I've been using a patch > > (http://people.freebsd.org/~gallatin/firmware_sx_recurse.diff) > > which works around the problem. Do you think it would be > > possible to commit this? > > i suppose it is ok... "iwi" uses a similar technique to avoid Would you mind committing it for me? I honestly don't feel comfortable touching anything in the core kernel, especially locking. I've tried the patch under WITNESS, and it doesn't complain, but I fully admit that I don't really understand the locking requirements of the linker. BTW, I'm somewhat surprised nobody else has complained about this. I see that iwi loads its firmware from an ioctl context, but at least isp seems to load its firmware from attach. I guess not many people load scsi drivers after the system is up.. > recursive locking. I wonder how common is this practice, and whether > it makes sense to define some standard macros to implement this. I'm not sure.. In some cases, recursive locking is OK (just icky), and in other cases you really want to catch it. Speaking of icky, I'm really worried about the pedantic nature of FreeBSD's WITNESS and "never, ever sleep while holding a mutex" dogma causing as many problems as it solves. For example, iwi seems to simply drop its lock when loading firmware (presumably to avoid WITNESS squawking about holding a mutex while grabbing an sx). This certainly shuts up WITNESS, but it may also introduce a race, especially if the linker needs to sleep and wait for the disk. Drew