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>