[ofw] [RFC] [PATCH 4/4] winverbs.sys: partial driver implementation
Sean Hefty
sean.hefty at intel.com
Wed Apr 23 15:17:43 PDT 2008
>>Index: wv_device.c
>>===================================================================
>>--- wv_device.c (revision 0)
>>+++ wv_device.c (revision 0)
>>@@ -0,0 +1,580 @@
>>+void WvDeviceGet(WV_DEVICE *pDevice)
>>+{
>>+ InterlockedIncrement(&pDevice->Ref);
>>+}
>>+
>>+void WvDevicePut(WV_DEVICE *pDevice)
>>+{
>>+ if (InterlockedDecrement(&pDevice->Ref) == 0) {
>>+ KeSetEvent(&pDevice->Event, 0, FALSE);
>>+ }
>>+}
>
>Why not use Ref/Deref instead of Get/Put since that's what the functions do?
To avoid confusion with 'dereferencing' a pointer.
>>+WV_DEVICE *WvDeviceAcquire(WV_PROVIDER *pProvider, UINT64 Id)
>>+{
>>+ WV_DEVICE *dev;
>>+
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ dev = IndexListAt(&pProvider->DevIndex, (SIZE_T) Id);
>>+ if (dev != NULL) {
>>+ WvProviderRemoveDisable(pProvider);
>>+ WvDeviceGet(dev);
>
>You need to handle concurrent removal. WvProviderRemoveDisable could release
>the mutex (due to a remove event being processed) for the caller (say a PD
>allocation), while at the same time a different thread tries to destroy the
>device. If the second thread gets the mutex (after the remove event is
>processed) before the first, the device would be removed and freed, then
>dereferenced by the first thread when it gets a chance to run.
>
>The simplest solution is to call WvProviderRemoveDisable before making the
>IndexList lookup, and calling WvProviderRemoveEnable if dev == NULL. Moving
>WvDeviceGet up before RemoveDisable means you need to handle a NULL
>hVerbsDevice in a bunch of places.
gurgle... yes - there's a race here. Good catch.
I think the code needs to check that the HCA is still there before calling any
verb routines anyway. Let me think of the best way to handle this. I may want
to keep the WV_RDMA_DEVICE around, but set all of its pVerbs functions to empty
routines. Or check hVerbsWidget handles everywhere. Or check pVerbs
everywhere... so many choices!
>>+ }
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>>+
>>+ return dev;
>>+}
>
>Snip...
>
>>+void WvDeviceRemoveHandler(WV_DEVICE *pDevice)
>>+{
>>+ LIST_ENTRY *entry;
>>+ WV_PROTECTION_DOMAIN *pd;
>>+
>>+ for (entry = pDevice->PdList.Flink; entry != &pDevice->PdList;
>>+ entry = entry->Flink) {
>>+ pd = CONTAINING_RECORD(entry, WV_PROTECTION_DOMAIN, Entry);
>>+ WvPdRemoveHandler(pd);
>>+ }
>>+
>>+ pDevice->pVerbs->um_close_ca(pDevice->pDevice->hDevice,
>>+ pDevice-
>>hVerbsDevice);
>
>Need to set the handle to NULL here, no? If nothing else, to force a NULL
>pointer dereference if something went wrong.
>
>>+ WvRdmaDevicePut(pDevice->pDevice);
>>+ pDevice->pDevice = NULL;
I set pDevice to NULL, which is the check that gets used. This falls into the
problem mentioned above.
>>+}
>
>Snip
>
>>+void WvDeviceQuery(WV_PROVIDER *pProvider, WDFREQUEST Request)
>>+{
>>+ UINT64 *id;
>>+ WV_IO_DEVICE_ATTRIBUTES *attr;
>>+ WV_DEVICE *dev;
>>+ ib_ca_attr_t *ca_attr;
>>+ NTSTATUS status;
>>+ UINT32 outlen = 0;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request, sizeof(UINT64), &id,
>NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+ status = WdfRequestRetrieveOutputBuffer(Request,
>sizeof(WV_IO_DEVICE_ATTRIBUTES),
>>+
>&attr, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+
>>+ dev = WvDeviceAcquire(pProvider, *id);
>>+ if (dev == NULL) {
>>+ status = STATUS_NO_SUCH_DEVICE;
>>+ goto complete;
>>+ }
>>+
>>+ ca_attr = WvQueryCaAttributes(dev);
>
>You could call WvDeviceRelease here and eliminate the 'release' label.
Why, yes, yes I could. I'll update.
>
>>+ if (ca_attr == NULL) {
>>+ status = STATUS_NO_MEMORY;
>>+ goto release;
>>+ }
>>+
>>+ WvConvertDevAttr(attr, ca_attr);
>>+ outlen = sizeof(WV_IO_DEVICE_ATTRIBUTES);
>>+ ExFreePool(ca_attr);
>>+
>>+release:
>>+ WvDeviceRelease(dev);
>>+complete:
>>+ WdfRequestCompleteWithInformation(Request, status, outlen);
>>+}
>>+
>>+void WvDevicePortQuery(WV_PROVIDER *pProvider, WDFREQUEST Request)
>>+{
>>+ WV_IO_DEVICE_PORT_QUERY *query;
>>+ WV_IO_PORT_ATTRIBUTES *attr;
>>+ WV_DEVICE *dev;
>>+ ib_ca_attr_t *ca_attr;
>>+ NTSTATUS status;
>>+ UINT32 outlen = 0;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request,
>sizeof(WV_IO_DEVICE_PORT_QUERY),
>>+
>&query, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+ status = WdfRequestRetrieveOutputBuffer(Request,
>sizeof(WV_IO_PORT_ATTRIBUTES),
>>+
>&attr, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+
>>+ dev = WvDeviceAcquire(pProvider, query->Id);
>>+ if (dev == NULL) {
>>+ status = STATUS_NO_SUCH_DEVICE;
>>+ goto complete;
>>+ }
>>+
>>+ ca_attr = WvQueryCaAttributes(dev);
>
>Same here, you can release the device now as it's no longer needed.
>
>>+ if (ca_attr == NULL) {
>>+ status = STATUS_NO_MEMORY;
>>+ goto release;
>>+ }
>>+
>>+ if (query->PortNumber >= ca_attr->num_ports) {
>>+ status = STATUS_INVALID_PORT_HANDLE;
>>+ goto free;
>>+ }
>>+
>>+ WvConvertPortAttr(attr, &ca_attr->p_port_attr[query->PortNumber]);
>>+ outlen = sizeof(WV_IO_PORT_ATTRIBUTES);
>>+
>>+free:
>>+ ExFreePool(ca_attr);
>>+release:
>>+ WvDeviceRelease(dev);
>>+complete:
>>+ WdfRequestCompleteWithInformation(Request, status, outlen);
>>+}
>>+
>>+void WvDeviceGidQuery(WV_PROVIDER *pProvider, WDFREQUEST Request)
>>+{
>>+ WV_IO_DEVICE_PORT_QUERY *query;
>>+ WV_IO_GID *gid;
>>+ WV_DEVICE *dev;
>>+ ib_ca_attr_t *ca_attr;
>>+ ib_port_attr_t *port_attr;
>>+ NTSTATUS status;
>>+ UINT32 i, size, outlen = 0;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request,
>sizeof(WV_IO_DEVICE_PORT_QUERY),
>>+
>&query, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+ status = WdfRequestRetrieveOutputBuffer(Request, sizeof(WV_IO_GID),
>&gid, &size);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+
>>+ dev = WvDeviceAcquire(pProvider, query->Id);
>>+ if (dev == NULL) {
>>+ status = STATUS_NO_SUCH_DEVICE;
>>+ goto complete;
>>+ }
>>+
>>+ ca_attr = WvQueryCaAttributes(dev);
>
>Same here...
>
>>+ if (ca_attr == NULL) {
>>+ status = STATUS_NO_MEMORY;
>>+ goto release;
>>+ }
>>+
>>+ if (query->PortNumber >= ca_attr->num_ports) {
>>+ status = STATUS_INVALID_PORT_HANDLE;
>>+ goto free;
>>+ }
>>+
>>+ size /= sizeof(WV_IO_GID);
>>+ port_attr = &ca_attr->p_port_attr[query->PortNumber];
>>+ for (i = 0; i < size && i < port_attr->num_gids; i++) {
>>+ RtlCopyMemory(&gid[i], &port_attr->p_gid_table[i],
>sizeof(WV_IO_GID));
>>+ }
>>+
>>+ outlen = i * sizeof(WV_IO_GID);
>>+ if (i < port_attr->num_gids) {
>>+ status = STATUS_MORE_ENTRIES;
>>+ }
>>+
>>+free:
>>+ ExFreePool(ca_attr);
>>+release:
>>+ WvDeviceRelease(dev);
>>+complete:
>>+ WdfRequestCompleteWithInformation(Request, status, outlen);
>>+}
>>+
>>+void WvDevicePkeyQuery(WV_PROVIDER *pProvider, WDFREQUEST Request)
>>+{
>>+ WV_IO_DEVICE_PORT_QUERY *query;
>>+ UINT16 *pkey;
>>+ WV_DEVICE *dev;
>>+ ib_ca_attr_t *ca_attr;
>>+ ib_port_attr_t *port_attr;
>>+ NTSTATUS status;
>>+ UINT32 i, size, outlen = 0;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request,
>sizeof(WV_IO_DEVICE_PORT_QUERY),
>>+
>&query, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+ status = WdfRequestRetrieveOutputBuffer(Request, sizeof(UINT16),
>&pkey, &size);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+
>>+ dev = WvDeviceAcquire(pProvider, query->Id);
>>+ if (dev == NULL) {
>>+ status = STATUS_NO_SUCH_DEVICE;
>>+ goto complete;
>>+ }
>>+
>>+ ca_attr = WvQueryCaAttributes(dev);
>
>Yep, you guessed it, same here too.
>
>>+ if (ca_attr == NULL) {
>>+ status = STATUS_NO_MEMORY;
>>+ goto release;
>>+ }
>>+
>>+ if (query->PortNumber >= ca_attr->num_ports) {
>>+ status = STATUS_INVALID_PORT_HANDLE;
>>+ goto free;
>>+ }
>>+
>>+ size /= sizeof(UINT16);
>>+ port_attr = &ca_attr->p_port_attr[query->PortNumber];
>>+ for (i = 0; i < size && i < port_attr->num_pkeys; i++) {
>>+ pkey[i] = port_attr->p_pkey_table[i];
>>+ }
>>+
>>+ outlen = i * sizeof(UINT16);
>>+ if (i < port_attr->num_pkeys) {
>>+ status = STATUS_MORE_ENTRIES;
>>+ }
>>+
>>+free:
>>+ ExFreePool(ca_attr);
>>+release:
>>+ WvDeviceRelease(dev);
>>+complete:
>>+ WdfRequestCompleteWithInformation(Request, status, outlen);
>>+}
>
>Snip
>
>>+void WvPdDeallocate(WV_PROVIDER *pProvider, WDFREQUEST Request)
>>+{
>>+ WV_PROTECTION_DOMAIN *pd;
>>+ UINT64 *id;
>>+ NTSTATUS status;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request, sizeof(UINT64), &id,
>NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto out;
>>+ }
>>+
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ WvProviderRemoveDisable(pProvider);
>>+ pd = IndexListRemove(&pProvider->PdIndex, (SIZE_T) id);
>>+ if (pd == NULL) {
>>+ status = STATUS_NOT_FOUND;
>>+ } else if (pd->Ref > 1) {
>>+ status = STATUS_ACCESS_DENIED;
>
>What happens to the ID? Shouldn't you restore it? I would think you'd need to
>use IndexListAt for the check, and call IndexListRemove if it's found and the
>ref count is 1.
Er... yes, the pd should be left in (or returned to) the list.
>
>>+ } else {
>>+ RemoveEntryList(&pd->Entry);
>>+ status = STATUS_SUCCESS;
>>+ }
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>>+
>>+ if (NT_SUCCESS(status)) {
>>+ WvPdDestroy(pd);
>>+ }
>>+ WvProviderRemoveEnable(pProvider);
>>+out:
>>+ WdfRequestComplete(Request, status);
>>+}
>>Index: wv_driver.c
>>===================================================================
>>--- wv_driver.c (revision 1035)
>>+++ wv_driver.c (working copy)
>>@@ -30,9 +30,10 @@
>> #include <ntddk.h>
>> #include <wdf.h>
>> #include <wdmsec.h>
>>-#include <winerror.h>
>>+#include <ntstatus.h>
>> #include <initguid.h>
>>
>>+#include <rdma/verbs.h>
>> #include "wv_driver.h"
>> #include "wv_ioctl.h"
>> #include "wv_provider.h"
>>@@ -44,20 +45,122 @@
>> #include "wv_listen.h"
>> #include "wv_ep.h"
>>
>>-WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(WV_RDMA_DEVICE, WvGetRdmaDevice)
>>-WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(WV_PROVIDER, WvGetProvider)
>>+WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(WV_RDMA_DEVICE, WvRdmaDeviceGetContext)
>>+WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(WV_PROVIDER, WvProviderGetContext)
>>
>>-static WDFDEVICE ControlDevice;
>>-KGUARDED_MUTEX DevLock;
>>-LIST_ENTRY DevList;
>>+static WDFDEVICE ControlDevice;
>>+static LIST_ENTRY DevList;
>>+static LIST_ENTRY ProvList;
>>+static KGUARDED_MUTEX Lock;
>>
>>-static EVT_WDF_DRIVER_DEVICE_ADD WvDeviceAdd;
>>-static EVT_WDF_OBJECT_CONTEXT_CLEANUP WvDeviceCleanup;
>>+static EVT_WDF_DRIVER_DEVICE_ADD WvRdmaDeviceAdd;
>>+static EVT_WDF_OBJECT_CONTEXT_CLEANUP WvRdmaDeviceCleanup;
>> static EVT_WDF_IO_QUEUE_IO_DEVICE_CONTROL WvIoDeviceControl;
>> static EVT_WDF_DEVICE_FILE_CREATE WvFileCreate;
>> static EVT_WDF_FILE_CLEANUP WvFileCleanup;
>> static EVT_WDF_FILE_CLOSE WvFileClose;
>>
>>+static WV_RDMA_DEVICE *WvRdmaDeviceFind(UINT64 Guid)
>>+{
>>+ WV_RDMA_DEVICE *dev;
>>+ LIST_ENTRY *entry;
>>+
>>+ for (entry = DevList.Flink; entry != &DevList; entry = entry->Flink) {
>>+ dev = CONTAINING_RECORD(entry, WV_RDMA_DEVICE, Entry);
>>+ if (dev->Interface.Verbs.guid == Guid) {
>
>I would suggest taking the reference where ever you return a device, rather
>than letting the caller do it.
>
>>+ return dev;
>>+ }
>>+ }
>>+ return NULL;
>>+}
>>+
>>+WV_RDMA_DEVICE *WvRdmaDeviceGet(UINT64 Guid)
>>+{
>>+ WV_RDMA_DEVICE *dev;
>>+
>>+ KeAcquireGuardedMutex(&Lock);
>>+ dev = WvRdmaDeviceFind(Guid);
>
>Do you ever call this from anywhere else? Why not just merge the two?
This is the only place it's called from. I started to merge the calls, but got
sidetracked...
>>+ if (dev != NULL) {
>>+ InterlockedIncrement(&dev->Ref);
>>+ }
>>+ KeReleaseGuardedMutex(&Lock);
>>+ return dev;
>>+}
>>+
>>+void WvRdmaDevicePut(WV_RDMA_DEVICE *pDevice)
>>+{
>>+ if (InterlockedDecrement(&pDevice->Ref) == 0) {
>>+ KeSetEvent(&pDevice->Event, 0, FALSE);
>>+ }
>>+}
>
>Snip
>
>>+static void WvLibraryQuery(WDFREQUEST Request)
>>+{
>>+ UINT64 *guid;
>>+ char *name;
>>+ size_t len = 0;
>>+ WV_RDMA_DEVICE *dev;
>>+ NTSTATUS status;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request, sizeof(UINT64), &guid,
>NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto out;
>>+ }
>>+ status = WdfRequestRetrieveOutputBuffer(Request,
>>+
>>sizeof(dev->Interface.Verbs.libname),
>>+
>&name, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto out;
>>+ }
>>+
>>+ dev = WvRdmaDeviceGet(*guid);
>>+ if (dev == NULL) {
>>+ status = STATUS_NO_SUCH_DEVICE;
>>+ goto out;
>>+ }
>>+
>>+ len = sizeof(dev->Interface.Verbs.libname);
>
>Why the local var? Just to save you typing sizeof(dev-
>>Interface.Verbs.libname)? If so, why not use it above in the call to
>WdfRequestRetrieveOutputBuffer too?
It sets the length of the output buffer of WdfRequestCompleteWithInformation.
Until we reach this point in the function, the output length should be 0.
>>+ RtlCopyMemory(name, dev->Interface.Verbs.libname, len);
>>+ WvRdmaDevicePut(dev);
>>+
>>+out:
>>+ WdfRequestCompleteWithInformation(Request, status, len);
>>+}
>>+
>> static VOID WvIoDeviceControl(WDFQUEUE Queue, WDFREQUEST Request,
>> size_t OutLen,
>size_t InLen, ULONG IoControlCode)
>> {
>
>Snip
>
>>@@ -153,10 +279,12 @@
>>
>> static VOID WvFileClose(WDFFILEOBJECT FileObject)
>> {
>>- WV_PROVIDER *prov;
>>+ WV_PROVIDER *prov = WvProviderGetContext(FileObject);
>>
>>- prov = WvGetProvider(FileObject);
>>- WvProviderDestroy(prov);
>>+ KeAcquireGuardedMutex(&Lock);
>>+ RemoveEntryList(&prov->Entry);
>>+ KeReleaseGuardedMutex(&Lock);
>>+ WvProviderCleanup(prov);
>
>Cleanup should probably be done in the EvtFileCleanup handler. EvtFileCleanup
>gets called when the app closes the file handle. EvtFileClose gets called when
>the last reference on the file object goes away (including internal kernel
>refrences taken for outstanding I/O requests). You want to deregister memory
>and so forth from the Cleanup handler.
Ok - I didn't realize that was the difference between those handlers.
>> }
>>
>> static VOID WvCreateControlDevice(WDFDRIVER Driver)
>>@@ -215,7 +343,7 @@
>> WdfDeviceInitFree(pinit);
>> }
>>
>>-static NTSTATUS WvDeviceAdd(WDFDRIVER Driver, PWDFDEVICE_INIT DeviceInit)
>>+static NTSTATUS WvRdmaDeviceAdd(WDFDRIVER Driver, PWDFDEVICE_INIT DeviceInit)
>> {
>> WDF_OBJECT_ATTRIBUTES attr;
>> WDFDEVICE dev;
>>@@ -226,19 +354,30 @@
>> WdfFdoInitSetFilter(DeviceInit);
>>
>> WDF_OBJECT_ATTRIBUTES_INIT_CONTEXT_TYPE(&attr, WV_RDMA_DEVICE);
>>- attr.EvtCleanupCallback = WvDeviceCleanup;
>>+ attr.EvtCleanupCallback = WvRdmaDeviceCleanup;
>> status = WdfDeviceCreate(&DeviceInit, &attr, &dev);
>> if (!NT_SUCCESS(status)) {
>> return status;
>> }
>>
>>- pdev = WvGetRdmaDevice(dev);
>>- pdev->Guid = 0x1234567890ABCDEF;
>>+ pdev = WvRdmaDeviceGetContext(dev);
>>+ RtlZeroMemory(pdev, sizeof *pdev);
>>+ pdev->Ref = 1;
>>+ KeInitializeEvent(&pdev->Event, NotificationEvent, FALSE);
>>
>>- KeAcquireGuardedMutex(&DevLock);
>>+ status = WdfFdoQueryForInterface(dev, &GUID_RDMA_INTERFACE_VERBS,
>>+
>(PINTERFACE) &pdev->Interface,
>>+
>sizeof(pdev->Interface), VerbsVersion(2, 0),
>>+
>NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ return status;
>>+ }
>>+ pdev->hDevice = pdev->Interface.Verbs.p_hca_dev;
>
>The p_hca_dev parameter of the verbs interface is a pointer to the HCA driver's
>device object for that HCA instance.
The mthca changes set this to a device specific context -- specifically
&p_ext->hca.hob -- when replying to a query interface routine. Basically, we
can eventually do away with the CI open/close ca calls, and the HCA driver
should not need to expose its DEVICE_OBJECT.
>>+
>>+ KeAcquireGuardedMutex(&Lock);
>> create = IsListEmpty(&DevList);
>> InsertHeadList(&DevList, &pdev->Entry);
>>- KeReleaseGuardedMutex(&DevLock);
>>+ KeReleaseGuardedMutex(&Lock);
>>
>> if (create) {
>> WvCreateControlDevice(Driver);
>
>Snip
>
>>Index: wv_driver.h
>>===================================================================
>>--- wv_driver.h (revision 1035)
>>+++ wv_driver.h (working copy)
>>@@ -34,19 +34,33 @@
>>
>> #include <ntddk.h>
>> #include <wdm.h>
>>+
>> #include <iba\ib_types.h>
>> #include <iba\ib_ci.h>
>>+#include <rdma\verbs.h>
>>
>> typedef struct _WV_RDMA_DEVICE
>> {
>>- UINT64 Guid;
>>- LIST_ENTRY Entry;
>>- ci_interface_t Verbs;
>>- ib_ca_handle_t hVerbsDevice;
>>+ LIST_ENTRY Entry;
>>+ LONG Ref;
>>+ KEVENT Event;
>>+ ib_ca_handle_t hDevice;
>
>This is not a ib_ca_handle_t, it's a DEVICE_OBJECT*.
>
>>+ RDMA_INTERFACE_VERBS Interface;
>>
>> } WV_RDMA_DEVICE;
>>
>>-extern KGUARDED_MUTEX DevLock;
>>-extern LIST_ENTRY DevList;
>>+WV_RDMA_DEVICE *WvRdmaDeviceGet(UINT64 Guid);
>>+void WvRdmaDevicePut(WV_RDMA_DEVICE *pDevice);
>>
>>+static inline void WvInitVerbsData(ci_umv_buf_t *pVerbsData, UINT32 Command,
>>+ UINT32
>InputLength, UINT32 OutputLength,
>>+ void
>*pBuffer)
>>+{
>>+ pVerbsData->command = Command;
>>+ pVerbsData->input_size = InputLength;
>>+ pVerbsData->output_size = OutputLength;
>>+ pVerbsData->p_inout_buf = pBuffer;
>>+ pVerbsData->status = 0;
>>+}
>>+
>> #endif // _WV_DRIVER_H_
>>Index: wv_pd.c
>>===================================================================
>>--- wv_pd.c (revision 0)
>>+++ wv_pd.c (revision 0)
>>@@ -0,0 +1,249 @@
>>+/*
>>+ * Copyright (c) 2008 Intel Corporation. All rights reserved.
>>+ *
>>+ * This software is available to you under the OpenIB.org BSD license
>>+ * below:
>>+ *
>>+ * Redistribution and use in source and binary forms, with or
>>+ * without modification, are permitted provided that the following
>>+ * conditions are met:
>>+ *
>>+ * - Redistributions of source code must retain the above
>>+ * copyright notice, this list of conditions and the following
>>+ * disclaimer.
>>+ *
>>+ * - Redistributions in binary form must reproduce the above
>>+ * copyright notice, this list of conditions and the following
>>+ * disclaimer in the documentation and/or other materials
>>+ * provided with the distribution.
>>+ *
>>+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AWV
>>+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>>+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>>+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>>+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>>+ * SOFTWARE.
>>+ */
>>+
>>+#include "wv_pd.h"
>>+#include "wv_ioctl.h"
>>+
>>+void WvPdGet(WV_PROTECTION_DOMAIN *pPd)
>>+{
>>+ InterlockedIncrement(&pPd->Ref);
>>+}
>>+
>>+void WvPdPut(WV_PROTECTION_DOMAIN *pPd)
>>+{
>>+ if (InterlockedDecrement(&pPd->Ref) == 0) {
>>+ KeSetEvent(&pPd->Event, 0, FALSE);
>>+ }
>>+}
>>+
>>+WV_PROTECTION_DOMAIN *WvPdAcquire(WV_PROVIDER *pProvider, UINT64 Id)
>>+{
>>+ WV_PROTECTION_DOMAIN *pd;
>>+
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ pd = IndexListAt(&pProvider->PdIndex, (SIZE_T) Id);
>>+ if (pd != NULL) {
>>+ WvProviderRemoveDisable(pProvider);
>
>Same issue with bumping the reference count after you did the lookup and
>released/reacquired the mutex.
Yes - I have the same problems duplicated everywhere...
>>+ WvPdGet(pd);
>>+ }
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>>+
>>+ return pd;
>>+}
>>+
>>+void WvPdRelease(WV_PROTECTION_DOMAIN *pPd)
>>+{
>>+ WvProviderRemoveEnable(pPd->pDevice->pProvider);
>>+ WvPdPut(pPd);
>>+}
>>+
>>+NTSTATUS WvPdAlloc(WV_DEVICE *pDevice, WV_PROTECTION_DOMAIN **ppPd,
>>+ ci_umv_buf_t *pVerbsData)
>>+{
>>+ ib_api_status_t ib_status;
>>+ WV_PROTECTION_DOMAIN *pd;
>>+
>>+ pd = ExAllocatePoolWithTag(PagedPool, sizeof(WV_PROTECTION_DOMAIN),
>'apvw');
>>+ if (pd == NULL) {
>>+ return STATUS_NO_MEMORY;
>>+ }
>>+
>>+ pd->Ref = 1;
>>+ KeInitializeEvent(&pd->Event, NotificationEvent, FALSE);
>>+ pd->pDevice = pDevice;
>>+ pd->pVerbs = pDevice->pVerbs;
>>+ cl_qmap_init(&pd->MrMap);
>>+ KeInitializeGuardedMutex(&pd->Lock);
>>+
>>+ ib_status = pDevice->pVerbs->allocate_pd(pDevice->hVerbsDevice,
>IB_PDT_NORMAL,
>>+
>&pd->hVerbsPd, pVerbsData);
>>+ if (ib_status != IB_SUCCESS) {
>>+ goto err;
>>+ }
>>+
>>+ WvDeviceGet(pDevice);
>
>I would put this line together with the pDevice assignment so that it's clearer
>why you're taking a reference.
Then I end up needing to put the reference on failure...
>
>>+ *ppPd = pd;
>>+ return STATUS_SUCCESS;
>>+
>>+err:
>>+ ExFreePool(pd);
>>+ return STATUS_UNSUCCESSFUL;
>>+}
>>+
>>+void WvPdDestroy(WV_PROTECTION_DOMAIN *pPd)
>>+{
>>+ WV_MEMORY_REGION *mr;
>>+ cl_map_item_t *item;
>>+
>>+ for (item = cl_qmap_head(&pPd->MrMap); item != cl_qmap_end(&pPd-
>>MrMap);
>>+ item = cl_qmap_next(item)) {
>>+ mr = CONTAINING_RECORD(item, WV_MEMORY_REGION, Item);
>>+ pPd->pVerbs->deregister_mr(mr->hVerbsMr);
>>+ mr->hVerbsMr = NULL;
>
>Shouldn't you check that hVerbsMr is not NULL here, since WvPdRemoveHandler
>could have run (and called WvPdRemoveHandler?
yes - see previous comments -- this problem is everywhere... :(
>>+ }
>>+
>>+ if (InterlockedDecrement(&pPd->Ref) > 0) {
>>+ KeWaitForSingleObject(&pPd->Event, Executive, KernelMode,
>FALSE, NULL);
>>+ }
>>+
>>+ if (pPd->hVerbsPd != NULL) {
>>+ pPd->pVerbs->deallocate_pd(pPd->hVerbsPd);
>>+ }
>>+
>>+ WvDevicePut(pPd->pDevice);
>>+ ExFreePool(pPd);
>>+}
>>+
>>+void WvPdRemoveHandler(WV_PROTECTION_DOMAIN *pPd)
>>+{
>>+ WV_MEMORY_REGION *mr;
>>+ cl_map_item_t *item;
>>+
>>+ for (item = cl_qmap_head(&pPd->MrMap); item != cl_qmap_end(&pPd-
>>MrMap);
>>+ item = cl_qmap_next(item)) {
>>+ mr = CONTAINING_RECORD(item, WV_MEMORY_REGION, Item);
>>+ pPd->pVerbs->deregister_mr(mr->hVerbsMr);
>>+ mr->hVerbsMr = NULL;
>>+ }
>>+
>>+ pPd->pVerbs->deallocate_pd(pPd->hVerbsPd);
>>+ pPd->hVerbsPd = NULL;
>>+}
>>+
>>+void WvMemoryRegister(WV_PROVIDER *pProvider, WDFREQUEST Request)
>
>WvPdRegisterMemory?
*shrugs*
>>+{
>>+ WV_IO_MEMORY_REGISTER *reg;
>>+ WV_IO_MEMORY_KEYS *keys;
>>+ WV_PROTECTION_DOMAIN *pd;
>>+ WV_MEMORY_REGION *mr;
>>+ ib_mr_create_t attr;
>>+ NTSTATUS status;
>>+ ib_api_status_t ib_status;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request,
>sizeof(WV_IO_MEMORY_REGISTER),
>>+
>®, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto err1;
>>+ }
>>+ status = WdfRequestRetrieveOutputBuffer(Request,
>sizeof(WV_IO_MEMORY_KEYS),
>>+
>&keys, NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto err1;
>>+ }
>>+
>>+ pd = WvPdAcquire(pProvider, reg->Id);
>>+ if (pd == NULL) {
>>+ status = STATUS_NOT_FOUND;
>>+ goto err1;
>>+ }
>>+
>>+ mr = ExAllocatePoolWithTag(PagedPool, sizeof(WV_MEMORY_REGION),
>'mrvw');
>>+ if (mr == NULL) {
>>+ status = STATUS_NO_MEMORY;
>>+ goto err2;
>>+ }
>>+
>>+ attr.access_ctrl = reg->AccessFlags;
>>+ attr.length = reg->BufferLength;
>>+ attr.vaddr = (void *) (ULONG_PTR) reg->Address;
>>+ ib_status = pd->pVerbs->register_mr(pd->hVerbsPd, &attr, &keys->Lkey,
>>+
>&keys->Rkey, &mr->hVerbsMr, TRUE);
>
>TRUE here should be WdfRequestGetRequestorMode( Request ) == UserMode.
Ok - though I wasn't expecting to support kernel clients with this. Kernel
clients wouldn't need the same level as validation that these calls provide.
>>+ if (ib_status != IB_SUCCESS) {
>>+ status = STATUS_UNSUCCESSFUL;
>>+ goto err3;
>>+ }
>>+
>>+ WvPdGet(pd);
>
>You already have the PD from Acquire above.
I'm trying to keep things simple...
*Acquire() matches with *Release()
*Get() matches with *Put()
Acquire() and Release() validates the userspace index, returns the kernel
pointer, and synchronizes with HCA device removal.
Get() and Put() track referencing counting.
Yes - it looks odd to call WvPdGet() here, but it's the cleanest alternative I
could come up with.
>>+ KeAcquireGuardedMutex(&pd->Lock);
>>+ cl_qmap_insert(&pd->MrMap, keys->Lkey, &mr->Item);
>>+ KeReleaseGuardedMutex(&pd->Lock);
>>+
>>+ WvPdRelease(pd);
>
>Don't need this if you get rid of the Get call above.
>
>>+ WdfRequestCompleteWithInformation(Request, status,
>sizeof(WV_IO_MEMORY_KEYS));
>>+ return;
>>+
>>+err3:
>>+ ExFreePool(mr);
>>+err2:
>>+ WvPdRelease(pd);
>>+err1:
>>+ WdfRequestComplete(Request, status);
>>+}
>>+
>>+void WvMemoryDeregister(WV_PROVIDER *pProvider, WDFREQUEST Request)
>>+{
>>+ WV_IO_ID *id;
>>+ WV_PROTECTION_DOMAIN *pd;
>>+ WV_MEMORY_REGION *mr;
>>+ cl_map_item_t *item;
>>+ NTSTATUS status;
>>+ ib_api_status_t ib_status;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request, sizeof(WV_IO_ID), &id,
>NULL);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto complete;
>>+ }
>>+
>>+ pd = WvPdAcquire(pProvider, id->Id);
>>+ if (pd == NULL) {
>>+ status = STATUS_NOT_FOUND;
>
>STATUS_INVALID_PARAMETER?
>
>>+ goto complete;
>>+ }
>>+
>>+ KeAcquireGuardedMutex(&pd->Lock);
>>+ item = cl_qmap_remove(&pd->MrMap, id->Data);
>>+ KeReleaseGuardedMutex(&pd->Lock);
>>+
>>+ if (item == cl_qmap_end(&pd->MrMap)) {
>>+ status = STATUS_NOT_FOUND;
>
>STATUS_INVALID_PARAMETER?
*shrugs* to both of these...
>
>>+ goto release;
>>+ }
>>+
>>+ mr = CONTAINING_RECORD(item, WV_MEMORY_REGION, Item);
>>+ if (mr->hVerbsMr == NULL) {
>>+ goto free;
>>+ }
>>+
>>+ ib_status = pd->pVerbs->deregister_mr(mr->hVerbsMr);
>>+ if (ib_status != IB_SUCCESS) {
>>+ status = STATUS_UNSUCCESSFUL;
>>+ KeAcquireGuardedMutex(&pd->Lock);
>>+ cl_qmap_insert(&pd->MrMap, id->Data, &mr->Item);
>>+ KeReleaseGuardedMutex(&pd->Lock);
>>+ goto release;
>>+ }
>>+
>>+free:
>>+ WvPdPut(pd);
>>+ ExFreePool(mr);
>>+release:
>>+ WvPdRelease(pd);
>>+complete:
>>+ WdfRequestComplete(Request, status);
>>+}
>>Index: wv_pd.h
>>===================================================================
>>--- wv_pd.h (revision 1035)
>>+++ wv_pd.h (working copy)
>>@@ -32,22 +32,52 @@
>> #ifndef _WV_PD_H_
>> #define _WV_PD_H_
>>
>>+#include <ntddk.h>
>> #include <wdm.h>
>> #include <iba\ib_types.h>
>> #include <iba\ib_ci.h>
>>+
>> #include "wv_device.h"
>>+#include "wv_provider.h"
>>
>> typedef struct _WV_PROTECTION_DOMAIN
>> {
>>- WV_DEVICE *pDevice;
>>- ci_interface_t *pVerbs;
>>- ib_pd_handle_t hVerbsPd;
>>- UINT64 Id;
>>- volatile LONG m_nRef;
>>+ WV_DEVICE *pDevice;
>>+ ci_interface_t *pVerbs;
>>+ ib_pd_handle_t hVerbsPd;
>>+ LIST_ENTRY Entry;
>>
>>+ KGUARDED_MUTEX Lock;
>>+ cl_qmap_t MrMap;
>>+
>>+ SIZE_T Id;
>>+ KEVENT Event;
>>+ LONG Ref;
>>+
>> } WV_PROTECTION_DOMAIN;
>>
>>+struct _WV_PROTECTION_DOMAIN *WvPdAcquire(WV_PROVIDER *pProvider, UINT64 Id);
>>+void WvPdRelease(WV_PROTECTION_DOMAIN *pPd);
>>+void WvPdGet(WV_PROTECTION_DOMAIN *pPd);
>>+void WvPdPut(WV_PROTECTION_DOMAIN *pPd);
>>
>>+NTSTATUS WvPdAlloc(WV_DEVICE *pDevice, WV_PROTECTION_DOMAIN **ppPd,
>>+ ci_umv_buf_t *pVerbsData);
>>+void WvPdDestroy(WV_PROTECTION_DOMAIN *pPd);
>>+void WvPdRemoveHandler(WV_PROTECTION_DOMAIN *pPd);
>>+
>>+void WvMemoryRegister(WV_PROVIDER *pProvider, WDFREQUEST Request);
>>+void WvMemoryDeregister(WV_PROVIDER *pProvider, WDFREQUEST Request);
>>+
>>+
>>+typedef struct _WV_MEMORY_REGION
>>+{
>>+ WV_PROTECTION_DOMAIN *pPd;
>
>I didn't see where you set this when you created the MR. Probably not even
>needed.
I added the code to initialize it, but I don't think it's needed for MRs.
>
>>+ ib_mr_handle_t hVerbsMr;
>>+ cl_map_item_t Item;
>>+
>>+} WV_MEMORY_REGION;
>>+
>> typedef struct _WV_MEMORY_WINDOW
>> {
>> WV_PROTECTION_DOMAIN *pPd;
>>Index: wv_provider.c
>>===================================================================
>>--- wv_provider.c (revision 1035)
>>+++ wv_provider.c (working copy)
>>@@ -27,62 +27,229 @@
>> * SOFTWARE.
>> */
>>
>>-#include <winerror.h>
>>+#include <ntstatus.h>
>>
>> #include <rdma\wvstatus.h>
>> #include "wv_driver.h"
>> #include "wv_ioctl.h"
>> #include "wv_provider.h"
>> #include "wv_device.h"
>>+#include "wv_pd.h"
>>
>>-void WvGuidQuery(WDFREQUEST Request)
>>+void WvProviderGet(WV_PROVIDER *pProvider)
>> {
>>- WV_IO_GUID_LIST *pioGuids;
>>- size_t len = 0;
>>- WV_RDMA_DEVICE *pdev;
>>- ULONG count, i;
>>- LIST_ENTRY *pentry;
>>- NTSTATUS status;
>>+ InterlockedIncrement(&pProvider->Ref);
>>+}
>>
>>- status = WdfRequestRetrieveOutputBuffer(Request,
>sizeof(WV_IO_GUID_LIST),
>>-
>&pioGuids, &len);
>>- if (!NT_SUCCESS(status)) {
>>- goto out;
>>+void WvProviderPut(WV_PROVIDER *pProvider)
>>+{
>>+ if (InterlockedDecrement(&pProvider->Ref) == 0) {
>>+ KeSetEvent(&pProvider->Event, 0, FALSE);
>> }
>>+}
>
>A lot of your objects seem to have a ref count and an event. Why not have a
>'base' structure? You can use inheritance for the structures rather than
>nesting, too. Then you could have a single Ref/Deref implementation used
>everywhere.
This is strict C code, not C++. I thought about a structure to combine the
event and reference count, but the savings in code is really only a couple of
lines per widget (that's my official name for PDs, CQs, etc.)/
>
>>
>>- count = (len - sizeof(UINT64)) / sizeof(UINT64);
>>- i = 0;
>>- len = sizeof(UINT64);
>>- KeAcquireGuardedMutex(&DevLock);
>>- for (pentry = DevList.Flink; pentry != &DevList; pentry = pentry-
>>Flink) {
>>- pdev = CONTAINING_RECORD(pentry, WV_RDMA_DEVICE, Entry);
>>- if (i < count) {
>>- pioGuids->Guid[i] = pdev->Guid;
>>- len += sizeof(UINT64);
>>- }
>>- i++;
>>+void WvProviderInit(WV_PROVIDER *pProvider)
>>+{
>>+ IndexListInit(&pProvider->DevIndex);
>>+ IndexListInit(&pProvider->PdIndex);
>>+
>>+ KeInitializeGuardedMutex(&pProvider->Lock);
>>+ pProvider->Ref = 1;
>>+ KeInitializeEvent(&pProvider->Event, NotificationEvent, FALSE);
>>+
>>+ pProvider->Pending = 0;
>>+ pProvider->Active = 0;
>>+ KeInitializeEvent(&pProvider->SharedEvent, NotificationEvent, FALSE);
>>+ pProvider->Exclusive = 0;
>>+ KeInitializeEvent(&pProvider->ExclusiveEvent, SynchronizationEvent,
>FALSE);
>>+}
>>+
>>+void WvProviderCleanup(WV_PROVIDER *pProvider)
>>+{
>>+ WV_DEVICE *dev;
>>+ WV_PROTECTION_DOMAIN *pd;
>>+
>>+ while ((pd = IndexListRemoveFirst(&pProvider->PdIndex)) != NULL) {
>>+ WvPdDestroy(pd);
>> }
>>- pioGuids->Count = i;
>>- KeReleaseGuardedMutex(&DevLock);
>>
>>-out:
>>- WdfRequestCompleteWithInformation(Request, status, len);
>>+ while ((dev = IndexListRemoveFirst(&pProvider->DevIndex)) != NULL) {
>>+ WvDeviceDestroy(dev);
>>+ }
>>+
>>+ if (InterlockedDecrement(&pProvider->Ref) > 0) {
>>+ KeWaitForSingleObject(&pProvider->Event, Executive,
>KernelMode, FALSE, NULL);
>>+ }
>>+
>>+ IndexListDestroy(&pProvider->PdIndex);
>>+ IndexListDestroy(&pProvider->DevIndex);
>> }
>>
>>-void WvProviderInit(WV_PROVIDER *pProvider)
>>+// See comment above WvProviderRemoveHandler.
>>+static void WvProviderRemoveLock(WV_PROVIDER *pProvider)
>> {
>>- InitializeListHead(&pProvider->DevList);
>>- KeInitializeGuardedMutex(&pProvider->Lock);
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ pProvider->Exclusive++;
>>+ KeClearEvent(&pProvider->SharedEvent);
>>+ while (pProvider->Active > 0) {
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>>+ KeWaitForSingleObject(&pProvider->ExclusiveEvent, Executive,
>KernelMode,
>>+ FALSE, NULL);
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ }
>>+ pProvider->Active++;
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>> }
>>
>>-void WvProviderDestroy(WV_PROVIDER *pProvider)
>>+// See comment above WvProviderRemoveHandler.
>>+static void WvProviderRemoveUnlock(WV_PROVIDER *pProvider)
>> {
>>- LIST_ENTRY *entry;
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ pProvider->Exclusive--;
>>+ pProvider->Active--;
>>+ if (pProvider->Exclusive > 0) {
>>+ KeSetEvent(&pProvider->ExclusiveEvent, 0, FALSE);
>>+ } else if (pProvider->Pending > 0) {
>>+ KeSetEvent(&pProvider->SharedEvent, 0, FALSE);
>>+ }
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>>+}
>>+
>>+/*
>>+ * Must hold pProvider->Lock. Function may release and re-acquire.
>>+ * See comment above WvProviderRemoveHandler.
>>+ */
>>+void WvProviderRemoveDisable(WV_PROVIDER *pProvider)
>>+{
>>+ while (pProvider->Exclusive > 0) {
>>+ pProvider->Pending++;
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>>+ KeWaitForSingleObject(&pProvider->SharedEvent, Executive,
>KernelMode,
>>+ FALSE, NULL);
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ pProvider->Pending--;
>>+ }
>>+ InterlockedIncrement(&pProvider->Active);
>>+}
>>+
>>+/*
>>+ * No need to hold pProvider->Lock when releasing.
>>+ * See comment above WvProviderRemoveHandler.
>>+ */
>>+void WvProviderRemoveEnable(WV_PROVIDER *pProvider)
>>+{
>>+ InterlockedDecrement(&pProvider->Active);
>>+ if (pProvider->Exclusive > 0) {
>>+ KeSetEvent(&pProvider->ExclusiveEvent, 0, FALSE);
>>+ }
>>+}
>>+
>>+/*
>>+ * The remove handler blocks all other threads executing through this
>>+ * provider until the remove has been processed. Because device removal is
>>+ * rare, we want a simple, optimized code path for all calls that access
>>+ * the underlying hardware device, making use of any locks that we would
>>+ * have to acquire anyway. The locking for exclusive access can be
>>+ * as ugly and slow as needed.
>>+ */
>>+void WvProviderRemoveHandler(WV_PROVIDER *pProvider, WV_RDMA_DEVICE *pDevice)
>>+{
>> WV_DEVICE *dev;
>>+ SIZE_T i;
>>
>>- while (!IsListEmpty(&pProvider->DevList)) {
>>- entry = RemoveHeadList(&pProvider->DevList);
>>- dev = CONTAINING_RECORD(entry, WV_DEVICE, Entry);
>>- //WvDeviceDestroy(dev);
>>+ WvProviderRemoveLock(pProvider);
>>+ IndexListForEach(&pProvider->DevIndex, i) {
>>+ dev = IndexListAt(&pProvider->DevIndex, i);
>>+ if (dev->pDevice == pDevice) {
>>+ WvDeviceRemoveHandler(dev);
>>+ }
>> }
>>+ WvProviderRemoveUnlock(pProvider);
>> }
>>+
>>+void WvDeviceOpen(WV_PROVIDER *pProvider, WDFREQUEST Request)
>>+{
>>+ WV_IO_ID *inid, *outid;
>>+ size_t inlen, outlen;
>>+ WV_DEVICE *dev;
>>+ NTSTATUS status;
>>+ ci_umv_buf_t verbsData;
>>+
>>+ status = WdfRequestRetrieveInputBuffer(Request, sizeof(WV_IO_ID),
>&inid, &inlen);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto err1;
>>+ }
>>+ status = WdfRequestRetrieveOutputBuffer(Request, sizeof(WV_IO_ID),
>&outid, &outlen);
>>+ if (!NT_SUCCESS(status)) {
>>+ goto err1;
>>+ }
>>+
>>+ dev = WvDeviceAlloc(pProvider);
>>+ if (dev == NULL) {
>>+ status = STATUS_NO_MEMORY;
>>+ goto err1;
>>+ }
>>+
>>+ KeAcquireGuardedMutex(&pProvider->Lock);
>>+ WvProviderRemoveDisable(pProvider);
>>+ dev->Id = IndexListInsert(&pProvider->DevIndex, dev);
>>+ if (dev->Id == 0) {
>>+ status = STATUS_NO_MEMORY;
>>+ goto err2;
>>+ }
>>+ KeReleaseGuardedMutex(&pProvider->Lock);
>
>This publishes the device object before it is fully initialized. As soon as
>you insert the object the handle becomes valid and a call could come down and
>try to use it and fail. You must finish initialization before you make the
>object accessible to user-mode.
Yes - I caught this same mistake in a couple of other places, but missed it
here.
- Sean
More information about the ofw
mailing list