On Saturday 09 February 2008, Jean-Christophe Dubois wrote: > On Saturday 09 February 2008 10:51:06 David Brownell wrote: > > A less intrusive fix would make rndis_host.c set dev->maxpacket before > > using it, and after calling usbnet_generic_cdc_bind(). That would > > be a one-line fix, affecting only the relevant driver ... much safer! > > > > The problem I have with how you approached this is that you're changing > > core code, and in a way that suggests all the minidriver bind() code > > should be changed (to rely on this having been done already). Now, > > there are a fair number of devices that will "obviously" work fine > > that way. But for ones that split their data and control interfaces, > > it's not quite obvious. Which means all true CDC devices ... and even > > the real RNDIS devices, as opposed to the ActiveSync version. > > > > So I'd rather see a one-line fix to the RNDIS host code, instead of > > this more elaborate approach. Could you give that a try? > > Well, I could certainly give it a try. However I'd like to point out a few > thing. > > 1) in the actual usbnet code, dev->out and dev->in are never set in case of a > bind() driver. As a result the computed dev->maxpacket is suspicious.
When there's a bind(), setting up endpoints is one of its responsibilities. Typically they call usbnet_get_endpoints(), or have it be called. > 2) I could set the dev->maxpacket in the rndis_host driver but you should > notice that whatever I set it to, it will be overwritten by usbnet afterward > (and based on a bogus dev->out). I can't agree about dev->out, since that gets set indirectly: usbnet_generic_cdc_bind() calls usbnet_get_endpoints(). It would be overwritten using the same value. (Or, it could use its own variable and ignore dev->maxpacket -- which as you noted, isn't yet valid. So it shouldn't have been using that yet...) > 3) doing the change in the bind() function (without touching usbnet) would > mean that any bind() driver (I don't know if there are some other ones) Monsieur Grep reports quite a number of them. :) > will have to do the same. Is this a contract that we can force on any bind() > driver? My point was that *ONLY* rndis_host uses dev->maxpacket before it's set. Since *ONLY* that bind() has such a bug, a broad change isn't necessary. > In the end it seems to me that dev->in, dev->out and dev->maxpacket are > generic enough values to be computed in one place whatever driver (bind() or > not) attach to usbnet. I'd agree as a simplification ... except that on the flip side: if it's changed to work that way, all the users of bind() should be updated to follow those new semantics. What you posted is either much too big, or is incomplete: almost all the minidrivers would still be using the original semantics. I'd rather have the consistency then be a smidgeon smaller. Smaller *and* more consistent is of course fine -- but even so, I'd rather see a small patch that's quickly mergeable, than a big one I'd have to take a long time reviewing (and probably need to make time to test, too). > bind() could then recompute them if need be Or in the case of CDC-derived devices, compute them for the first time... > but is not > forced to. In any case it seems to be the usbnet job to set these values (as > it is doing it now but late) and computing them before bind() seems cleaner > to me. The only case that's "too late" is one value -- and only for rndis_host. Plus, as I noted, in the case of CDC-derived devices it *can't* compute those values before bind(). Normal RNDIS devices work that way, for example -- but the ActiveSync ones (like yours) don't. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
