From owner-freebsd-current@FreeBSD.ORG Thu Feb 15 17:43:44 2007 Return-Path: X-Original-To: current@freebsd.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 9356816A400 for ; Thu, 15 Feb 2007 17:43:44 +0000 (UTC) (envelope-from rizzo@icir.org) Received: from xorpc.icir.org (xorpc.icir.org [192.150.187.68]) by mx1.freebsd.org (Postfix) with ESMTP id 6AFDF13C461 for ; Thu, 15 Feb 2007 17:43:44 +0000 (UTC) (envelope-from rizzo@icir.org) Received: from xorpc.icir.org (localhost [127.0.0.1]) by xorpc.icir.org (8.12.11/8.13.6) with ESMTP id l1FHhgaY091902; Thu, 15 Feb 2007 09:43:42 -0800 (PST) (envelope-from rizzo@xorpc.icir.org) Received: (from rizzo@localhost) by xorpc.icir.org (8.12.11/8.12.3/Submit) id l1FHhg7X091901; Thu, 15 Feb 2007 09:43:42 -0800 (PST) (envelope-from rizzo) Date: Thu, 15 Feb 2007 09:43:42 -0800 From: Luigi Rizzo To: current@freebsd.org Message-ID: <20070215094342.B91479@xorpc.icir.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5.1i Cc: Subject: one last firmware(9) issue X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Feb 2007 17:43:44 -0000 I have committed the cleanup to firmware(9) discussed earlier on this list, see the commit log below. There is a remaining issue on which i would like some advice. From the commit log: Note also that there is a subtle issue with the implementation of firmware_register(): currently, as in the previous version, we just store a reference to the 'imagename' argument, but we should rather copy it because there is no guarantee that this is a static string. Now, what do you think is the best way to handle this ? The existing code tries not to allocate memory on a firmware_register(), not sure if it is on purpose (e.g. to avoid sleeping) or for other reasons. To preserve this, we should use static buffers for the image names, and pick a reasonable size for them (128 ? 256 ? 1025 ?). If on the other hand, we can afford a malloc in firmware_register(), i'd move the whole registry to a linked list, to avoid the hard limit of 30 slots in the firmware table. suggestions ? cheers luigi > luigi 2007-02-15 17:21:31 UTC > > FreeBSD src repository > > Modified files: > sys/arm/xscale/ixp425 ixp425_npe.c > sys/dev/ipw if_ipw.c if_ipwvar.h > sys/dev/isp isp_freebsd.h > sys/dev/iwi if_iwi.c if_iwivar.h > sys/dev/mxge if_mxge.c > sys/kern subr_firmware.c > sys/sys firmware.h > sys/tools fw_stub.awk > Log: > 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. > > The core of the change is just in sys/firmware.h and kern/subr_firmware.c, > while other files are just adaptation of the clients to the ABI change > (const-ification of some parameters and hiding of internal info, > so this is fully compatible at the binary level). > > In detail: > - reduce the amount of information exported to clients in struct firmware, > and constify the pointer; > > - internally, document and simplify the implementation of the various > functions, and make sure error conditions are dealt with properly. > > The diffs are large, but the code is really straightforward now (i hope). > > Note also that there is a subtle issue with the implementation of > firmware_register(): currently, as in the previous version, we just > store a reference to the 'imagename' argument, but we should rather > copy it because there is no guarantee that this is a static string. > I realised this while testing this code, but i prefer to fix it in > a later commit -- there is no regression with respect to the past. > > Note, too, that the version in RELENG_6 has various bugs including > missing locks around the module release calls, mishandling of modules > loaded by /boot/loader, and so on, so an MFC is absolutely necessary > there. I was just postponing it until this cleanup to avoid doing > things twice. > > MFC after: 1 week > > Revision Changes Path > 1.2 +1 -1 src/sys/arm/xscale/ixp425/ixp425_npe.c > 1.23 +1 -1 src/sys/dev/ipw/if_ipw.c > 1.6 +1 -1 src/sys/dev/ipw/if_ipwvar.h > 1.99 +1 -1 src/sys/dev/isp/isp_freebsd.h > 1.46 +2 -2 src/sys/dev/iwi/if_iwi.c > 1.12 +1 -1 src/sys/dev/iwi/if_iwivar.h > 1.21 +1 -1 src/sys/dev/mxge/if_mxge.c > 1.9 +272 -153 src/sys/kern/subr_firmware.c > 1.4 +15 -16 src/sys/sys/firmware.h > 1.4 +1 -1 src/sys/tools/fw_stub.awk