See my comments below
-----Original Message-----
From: Hefty, Sean [mailto:[email protected]]
Sent: Thursday, December 02, 2010 6:22 PM
To: Uri Habusha; [email protected]; Smith, Stan; Fab Tillier
Subject: RE: Add Assert to debug BS while deleting WDF device
copying your patch inline for comments. See '>>' below:
Index: winmad/kernel/wm_driver.c
===================================================================
--- winmad/kernel/wm_driver.c (revision 3009)
+++ winmad/kernel/wm_driver.c (working copy)
@@ -41,7 +41,7 @@
WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(WM_IB_DEVICE, WmIbDeviceGetContext)
WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(WM_PROVIDER, WmProviderGetContext)
-WDFDEVICE ControlDevice;
+WDFDEVICE ControlDevice = NULL;
>> Globals are initialized to 0 by default. This assignment isn't needed.
[Uri Habusha] I think it's a good practice to initialize variable also global.
It also much better documented the code.
static LIST_ENTRY DevList;
static LIST_ENTRY ProvList;
static KGUARDED_MUTEX Lock;
@@ -195,11 +195,13 @@
WdfDeviceInitSetFileObjectConfig(pinit, &fileconfig, &attr);
WDF_OBJECT_ATTRIBUTES_INIT(&attr);
+ ASSERT(ControlDevice == NULL);
status = WdfDeviceCreate(&pinit, &attr, &ControlDevice);
if (!NT_SUCCESS(status)) {
goto err1;
}
+ ASSERT(ControlDevice != NULL);
status = WdfDeviceCreateSymbolicLink(ControlDevice, &symlink);
if (!NT_SUCCESS(status)) {
goto err2;
@@ -220,6 +222,7 @@
err2:
WdfObjectDelete(ControlDevice);
+ ControlDevice = NULL;
return;
err1:
WdfDeviceInitFree(pinit);
@@ -299,7 +302,8 @@
status = WdfFdoQueryForInterface(Device, &GUID_RDMA_INTERFACE_VERBS,
(PINTERFACE) &dev->VerbsInterface,
-
sizeof(dev->VerbsInterface), VerbsVersion(2, 0),
+
sizeof(dev->VerbsInterface),
+
VerbsVersion(VERBS_MAJOR_VER, VERBS_MINOR_VER),
NULL);
if (!NT_SUCCESS(status)) {
return status;
@@ -340,14 +344,12 @@
WM_REGISTRATION *reg;
LIST_ENTRY *entry;
BOOLEAN destroy;
- WDFDEVICE ctrldev;
pdev = WmIbDeviceGetContext(Device);
KeAcquireGuardedMutex(&Lock);
RemoveEntryList(&pdev->Entry);
destroy = IsListEmpty(&DevList);
- ctrldev = ControlDevice;
for (entry = ProvList.Flink; entry != &ProvList; entry = entry->Flink) {
prov = CONTAINING_RECORD(entry, WM_PROVIDER, Entry);
@@ -364,8 +366,9 @@
ExFreePoolWithTag(pdev->pPortArray, 'pimw');
}
- if (destroy) {
- WdfObjectDelete(ctrldev);
+ if (destroy && (ControlDevice != NULL)) {
+ WdfObjectDelete(ControlDevice);
+ ControlDevice = NULL;
>> This change breaks the locking that was there. We set ctrldev under lock
>> and use that rather than ControlDevice directly outside of the lock. If
>> there are multiple HCAs in a system, then we could be processing power enter
>> on one, while getting power exit for the other. The locking needs to
>> account for this, and this is why the code is structured as it is. Because
>> of this, I don't know that the assertions that you added will be true.
[Uri Habusha] It will not resolve the problem. Let's assume 2 instances of the
function is called. The first one is set the ctrldev under the lock than come
second process and set the ctrldev. Now you try to delete the object twice. If
you really think that multiple call of the function you should take the ctrldev
and then set the ControlDevice to NULL under the lock.
>> If all device power callbacks are serialized, we can simplify the code, but
>> I couldn't find any documentation that clarifies whether this is the case.
>> The same issue exists in winverbs with your patches. Can you please try
>> running only with the changes that I submitted?
- Sean
_______________________________________________
ofw mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
