[ofw] [RFC] [PATCH 4/4] winverbs.sys: partial driver implementation
Fab Tillier
ftillier at windows.microsoft.com
Wed Apr 23 13:00:25 PDT 2008
Your device removal handler logic seems fine.
More comments inline.
>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?
>+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.
>+ }
>+ 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;
>+}
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.
>+ 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.
>+ } 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?
>+ 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?
>+ 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.
> }
>
> 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.
>+
>+ 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.
>+ 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.
>+ *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?
>+ }
>+
>+ 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?
>+{
>+ 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.
>+ if (ib_status != IB_SUCCESS) {
>+ status = STATUS_UNSUCCESSFUL;
>+ goto err3;
>+ }
>+
>+ WvPdGet(pd);
You already have the PD from Acquire above.
>+ 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?
>+ 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.
>+ 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.
>
>- 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.
>+
>+ WvInitVerbsData(&verbsData, inid->VerbInfo, inlen - sizeof(WV_IO_ID),
>+ outlen - sizeof(WV_IO_ID), inid + 1);
>+ status = WvDeviceInit(dev, inid->Id, &verbsData);
>+ if (!NT_SUCCESS(status)) {
>+ goto err3;
>+ }
>+
>+ WvProviderRemoveEnable(pProvider);
>+ outid->Id = dev->Id;
>+ outid->VerbInfo = verbsData.status;
>+ WdfRequestCompleteWithInformation(Request, status, outlen);
>+ return;
>+
>+err3:
>+ KeAcquireGuardedMutex(&pProvider->Lock);
>+ IndexListRemove(&pProvider->DevIndex, dev->Id);
>+err2:
>+ KeReleaseGuardedMutex(&pProvider->Lock);
>+ WvProviderRemoveEnable(pProvider);
>+ WvDeviceDestroy(dev);
>+err1:
>+ WdfRequestComplete(Request, status);
>+}
>+
>+void WvDeviceClose(WV_PROVIDER *pProvider, WDFREQUEST Request)
>+{
>+ WV_DEVICE *dev;
>+ UINT64 *id;
>+ NTSTATUS status;
>+
>+ status = WdfRequestRetrieveInputBuffer(Request, sizeof(UINT64), &id, NULL);
>+ if (!NT_SUCCESS(status)) {
>+ goto out;
>+ }
>+
>+ KeAcquireGuardedMutex(&pProvider->Lock);
>+ WvProviderRemoveDisable(pProvider);
>+ dev = IndexListRemove(&pProvider->DevIndex, (SIZE_T) id);
>+ if (dev == NULL) {
>+ status = STATUS_NO_SUCH_DEVICE;
>+ } else if (dev->Ref > 1) {
>+ status = STATUS_ACCESS_DENIED;
>+ } else {
>+ status = STATUS_SUCCESS;
>+ }
>+ KeReleaseGuardedMutex(&pProvider->Lock);
>+
>+ if (NT_SUCCESS(status)) {
>+ WvDeviceDestroy(dev);
>+ }
>+ WvProviderRemoveEnable(pProvider);
>+out:
>+ WdfRequestComplete(Request, status);
>+}
>Index: wv_provider.h
>===================================================================
>--- wv_provider.h (revision 1035)
>+++ wv_provider.h (working copy)
>@@ -36,16 +36,40 @@
> #include <wdf.h>
> #include <wdm.h>
>
>+#include <complib\cl_qmap.h>
>+#include "wv_driver.h"
>+#include "index_list.h"
>+
>+struct _WV_DEVICE;
>+struct _WV_PROTECTION_DOMAIN;
>+
> typedef struct _WV_PROVIDER
> {
>+ LIST_ENTRY Entry;
>+ INDEX_LIST DevIndex;
>+ INDEX_LIST PdIndex;
>+
> KGUARDED_MUTEX Lock;
>- LIST_ENTRY DevList;
>+ LONG Ref;
>+ KEVENT Event;
>+ LONG Pending;
>+ LONG Active;
>+ KEVENT SharedEvent;
>+ LONG Exclusive;
>+ KEVENT ExclusiveEvent;
>
> } WV_PROVIDER;
>
>+void WvProviderGet(WV_PROVIDER *pProvider);
>+void WvProviderPut(WV_PROVIDER *pProvider);
> void WvProviderInit(WV_PROVIDER *pProvider);
>-void WvProviderDestroy(WV_PROVIDER *pProvider);
>+void WvProviderCleanup(WV_PROVIDER *pProvider);
>
>-void WvGuidQuery(WDFREQUEST Request);
>+void WvProviderRemoveHandler(WV_PROVIDER *pProvider, WV_RDMA_DEVICE *pDevice);
>+void WvProviderRemoveDisable(WV_PROVIDER *pProvider);
>+void WvProviderRemoveEnable(WV_PROVIDER *pProvider);
>
>+void WvDeviceOpen(WV_PROVIDER *pProvider, WDFREQUEST Request);
>+void WvDeviceClose(WV_PROVIDER *pProvider, WDFREQUEST Request);
>+
> #endif // _WV_PROVIDER_H_
>
>
>_______________________________________________
>ofw mailing list
>ofw at lists.openfabrics.org
>http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
>
More information about the ofw
mailing list