Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 15 Feb 2007 09:43:42 -0800
From:      Luigi Rizzo <rizzo@icir.org>
To:        current@freebsd.org
Subject:   one last firmware(9) issue
Message-ID:  <20070215094342.B91479@xorpc.icir.org>

next in thread | raw e-mail | index | archive | help
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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070215094342.B91479>