On Mon, Feb 14, 2011 at 09:07:33PM -0800, Ping Cheng wrote: > On Mon, Feb 14, 2011 at 6:03 PM, Peter Hutterer > <peter.hutte...@who-t.net>wrote: > > > On Fri, Feb 11, 2011 at 04:54:48PM -0800, Ping Cheng wrote: > > > DBG(2, priv, "BEGIN dev=%p priv=%p " > > > @@ -900,9 +906,14 @@ static int wcmDevProc(DeviceIntPtr pWcm, int what) > > > case DEVICE_INIT: > > > if (!wcmDevInit(pWcm)) > > > goto out; > > > + > > > + /* All tools on the same port are initialized one > > by one > > > + * here before any of them are enabled */ > > > + common->wcmNumTools++; > > > > As I've said in other emails, this is not the case for the hotplugging case > > which is the most common case we face on modern desktop distributions. > > > > Is there a specific way for me to tell if I am testing on a "modern desktop > distributions" or not, such as the version of X server, XInput, or > something? My testing was based on Fedora 14, up to date. It showed > "DEVICE_INIT for all devices, then DEVICE_ON for all devices." for > hot-plugging. Which distro should I use to test the second path? > > Don't get me wrong, please. I am not question you. I need to understand it > so I can do it right.
did you have a leftover xorg.conf? I'm testing on F14 (with upstream X server but that doesn't matter much). We call NewInputDeviceRequest during PreInit of the first device and that in turn calls ActivateDevice + EnableDevice before returning. if you put a printf statement in DEVICE_ENABLE and print wcmNumTools, it just goes up one-by-one instead of printing 4 each time. the server input ABI has changed, but not in this regard and this should be the same result down to server 1.7 > > quote: > > "init in the config is quite different to init during hotplugging. > > the server will call, PreInit for all devices, DEVICE_INIT for all devices, > > then DEVICE_ON for all devices. > > > > For hotplugging, it's PreInit first device, PreInit + DEVICE_INIT + > > DEVICE_ON for all dependent devices in order, then DEVICE_INIT and > > DEVICE_ON > > for the first device." > > > > so with this patch, whenever DEVICE_ENABLE is called, you have > > common->wcmNumTools being equal to common->fd_refs. which means it'll go > > happily past the condition in wcmDevReadInput if a device sends events > > before all devices are enabled. > > > > I need to know which system I can test with before I comment more. My > testing result doesn't tell me the above, somehow. > > > > the only reason why it doesn't crash (though the first hunk should avoid > > the > > crash anyway) > > > I do not know exactly why the patch works. As I've told you, I have been > working on this issue with a customer for sometime. We come to this point > based on intensive testing result, not theory. So, I can not say I am sure > it fixes the issue at its root cause. > > The testing result showed the following: > > without the "first hunk", the system crashes very often: once about a > hundred hot-plugging; > with the "first hunk", the system is still crashing. But a lot less: once > about 2k hot-plugging; > with the whole patch, the system has survived about 40k hot-plugging. Since > the customer feels that is good enough, they stopped the testing and called > it accepted. the problem is that you're debugging a race condition so any actual testing relies heavily on randomness. I can't even reproduce the original issue on my machine (too slow? too fast? i don't know) without intentionally halting the driver during the time window in question so I can generate an event while the driver is vulnerable. This is the same I did here and I was literally single-stepping through the driver to figure out where the events disappear. i expected it to hit the condition in hunk 1 and was suprised when it didnt. fwiw, I wrote up debugging with gdb today so others can try the same https://sourceforge.net/apps/mediawiki/linuxwacom/index.php?title=Debugging > > > is because a memset in usbDispatchEvents resets for protocol > > level 5 and proximity resets pChannel->nSamples and any event is discarded > > because of the "drop the first 2 usb events" condition. so > > commonDispatchDevice is just never called for any event during this init > > period because nSamples is just reset after every event. > > At that point I stopped debugging because whatever the right behaviour is, > > I'm pretty sure this isn't it. > > > > > break; > > > > > > case DEVICE_ON: > > > + /* common->fd_refs is updated for each tool in > > wcmDevOpen */ > > > > we need to avoid this sort of comment. It's confusing. there is no mention > > of fd_refs anywhere in wcmDevProc and it doesn't matter anyway - that's > > what > > wcmDevOpen() and wcmDevInit() are supposed to handle. > > > > We can discuss how to make a comment more clear. I am willing to learn. For > this case, my intention was: > > To point out that common->fd_refs is updated in wcmDevOpen so readers could > link common->fd_refs to common->wcmNumTools in the DEVICE_INIT above when > they see " if (common->fd_refs != common->wcmNumTools)". > > If this is still unclear to you, I'll make more attempts until it makes > sense to you so you can reword the comments to make it clear. the thing is, fd_refs doesn't matter in this context. if you look at just this hunk, there is no visible link between fd_refs and wcmNumTools. so if you say it's updated in open, I still wouldn't know why this would matter because I can't associate it with anything. if anything, it should be documented in wcmDevOpen where it is actually used. Cheers, Peter ------------------------------------------------------------------------------ The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE: Pinpoint memory and threading errors before they happen. Find and fix more than 250 security defects in the development cycle. Locate bottlenecks in serial and parallel code that limit performance. http://p.sf.net/sfu/intel-dev2devfeb _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
