Sebastien Roy wrote: > Nicolas, > > On Mon, 2009-11-23 at 13:41 -0800, Nicolas Droux wrote: >> On Nov 20, 2009, at 11:25 AM, Nicolas Droux wrote: >> >>> Seb, >>> >>> My intent is to include this entry point as part of the GLDv3 APIs being >>> committed. It is documented it in the mac(9F) man page draft [1], >>> but I did not list it in the overview. I will update the spec to list it >>> there as well. >> The updated spec is now online in the case directory (see proposal.txt) > > Excellently written documentation, +1 on the case. I only have a couple > of requests for documentation clarification and minor nits:
Seb, Thanks a lot for the thorough review. The updated specs will soon be posted in the case directory. Until then you can find them here: http://zia.sfbay/~nd99603/gldv3_public_psarc > > In proposal.txt: > > * "A GLDv3 device driver calls mac_register(9F) to register a new > instance with the framework. mac_register(9F) is typically called from > the attach(9E) entry point implemented by the device driver." > > In fact, it's not only typical, but for physical devices, mac_register() > _must_ be called from attach(9E). Perhaps that needs to be clearly > stated in the documentation, because drivers for physical devices that > call mac_register() from anywhere but attach() won't work correctly > (softmac_hold_device() will break). OK, fixed. > > > * Somewhere in the spec and/or documentation, there needs to be a > discussion about locking. There is a mention in the mac(9F) man page > that drivers should avoid holding locks when calling mac_rx(). Are > there additional locking requirements while issuing other upcalls, > and/or while handling callbacks? That's the only minimum requirement. Note that I also added a CONTEXT section to the man page describing device drivers entry points. > > > * In mac.c, there is the following comment in the "MAC driver rules" > section: > > * R17 Similarly mi_stop is another synchronization point and the driver must > * ensure that all upcalls are done and there won't be any future upcall > * before returning from mi_stop. > > This is likely an important bit of information that driver developers > need to know about. More generally, it would be good to go through > those rules and ensure that the important parts end up reflected in > developer documentation. OK, good suggestion. I have added the following info to the specs: - Note to mc_stop() in mac-9e.txt from R17 - Added note to mac-9f.txt for R15. - Added note to mac-9e.txt for R18. > > > * Regarding mac_hcksum_get(9F): Where are the definitions of such terms > as "full checksum" and "partial checksum" that are used to define what > the MAC_HCK_* flags represent? I have added a definition of full vs partial checksum to the capabilities section of mac-9e.txt. I have then added a cross-reference to that section from mac_hcksum_get-9f.txt, see first paragraph of the description section. > > * Regarding mac_hcksum_get(9F), are the MAC_HCK_* flags applicable to > both IPv4 and IPv6 (except for MAC_HCK_IPV4_HDRCKSUM_OK, which is > obvious)? Yes, the driver can expose separate capabilities for IPv4 and IPv6 full checksum offload, but the per-packet flags are the same in both cases (except MAC_HCK_IPV4_HDRCKSUM_OK of course). I added a note to mac_hcksum_get-9f.txt to capture this. > > > * Regarding LSO: Are drivers expected to be able to handle transmitted > packets with IP options (such as TX labels), or does the framework > guarantee that the IP header does not contain options when LSO is > enabled? It's the latter, LSO should be in general disabled if an IP option is present, since it's not possible to safely assume that all IP options available today and tomorrow will be handled by the hardware. If we need to relax that limitation in the future, we can use the lso_flags field of the mac_capab_lso_t structure to convey more information on the hardware support with respect to IP options. I have added a corresponding note to mac_lso_get-9f.txt to make this clear. > > > * Typos in mac_prop_info_set_perm-9f.txt: > - The man page header refers to mac_lso_get(9F). > - "mac_prop_info_set_perm() specifies the property of the property." > s/specifies the property/specifies the permissions associated with/ (?). Fixed. Thanks, Nicolas. > > -Seb > >
