[PATCH] Fix for USB ugen panics
Jay Cornwall
jay at evilrealms.net
Tue Jan 6 15:14:28 PST 2004
Hi
Attached is a patch which I believe fixes two issues found in the ugen driver,
as described in these mails:
http://lists.freebsd.org/pipermail/freebsd-current/2003-November/015065.html
http://lists.freebsd.org/pipermail/freebsd-current/2003-December/016260.html
The first is caused by an unhandled attempt to set a USB device's
configuration to number 0 (the unconfigurd state, USB_UNCONFIG_NO) and the
second by an inconsistent state left after an error occurs in one of ugen's
functions, since the introduction of devfs.
A breakdown of the patch and justification for each change is given below.
Note that I've only been able to confirm the first bug has been fixed, as the
second did not have an easily reproducable cause.
- struct ugen_endpoint *sce;
- u_int8_t niface, nendpt;
+ struct ugen_endpoint *sce, **sce_cache, ***sce_cache_arr;
+ u_int8_t niface, niface_cache, nendpt, *nendpt_cache;
Four variables are declared to facilitate the endpoint caching method
described below.
-#if defined(__FreeBSD__)
- ugen_destroy_devnodes(sc);
-#endif
This call is moved further down into the function. If any of the subsequent
function calls fail and return(err) (this can happen during normal use in
usbd_set_config_no(), for example), devfs will be left in an inconsistent
state which will panic on the next attempt to ugen_destroy_devnodes.
- /* Avoid setting the current value. */
- if (usbd_get_config_descriptor(dev)->bConfigurationValue != configno) {
- err = usbd_set_config_no(dev, configno, 1);
- if (err)
- return (err);
- }
This code is moved further down into the function. We must cache the current
set of endpoint descriptors before the call to usbd_set_config_no(), otherwise
if the call succeeds we will be unable to clear the old set of endpoint
descriptors. This is needed as a consequence of moving the
ugen_destroy_devnodes() call below here.
- memset(sc->sc_endpoints, 0, sizeof sc->sc_endpoints);
This is an unnecessary call now, as we clear each endpoint descriptor
explicitly later.
+ [Large set of endpoint caching code]
This allocates and stores in a structure the set of endpoint descriptors so
they can be freed after the ugen_destroy_devnodes call. I wish there were a
cleaner way to achieve this, but the separation of devfs from internal usbd_*
code makes this necessary.
- return (err);
+ panic("ugen_set_config: can't obtain interface handle");
Leaving the function at this point will result in an inconsistent state.
Better to panic now.
- return (err);
+ panic("ugen_set_config: endpoint count failed");
Ditto.
+ if (configno != USB_UNCONFIG_NO)
+ {
Continuing at this point with a NULL dev->cdesc will result in a panic in
usbd_interface_count. The endpoint regeneration code is skipped instead.
All the changes in the patch should be attributable to one of the explanations
given above.
I welcome any comments on the patch, particularly on structure and the use of
M_TEMP (I couldn't find documentation on the correct form of malloc for
allocating temporary storage). :)
--
Cheers,
Jay
http://www.evilrealms.net/ - Systems Administrator & Developer
http://www.imperial.ac.uk/ - 3rd year CS student
More information about the freebsd-current
mailing list