This repository has been archived by the owner on Dec 17, 2017. It is now read-only.
Usb pnp fixes v4 #17
Open
Usb pnp fixes v4 #17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NO-OP changes.
This flag is set in IRP_MN_QUERY_REMOVE_DEVICE and IRP_MN_SURPRISE_REMOVAL, and then checked in PDO's IRP dispatch routines to prevent starting of any operation which can prevent device removal.
As child's parent FDO at any moment can receive surprise removal, Before being it should notify it's childs by setting their parents to NULL. So if we see PDO with parent==NULL then we can consider it as physically removed, i.e. invalid.
We should fail this request, because we're not handling IRP_MN_STOP_DEVICE for now. On systems above win2000 we'll receive this IRP ONLY when the PnP manager rebalances resources.
NO-OP changes.
On this IRP we should free recources and delete child's PDO ONLY if it isn't presented physically on the bus. If it is presented we're leaving as it was, thinking that devices on top of it already frees otained from it resources. If child PDO is presented physically, then should be already marked as remove_pending when it will receive this IRP.
First of all we should keep in account that there might be device relations below and above this FDO, so we should save previous relations coming from top object and shuld pass this IRP down to stack after adding our relations. In case of success query devcie relations must be completed in the PDO. As MSDN requires, if the upper layer provided this IRP with initialized DeviceRelation, then we should replace that relation with another one which will contain our child PDOs too. And after replacement we should free the recources allocated for previous relation structure. If there is relations coming from upper layer, we shuldn't complete this IRP with fail, because that will bring upper layer into unstabile state, it will 'think' that succesfully reported it's relations.
Here we shouldn't modify information field of IRP, because for example in case of bus relations IRP must reach the stack's PDO collecting on its way all relations. This was the one of the causes of MS's usbccgp fail. usbccgp was not able to report to PnP manager about it's child devices. There is also another issues in usbuhci, which prevents usbccgp from normal operation. [THIS PATCH SHULD NOT BE MERGED WITH TRUNK UNTIL UHCI BUGS ARE NOT FIXED]
The status block of an IRP must be initialized before calling driver.
There is many cases where we should forward IRP instead of complete it.
I don't see the reason for allocation from NonPagedPool, also the information field writing was wrong.
In case of FDO it should pass IRP down.
In some cases our driver was changing IRP status in places where it shouldn't.
There were code dublication on copy device interface, also after copying interface, we should call InterfaceReference() routine of interface, to prevent interface provider deleiton. I have moved RtlCopyMemory() and InterfaceReference() to the end of this function to siplify cleanup part, because otherwise we should call InterfaceDereference() in cleanup part. The changing of interface context seems wrong for me, let me give just one example. When we are changing only buscontext, then when upper layer calls interfaceDereference(), which is "thinking" that works with it's buscontext, and tries to work with it using it's buscontext pointer. So changing of buscontext is completely wrong.
Added InterfaceReference() calls before passing interface to upper layer.
Done fixes in cleanup also refactored code to be more readable and error safe.
FDO_QueryInterface() is unuseful beacuse we have more generic QueryInterface() function which is able to replace all functionality which provides FDO_QueryInterface(). Also we do not need to have two "USB_BUS_INTERFACE_USBDI_V2" fields in "HUB_DEVICE_EXTENSION". FDO_QueryInterface() was added in svn-rev 55983, and on review of that commit I have found some copypasta issues and also tricks which was not documented and marked by me as wrong. For example changing if buscontext which was fixed in previous commits of this commit set.
On IRP_MN_QUERY_REMOVE_DEVICE we are freeing interface obtained from bottom according MSDN we should check interfaces provided to top, but here we are not checking. All checking will be performed in roothub driver's IRP_MN_QUERY_REMOVE_DEVICE handler. This will make problems when buggy driver is loaded on top of us. But we decided to keep source simpler, because in any case buggy driver will prevent removing of whole stack.
As FDO's child list is getting changed not from PnP dispatcher but from DeviceStatusChangeThread(), we need to sychronize access for it.
Added PDO/FDO PnP state tracking, which is done according MSDN's "State Transitions for PnP Devices" topic.
Added PDO/FDO remove synchronization to prevent device removal while another IRP is in process. As guidelines used ch6 of Walter Oney's "Programming the Microsoft Windows Driver Model 2ed"
Added remove handler for usbhub FDO. When FDO is being removed, it should delete all it's child PDOs. Because when PnP sends removes to child devices they are still presented on the bus, so their handler will not delete PDO.
Refactored power dispatcher, added remove sychronization to avoid early remove or call on removed device.
Added USBHUB_DispatchSystemControl stub
As InternalDeviceControl can be called in DPC, we should avoid scanning of parent's child list because it uses guarded mutex for synchronization. So here we just adding new checking for safety and removing isValidPDO() call.
This commit fixes issue https://jira.reactos.org/browse/CORE-11538. There were mistakes in buffer manipulation loops.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rebased and done changes to bring up on ROS. Also fixed some style issues.