[Openib-windows] RE: A patch for listing the ip addresses and fetching remote gids.
Fab Tillier
ftillier at silverstorm.com
Fri Sep 16 11:36:12 PDT 2005
> From: Tzachi Dar [mailto:tzachid at mellanox.co.il]
> Sent: Friday, September 16, 2005 1:09 AM
>
> Hi fab,
>
> Bellow is the patch that allows other components to communicate with ipoib,
> and to get ip addresses as well as ports.
This is great, thanks.
A few nits about this patch:
- Use tabs, not spaces.
- The ip_addresses_shared.h file, by virtue of being in the ipoib directory and
not the user or kernel subdirectory, is shared. No need to add the shared
suffix to the filename.
- Curly braces in the rest of the code base are on separate lines, and line up
with one another vertically. While this is different than the Linux coding
guidelines, we should stick to the existing model. It makes code much easier to
read, especially for cases where you have multi-line conditional/loop statements
by making it clear where the conditional/loop body begins and ends.
- Lines should be <= 80 characters
- For some reason, the PrintIp test case and its makefile and sources file were
repeated in the patch you sent 3 times.
- Try to avoid trailing white space.
Also, I think the shared header should be named something like ib_at.h, since
that's the functionality it provides.
I will start merging some of these things into the code base, starting with
simple functionality like:
- Tracking adapters in a global list
- Registering the unload handler
> (the file is also attached).
> Index: ulp/ipoib/ip_addresses_shared.h
> ===================================================================
> --- ulp/ipoib/ip_addresses_shared.h (revision 0)
> +++ ulp/ipoib/ip_addresses_shared.h (revision 0)
> @@ -0,0 +1,109 @@
> +/*
<snip>
> +// This file is shared between the user and kernel
> +
> +#ifndef _IP_ADDRESSES_SHARED_H_
> +#define _IP_ADDRESSES_SHARED_H_
> +
> +#define IPOIB_IOCTL_VERSION 1
> +
> +#define HW_ADDR_LEN 6
Only define HW_ADDR_LEN if it hasn't been defined yet. If you fear different
sizes, then define it for use in the IOCTL definition only (change the name to
something like AT_MAC_LEN).
> +
> +
> +// This IRP is used to return all available CAs ports number and port guid
> +#define IOCTL_IPOIB_PORTS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x801,
> METHOD_BUFFERED ,FILE_ANY_ACCESS)
> +
> +struct IPOIB_AT_PORT_RECORD {
> + ib_net64_t CaGuid;
> + ib_net64_t PortGuid;
> + ULONG PortNumber;
> +};
Should we just use native Windows types here (UINT64 rather than ib_net64_t)?
> +
> +struct IOCTL_IPOIB_PORTS_IN {
> + ULONG Version;
> + ULONG Size; //Total size, of
> the output buffers
> +};
> +
> +struct IOCTL_IPOIB_PORTS_OUT {
> + ULONG Version;
> + ULONG Size; //Total size,
> of the output buffer needed if the
> + // suplied buffer wasn't enough
> + ULONG NumPorts;
> + struct IPOIB_AT_PORT_RECORD Ports[1];
> +};
> +
> +// This IRP is used to return all the ip addresses that are assigned to a
> port
> +#define IOCTL_IPOIB_IP_ADDRESSES CTL_CODE(FILE_DEVICE_UNKNOWN, 0x802,
> METHOD_BUFFERED ,FILE_ANY_ACCESS)
FILE_READ_ACCESS should be all that's needed, since the clients can only query
information, and not set it. Or does that not work with IOCTLs?
> +
> +struct IOCTL_IPOIB_IP_ADDRESSES_IN {
> + ULONG Version;
> + ib_net64_t PortGuid; // The guid of
> the port that we are queriyn for
> +};
> +
> +struct IP_ADDRESS {
> + char IpVersion; // might only be 4 or 6
> + unsigned char Data[16]; // This allows to return ip v6 as
> well
> +};
> +
> +struct IOCTL_IPOIB_IP_ADDRESSES_OUT {
> + ULONG Version;
> + ULONG Size; //Total size,
> of the output buffer needed if the
> + // suplied buffer wasn't enough
> + ULONG NumIps;
> + struct IP_ADDRESS Addreses[1];
> +};
> +
> +
> +// This IRP is used to convert a remote mac addresses to a remote GID
> +#define IOCTL_IPOIB_MAC_2_GID CTL_CODE(FILE_DEVICE_UNKNOWN, 0x803,
> METHOD_BUFFERED ,FILE_ANY_ACCESS)
> +
> +
> +struct IOCTL_IPOIB_MAC_2_GID_IN {
> + unsigned char DestMac[HW_ADDR_LEN];
This should take as input the source port GUID. It is valid for different,
disjoint subnets to have duplicate MAC addresses.
This also optimizes the lookup by targeting a specific instance rather than
searching all.
> +};
> +
> +struct IOCTL_IPOIB_MAC_2_GID_OUT {
> + ib_gid_t DestGid;
> +};
> +
> +// The following structure functions and structurs define the functions
> that are needed in order to
> +// hold an array of the ports that are currently available.
> +
> +#define Name L"IPOIB"
> +
> +#define IPOIB_DEV_NAME L"\\Device\\"Name
> +#define IPOIB_DOS_DEV_NAME L"\\DosDevices\\Global\\"Name
> +#define IPOIB_WIN32_NAME L"\\\\.\\"Name
> +
> +
> +
> +#endif /* _IP_ADDRESSES_SHARED_H_ */
> Index: ulp/ipoib/ip_packet.h
> ===================================================================
> --- ulp/ipoib/ip_packet.h (revision 385)
> +++ ulp/ipoib/ip_packet.h (working copy)
> @@ -37,12 +37,6 @@
> #include <complib/cl_types.h>
> #include <complib/cl_byteswap.h>
>
> -
> -#ifndef HW_ADDR_LEN
> -#define HW_ADDR_LEN 6
> -#endif /* HW_ADDR_LEN */
> -
> -
This should be left here to allow other NDIS drivers to use this file without
having to include the IPoIB address translation file - this is needed for IB to
Ethernet gateways, for example. If the other definition is protected with a
#ifndef, everything should be fine. Alternatively, the other definition should
have its own name, so that it is consistent with the IOCTL buffer definition.
> #define ETH_PROT_TYPE_IP CL_HTON16(0x800)
> #define ETH_PROT_TYPE_ARP CL_HTON16(0x806)
>
> Index: ulp/ipoib/kernel/ip_Addresses.c
> ===================================================================
> --- ulp/ipoib/kernel/ip_Addresses.c (revision 0)
> +++ ulp/ipoib/kernel/ip_Addresses.c (revision 0)
> @@ -0,0 +1,580 @@
> +/*
<snip>
> + */
> +#include <iba/ib_types.h>
> +#include "ip_addresses_shared.h"
> +#include "ipoib_adapter.h"
> +#include "ipoib_port.h"
> +#include "ipoib_debug.h"
> +#include "ip_addresses.h"
> +#include "ipoib_endpoint.h"
> +
> +
> +NTSTATUS
> +__ipoib_dispatch(
> + IN PDEVICE_OBJECT device_object,
> + IN PIRP Irp
> + );
> +
> +
> +struct ipoib_addresses_config {
> +
> + PDEVICE_OBJECT device_object;
> + NDIS_HANDLE ndis_device_handle;
> + cl_list_t ports_list;
Don't use cl_list - it performs allocations on the fly, which means you have to
handle potential failures. Just use qlist here, and add a list item to the
adapter object. If you want efficient lookups using the port GUID, use a quick
map with the port GUID as key.
The adapter object should remove itself from the list/map in its "destroying"
handler or similar.
> + cl_spinlock_t spin_lock; // To protect the list
> +
> +};
> +
> +struct ipoib_addresses_config g_ipoib_addresses_config;
> +
> +void __ipoib_lock_list()
> +{
> + cl_spinlock_acquire(&g_ipoib_addresses_config.spin_lock);
> +}
> +
> +void __ipoib_unlock_list()
> +{
> + cl_spinlock_release(&g_ipoib_addresses_config.spin_lock);
> +}
Having wrapper functions to take and release a lock seems silly. Why not just
make the call to the spinlock functions directly?
Also, I would recommend using in stack queued spinlocks, as they supposedly
perform better. For this, just call the native Windows calls.
> +
> +
> +NDIS_STATUS __ipoib_ip_create_device(
> + NDIS_HANDLE ndis_wrapper_handle
> + );
> +
> +VOID __ipoib_dregister_device();
> +
> +NDIS_STATUS ipoib_get_ipoib_ports(
> + struct IOCTL_IPOIB_PORTS_IN ports_in,
> + struct IOCTL_IPOIB_PORTS_OUT *ports_out,
> + IN ULONG OutputBufferLength,
> + OUT ULONG *pOutputDataSize
> + )
> +{
> + NDIS_STATUS status = NDIS_STATUS_SUCCESS;
> + ULONG num_ports, i = 0;
> + ULONG needed_size_bytes;
> + cl_list_iterator_t it;
> + ipoib_adapter_t *p_adapter;
> + struct IPOIB_AT_PORT_RECORD *ports_records;
> +
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> + ports_out->Version = IPOIB_IOCTL_VERSION;
> + ports_out->Size = 0;
> + if (ports_in.Version != IPOIB_IOCTL_VERSION) {
> + IPOIB_TRACE( IPOIB_DBG_ERROR, ("unsupported version = 0x%x\n",
> ports_in.Version ));
> + return STATUS_INVALID_PARAMETER;
> + }
> + num_ports = cl_list_count(&g_ipoib_addresses_config.ports_list);
> + if (num_ports == 0) {
> + needed_size_bytes = sizeof (struct IOCTL_IPOIB_PORTS_OUT);
> + } else {
> + needed_size_bytes = sizeof (struct IOCTL_IPOIB_PORTS_OUT) +
> + (num_ports - 1) * sizeof (struct IPOIB_AT_PORT_RECORD);
> + }
> + if (needed_size_bytes > OutputBufferLength) {
> + // Not enough data for us to use, we should return only the
> + // size
Return the size, and as much data as will fit. This allows clients to query for
the first address in the list without having to query for all.
> + ports_out->NumPorts = num_ports;
> + ports_out->Size = needed_size_bytes;
> + return NDIS_STATUS_SUCCESS;
> + }
> + *pOutputDataSize = needed_size_bytes;
> + ports_out->NumPorts = num_ports;
> + ports_records = ports_out->Ports;
> +
> + __ipoib_lock_list();
Locking needs to either be moved up to protect the size checks so that we don't
overflow here, or we need to perform boundary checks on the input buffer as we
fill it here.
I would prefer to see us perform the boundary checks here, and return a partial
buffer if possible (along with the required size).
> +
> + for (it = cl_list_head(&g_ipoib_addresses_config.ports_list);
> + it != cl_list_end(&g_ipoib_addresses_config.ports_list);
> + it = cl_list_next(it), i++) {
> + p_adapter = cl_list_obj(it);
> +
> + cl_obj_lock( &p_adapter->obj );
> + ports_records[i].CaGuid = p_adapter->guids.ca_guid;
> + ports_records[i].PortGuid = p_adapter->guids.port_guid;
> + cl_obj_unlock( &p_adapter->obj );
> +
> + }
> + __ipoib_unlock_list();
> + CL_ASSERT(i == num_ports);
This assert will be triggered if an adapter is enabled (or disabled) while a
query is in progress, but before the lock was acquired. We don't want this.
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return status;
> +
> +}
> +
> +
> +NDIS_STATUS __ipoib_get_port_ips(
> + struct IOCTL_IPOIB_IP_ADDRESSES_IN addresses_in,
> + struct IOCTL_IPOIB_IP_ADDRESSES_OUT *addresses_out,
> + IN ULONG OutputBufferLength,
> + OUT ULONG *pOutputDataSize
> + )
> +{
> + ULONG num_ips, i = 0;
> + ULONG needed_size_bytes;
> + cl_list_iterator_t it;
> + ipoib_adapter_t *p_adapter;
> + struct IP_ADDRESS *ip_records;
> + net_address_item_t *p_addr_item;
> +
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + addresses_out->Version = IPOIB_IOCTL_VERSION;
> + addresses_out->Size = 0;
> + if (addresses_in.Version != IPOIB_IOCTL_VERSION) {
> + IPOIB_TRACE( IPOIB_DBG_ERROR, ("unsupported version = 0x%x\n",
> addresses_in.Version ));
> + return STATUS_INVALID_PARAMETER;
> + }
> + __ipoib_lock_list();
> +
> + for (it = cl_list_head(&g_ipoib_addresses_config.ports_list);
> + it != cl_list_end(&g_ipoib_addresses_config.ports_list);
> + it = cl_list_next(it)) {
> + p_adapter = cl_list_obj(it);
> +
> + cl_obj_lock( &p_adapter->obj );
> + if (p_adapter->guids.port_guid == addresses_in.PortGuid) {
> + // We should return our answer based on our answer
> + num_ips = cl_vector_get_size(&p_adapter->ip_vector);
> + if (num_ips == 0) {
> + needed_size_bytes = sizeof (struct
> IOCTL_IPOIB_IP_ADDRESSES_OUT);
> + } else {
> + needed_size_bytes = sizeof (struct
> IOCTL_IPOIB_IP_ADDRESSES_OUT) +
> + (num_ips - 1) * sizeof (struct IP_ADDRESS);
> + }
> + if (needed_size_bytes > OutputBufferLength) {
> + // Not enough data for us to use, we should return only the
> + // size
Return as much data as can fit (in integral IB_ADDRESS units, of course).
> + addresses_out->NumIps = num_ips;
> + addresses_out->Size = needed_size_bytes;
> + cl_obj_unlock( &p_adapter->obj );
> + __ipoib_unlock_list();
> + return NDIS_STATUS_SUCCESS;
Set status and break would be cleaner.
> + }
> + *pOutputDataSize = needed_size_bytes;
> +
> + addresses_out->NumIps = num_ips;
> + ip_records = addresses_out->Addreses;
> +
> + // Copy all the IP's into the vector
> + for (i = 0 ; i < num_ips; i++) {
> +
> + ip_records[i].IpVersion = 4;
> + p_addr_item = (net_address_item_t*)
> + cl_vector_get_ptr(
> &p_adapter->ip_vector, i );
> + ip_records[i].Data[12] = p_addr_item->address.as_bytes[0];
> + ip_records[i].Data[13] = p_addr_item->address.as_bytes[1];
> + ip_records[i].Data[14] = p_addr_item->address.as_bytes[2];
> + ip_records[i].Data[15] = p_addr_item->address.as_bytes[3];
The first 12 bytes should be cleared (matches how IPv4 addresses are specified
in the SDP hello message, as well as ATS service registrations).
> + }
> +
> + cl_obj_unlock( &p_adapter->obj );
> +
> + __ipoib_unlock_list();
> + return NDIS_STATUS_SUCCESS;
> +
> + }
> + cl_obj_unlock( &p_adapter->obj );
It would be cleaner to release the lock, set the return value, and break out of
the for loop.
> +
> +
> + }
> + // If we have reached here, this means that the port was not found
> + __ipoib_unlock_list();
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return STATUS_INVALID_PORT_HANDLE;
> +
> +}
> +
> +
> +NDIS_STATUS __ipoib_get_gid(
> + struct IOCTL_IPOIB_MAC_2_GID_IN mac_in,
> + struct IOCTL_IPOIB_MAC_2_GID_OUT *gid_out,
> + OUT ULONG *pOutputDataSize
> + )
> +{
> + cl_list_iterator_t it;
> + ipoib_adapter_t *p_adapter;
> + NDIS_STATUS status;
> + mac_addr_t mac;
> +
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + *pOutputDataSize = sizeof (gid_out->DestGid);
> +
> + // Go over all addapters and try to find the correct endpoint
> +
> + memcpy(mac.addr, mac_in.DestMac, HW_ADDR_LEN);
> +
> + __ipoib_lock_list();
> +
> + for (it = cl_list_head(&g_ipoib_addresses_config.ports_list);
> + it != cl_list_end(&g_ipoib_addresses_config.ports_list);
> + it = cl_list_next(it)) {
> + p_adapter = cl_list_obj(it);
> +
> + cl_obj_lock( &p_adapter->obj );
> +
> + status = gid_from_mac( p_adapter->p_port, mac, &gid_out->DestGid );
> +
> + if (status == NDIS_STATUS_SUCCESS) {
> + cl_obj_unlock( &p_adapter->obj );
> + __ipoib_unlock_list();
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return status;
> + }
> +
> + cl_obj_unlock( &p_adapter->obj );
> +
> +
> + }
> + __ipoib_unlock_list();
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return NDIS_STATUS_NO_ROUTE_TO_DESTINATION;
> +
> +}
> +
> +#define VERIFY_BUFFERS(InputBufferLength, OutputBufferLength, InStructSize,
> OutStructZize) \
> +if ((InputBufferLength < InStructSize) ||
> \
> + (OutputBufferLength < OutStructZize)) {
> \
> + IPOIB_TRACE( IPOIB_DBG_ERROR,
> \
> + ("ipoib_dispatch_device_io_control bad buffer sizes\n" ));
> \
> + CL_ASSERT(FALSE);
> \
> + status = STATUS_ACCESS_VIOLATION;
> \
> + goto Cleanup;
> \
> +}
Yuk. I hate macros, especially with an embedded goto. It completely obscures
the code flow through the function, making the code harder to read. Make the
checks explicit - it won't hurt readability.
> +
> +NDIS_STATUS __ipoib_dispatch_device_io_control(
> + IN PVOID pInputBuffer,
> + IN ULONG InputBufferLength ,
> + IN PVOID pOutputBuffer,
> + IN ULONG OutputBufferLength,
> + IN ULONG IoControlCode,
> + OUT ULONG *pOutputDataSize
> + )
> +{
> + NDIS_STATUS status = NDIS_STATUS_SUCCESS;
> + struct IOCTL_IPOIB_PORTS_IN ports_in;
> + struct IOCTL_IPOIB_PORTS_OUT *ports_out;
> + struct IOCTL_IPOIB_IP_ADDRESSES_IN addreses_in;
> + struct IOCTL_IPOIB_IP_ADDRESSES_OUT *addreses_out;
> + struct IOCTL_IPOIB_MAC_2_GID_IN ip_in;
> + struct IOCTL_IPOIB_MAC_2_GID_OUT *ip_out;
You should try to avoid making copies of the input parameters. I think simply
by restructuring the handlers to not touch the output buffer first, you could
eliminate all these. If you do want to keep them, you should declare all input
parameters in a union, likewise for all output parameters.
> +
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + switch (IoControlCode) {
> + case IOCTL_IPOIB_PORTS :
> + {
> + IPOIB_TRACE( IPOIB_DBG_IOCTL, ("IOCTL_IPOIB_PORTS recieved\n"
> ));
> + VERIFY_BUFFERS(InputBufferLength, OutputBufferLength, sizeof
> (struct IOCTL_IPOIB_PORTS_IN), sizeof (struct IOCTL_IPOIB_PORTS_OUT));
> +
> +
> + ports_in = *(struct IOCTL_IPOIB_PORTS_IN *) pInputBuffer;
> + ports_out = (struct IOCTL_IPOIB_PORTS_OUT *) pOutputBuffer;
> +
> + status = ipoib_get_ipoib_ports(
> + ports_in,
> + ports_out,
> + OutputBufferLength,
> + pOutputDataSize
> + );
> + }
> + break;
> + case IOCTL_IPOIB_IP_ADDRESSES :
> + {
> + IPOIB_TRACE( IPOIB_DBG_IOCTL, ("IOCTL_IPOIB_IP_ADDRESSES
> recieved\n" ));
> + VERIFY_BUFFERS(InputBufferLength, OutputBufferLength, sizeof
> (struct IOCTL_IPOIB_IP_ADDRESSES_IN), sizeof (struct
> IOCTL_IPOIB_IP_ADDRESSES_OUT));
> +
> + addreses_in = *(struct IOCTL_IPOIB_IP_ADDRESSES_IN *)
> pInputBuffer;
> + addreses_out = (struct IOCTL_IPOIB_IP_ADDRESSES_OUT *)
> pOutputBuffer;
> +
> + status = __ipoib_get_port_ips(
> + addreses_in,
> + addreses_out,
> + OutputBufferLength,
> + pOutputDataSize
> + );
> + }
> + break;
> +
> + case IOCTL_IPOIB_MAC_2_GID :
> + {
> + IPOIB_TRACE( IPOIB_DBG_IOCTL, ("IOCTL_IPOIB_MAC_2_GID
> recieved\n" ));
> + VERIFY_BUFFERS(InputBufferLength, OutputBufferLength, sizeof
> (struct IOCTL_IPOIB_MAC_2_GID_IN), sizeof (struct
> IOCTL_IPOIB_MAC_2_GID_OUT));
> +
> + ip_in = *(struct IOCTL_IPOIB_MAC_2_GID_IN *) pInputBuffer;
> + ip_out = (struct IOCTL_IPOIB_MAC_2_GID_OUT *) pOutputBuffer;
> +
> + status = __ipoib_get_gid(
> + ip_in,
> + ip_out,
> + pOutputDataSize
> + );
> + }
> + break;
> +
> +
> +
> +
> + default:
> + // This is an unrecgnized IOCTL
> + CL_ASSERT(FALSE);
> + IPOIB_TRACE( IPOIB_DBG_ERROR, ("unknow IOCTL code = 0x%x\n",
> IoControlCode ));
> + status = STATUS_INVALID_PARAMETER;
> + }
> +Cleanup:
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return status;
> +
> +}
> +
> +NDIS_STATUS ipoib_driver_up(NDIS_HANDLE ndis_wrapper_handle)
> +{
> + NDIS_STATUS status;
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> + status = cl_list_init(&g_ipoib_addresses_config.ports_list, 8);
> + if( status != CL_SUCCESS )
> + {
> + IPOIB_TRACE( IPOIB_DBG_ERROR,
> + ("cl_list_init failed with status of
> %d\n", status) );
> + return status;
> + }
> +
> + status = __ipoib_ip_create_device(ndis_wrapper_handle);
> + if( status != NDIS_STATUS_SUCCESS )
> + {
> + IPOIB_TRACE( IPOIB_DBG_ERROR,
> + ("NdisMRegisterDevice failed with status
> of %d\n", status) );
> + cl_list_destroy(&g_ipoib_addresses_config.ports_list);
> + return status;
> + }
> +
> + status = cl_spinlock_init(&g_ipoib_addresses_config.spin_lock);
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return status;
> +
> +
> +}
> +
> +void ipoib_driver_unload()
> +{
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> + cl_list_destroy(&g_ipoib_addresses_config.ports_list);
> + cl_spinlock_destroy(&g_ipoib_addresses_config.spin_lock);
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> +
> +}
I'll move this into the same file as DriverEntry - it makes more sense there.
> +
> +NDIS_STATUS __ipoib_ip_create_device(
> + NDIS_HANDLE ndis_wrapper_handle
> + )
> +{
> + NDIS_STATUS status;
> + NDIS_STRING DeviceName;
> + NDIS_STRING DeviceLinkUnicodeString;
> + PDRIVER_DISPATCH DispatchTable[IRP_MJ_MAXIMUM_FUNCTION+1];
> +
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + NdisInitUnicodeString(&DeviceName, IPOIB_DEV_NAME);
> + NdisInitUnicodeString(&DeviceLinkUnicodeString, IPOIB_DOS_DEV_NAME);
> +
> + NdisZeroMemory(DispatchTable, (IRP_MJ_MAXIMUM_FUNCTION+1) *
> sizeof(PDRIVER_DISPATCH));
> +
> + DispatchTable[IRP_MJ_CREATE] = __ipoib_dispatch;
> + DispatchTable[IRP_MJ_CLEANUP] = __ipoib_dispatch;
> + DispatchTable[IRP_MJ_CLOSE] = __ipoib_dispatch;
> + DispatchTable[IRP_MJ_DEVICE_CONTROL] = __ipoib_dispatch;
> + DispatchTable[IRP_MJ_INTERNAL_DEVICE_CONTROL] = __ipoib_dispatch;
> +
> + status = NdisMRegisterDevice(
> + ndis_wrapper_handle,
> + &DeviceName,
> + &DeviceLinkUnicodeString,
> + &DispatchTable[0],
> + &g_ipoib_addresses_config.device_object,
> + &g_ipoib_addresses_config.ndis_device_handle
> + );
> + if( status != NDIS_STATUS_SUCCESS )
> + {
> + IPOIB_TRACE( IPOIB_DBG_ERROR,
> + ("NdisMRegisterDevice failed with status
> of %d\n", status) );
> + } else {
> + IPOIB_TRACE( IPOIB_DBG_IOCTL,
> + ("NdisMRegisterDevice succeeded\n") );
> + }
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return status;
> +
> +
> +}
> +
> +NDIS_STATUS ipoib_add_adapter(ipoib_adapter_t *p_adapter)
> +{
> + NDIS_STATUS status;
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + __ipoib_lock_list();
> + status = cl_list_insert_head(&g_ipoib_addresses_config.ports_list,
> p_adapter);
> + __ipoib_unlock_list();
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> + return status;
> +}
> +
> +
> +VOID ipoib_remove_adapter(ipoib_adapter_t *p_adapter)
> +{
> + cl_status_t status;
> + ULONG count;
> +
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + __ipoib_lock_list();
> + status = cl_list_remove_object(&g_ipoib_addresses_config.ports_list,
> p_adapter);
> + CL_ASSERT(status == CL_SUCCESS);
> +
> + count = cl_list_count(&g_ipoib_addresses_config.ports_list);
> + __ipoib_unlock_list();
> +
> + if (count == 0 ) {
> + __ipoib_dregister_device();
> + }
> +
> +
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> +
> +}
> +
> +VOID __ipoib_dregister_device()
> +{
> + NDIS_STATUS status;
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + if (g_ipoib_addresses_config.ndis_device_handle != NULL) {
> + status =
> NdisMDeregisterDevice(g_ipoib_addresses_config.ndis_device_handle);
What happens if you disable the last IPoIB instance while an application has the
associated file object open? The docs say the function will fail. If it does,
the code should check the count when it receives a IRP_MJ_CLOSE, and try again
if it's zero.
> + if( status != NDIS_STATUS_SUCCESS )
> + {
> + IPOIB_TRACE( IPOIB_DBG_ERROR,
> + ("NdisMDeregisterDevice failed with
> status of %d", status) );
> + } else {
> + IPOIB_TRACE( IPOIB_DBG_IOCTL,
> + ("NdisMDeregisterDevice succeeded\n") );
> + }
> +
> + g_ipoib_addresses_config.ndis_device_handle = NULL;
> + }
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> +}
> +
> +
> +NTSTATUS
> +__ipoib_dispatch(
> + IN PDEVICE_OBJECT device_object,
> + IN PIRP pIrp
> + )
> +/*++
> +Routine Description:
> +
> + Process IRPs sent to this device.
> +
> +Arguments:
> +
> + device_object - pointer to a device object
> + Irp - pointer to an I/O Request Packet
> +
> +Return Value:
> +
> + NTSTATUS - STATUS_SUCCESS always - change this when adding
> + real code to handle ioctls.
> +
> +--*/
Documenation before the function like the rest of the code base, please. I know
the Windows headers do it this way, but it becomes unreadable if the
documentation gets of any significant length (you can't see what the function
prototype is without scrolling up.
> +{
> + PIO_STACK_LOCATION pIrpSp;
> + NTSTATUS status = STATUS_SUCCESS;
> + int Method;
> + ULONG OutputDataSize = 0;
> +
> + UNREFERENCED_PARAMETER(device_object);
> +
> + IPOIB_ENTER(IPOIB_DBG_IOCTL);
> +
> + pIrpSp = IoGetCurrentIrpStackLocation(pIrp);
> +
> +
> + switch (pIrpSp->MajorFunction)
> + {
> + case IRP_MJ_CREATE:
> + IPOIB_TRACE( IPOIB_DBG_IOCTL, ("IRP_MJ_CREATE recieved\n") );
> + break;
> +
> + case IRP_MJ_CLEANUP:
> + IPOIB_TRACE( IPOIB_DBG_IOCTL, ("IRP_MJ_CLEANUP recieved\n") );
>
> + break;
> +
> + case IRP_MJ_CLOSE:
> + IPOIB_TRACE( IPOIB_DBG_IOCTL, ("IRP_MJ_CLOSE recieved\n") );
>
> + break;
> +
> + case IRP_MJ_DEVICE_CONTROL:
> + IPOIB_TRACE( IPOIB_DBG_IOCTL, ("IRP_MJ_DEVICE_CONTROL
> recieved\n") );
> + Method = pIrpSp->Parameters.DeviceIoControl.IoControlCode &
> 0x3;
> + if (Method != METHOD_BUFFERED) {
> + // We only support method buffered (very small requests)
> + status = STATUS_INVALID_PARAMETER;
> + IPOIB_TRACE( IPOIB_DBG_ERROR,
> + ("Unsupporteed ioctl Method %d\n",
> Method) );
> + } else {
> + status = __ipoib_dispatch_device_io_control(
> + pIrp->AssociatedIrp.SystemBuffer,
> + pIrpSp->Parameters.DeviceIoControl.InputBufferLength,
> + pIrp->AssociatedIrp.SystemBuffer,
> + pIrpSp->Parameters.DeviceIoControl.OutputBufferLength,
> + pIrpSp->Parameters.DeviceIoControl.IoControlCode,
> + &OutputDataSize
> + );
> + }
> + break;
> + default:
> + break;
> + }
> + CL_ASSERT(status != STATUS_PENDING);
> +
> + pIrp->IoStatus.Status = status;
> + pIrp->IoStatus.Information = OutputDataSize;
> + IoCompleteRequest(pIrp, IO_NO_INCREMENT);
> +
> + IPOIB_EXIT(IPOIB_DBG_IOCTL);
> +
> + return status;
> +
> +}
> +
> Index: ulp/ipoib/kernel/ip_Addresses.h
> ===================================================================
> --- ulp/ipoib/kernel/ip_Addresses.h (revision 0)
> +++ ulp/ipoib/kernel/ip_Addresses.h (revision 0)
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright (c) 2005 Mellanox Technologies. 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 AND
> + * 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.
> + *
> + * $Id: ip_packet.h 47 2005-05-30 18:00:40Z sleybo $
> + */
> +
> +#ifndef _IP_ADDRESSES_H_
> +#define _IP_ADDRESSES_H_
> +
> +#include <ip_addresses_shared.h>
> +
> +NDIS_STATUS ipoib_driver_up(
> + NDIS_HANDLE ndis_wrapper_handle
> + );
> +
> +void ipoib_driver_unload();
> +
> +//VOID dregister_device();
> +
> +NDIS_STATUS ipoib_add_adapter(ipoib_adapter_t *p_adapter);
> +
> +VOID ipoib_remove_adapter(ipoib_adapter_t *p_adapter);
> +
> +
> +#endif /* _IP_ADDRESSES_H_ */
> Index: ulp/ipoib/kernel/ipoib_adapter.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_adapter.c (revision 385)
> +++ ulp/ipoib/kernel/ipoib_adapter.c (working copy)
> @@ -30,14 +30,15 @@
> */
>
>
> -
> +#include <iba/ib_types.h>
> +#include "ip_addresses_shared.h"
> #include "ipoib_adapter.h"
> #include "ipoib_port.h"
> #include "ipoib_driver.h"
> #include "ipoib_debug.h"
> #include <complib/cl_init.h>
> +#include <ip_addresses.h>
>
> -
> #define ITEM_POOL_START 16
> #define ITEM_POOL_GROW 16
>
> @@ -207,6 +208,16 @@
> return status;
> }
>
> + status = ipoib_add_adapter(p_adapter);
> + if( status != IB_SUCCESS )
> + {
> + cl_obj_destroy( &p_adapter->obj ); // ??? is this the
> correct way to stop
Yes, that's the correct way to destroy the adapter. I don't think that's what
you want to do, though - why disable IPoIB all together just because address
translation won't work? Also, using quick list allows you to do this without
having to check for success or failure.
> + IPOIB_TRACE_EXIT( IPOIB_DBG_ERROR,
> + ("adapter_init returned %s.\n",
> + p_adapter->p_ifc->get_err_str( status ))
> );
> + return status;
> + }
> +
> *pp_adapter = p_adapter;
>
> IPOIB_EXIT( IPOIB_DBG_INIT );
> @@ -235,6 +246,8 @@
> {
> IPOIB_ENTER( IPOIB_DBG_INIT );
>
> + ipoib_remove_adapter(p_adapter);
> +
> CL_ASSERT( p_adapter );
>
> /*
> Index: ulp/ipoib/kernel/ipoib_debug.h
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_debug.h (revision 385)
> +++ ulp/ipoib/kernel/ipoib_debug.h (working copy)
> @@ -59,7 +59,9 @@
> #define IPOIB_DBG_MCAST (1 << 7)
> #define IPOIB_DBG_ALLOC (1 << 8)
> #define IPOIB_DBG_OID (1 << 9)
> +#define IPOIB_DBG_IOCTL (1 << 10)
>
> +
> #define IPOIB_DBG_FUNC (1 << 28) /* For function entry/exit
> */
> #define IPOIB_DBG_INFO (1 << 29) /* For verbose information
> */
> #define IPOIB_DBG_WARN (1 << 30) /* For warnings. */
> Index: ulp/ipoib/kernel/ipoib_driver.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_driver.c (revision 385)
> +++ ulp/ipoib/kernel/ipoib_driver.c (working copy)
> @@ -29,13 +29,15 @@
> * $Id$
> */
>
> -
> +#include <iba/ib_types.h>
> +#include "ip_addresses_shared.h"
> #include "ipoib_driver.h"
> #include "ipoib_debug.h"
> #include "ipoib_port.h"
> #include <complib/cl_bus_ifc.h>
> #include <initguid.h>
> #include <iba/ipoib_ifc.h>
> +#include <ip_addresses.h>
>
>
> /* Default parameters values for optional parameters */
> @@ -119,7 +121,7 @@
>
>
> /* Global driver debug level */
> -uint32_t g_ipoib_dbg_lvl = IPOIB_DBG_ERROR;
> +uint32_t g_ipoib_dbg_lvl = IPOIB_DBG_ERROR | IPOIB_DBG_IOCTL |
> IPOIB_DBG_INIT |
> IPOIB_DBG_FUNC;
This should always be just IPOIB_DBG_ERROR for the code checked into SVN.
> atomic32_t g_laa_idx = 0;
>
>
> @@ -219,6 +221,15 @@
> IN ipoib_adapter_t*
> const p_adapter );
>
>
> +
> +VOID DriverUnload (
> + IN PDRIVER_OBJECT pDriverObject
> + )
> +{
> + IPOIB_ENTER( IPOIB_DBG_INIT );
> + pDriverObject = NULL;
> + ipoib_driver_unload();
> +}
> //! Standard Windows Device Driver Entry Point
> /*! DriverEntry is the first routine called after a driver is loaded, and
> is responsible for initializing the driver. On W2k this occurs when the
> PnP
> @@ -242,6 +253,8 @@
>
> IPOIB_ENTER( IPOIB_DBG_INIT );
>
> + p_drv_obj->DriverUnload = DriverUnload;
> +
> #ifdef _DEBUG_
> PAGED_CODE();
> #endif
> @@ -250,7 +263,6 @@
> ndis_handle = NULL;
>
> NdisMInitializeWrapper( &ndis_handle, p_drv_obj,
> p_registry_path, NULL );
> -
> memset(&characteristics, 0, sizeof(characteristics));
> characteristics.MajorNdisVersion =
> MAJOR_NDIS_VERSION;
> characteristics.MinorNdisVersion =
> MINOR_NDIS_VERSION;
> @@ -273,11 +285,26 @@
> ndis_handle, &characteristics,
> sizeof(characteristics) );
> if( status != NDIS_STATUS_SUCCESS )
> {
> +
> IPOIB_TRACE( IPOIB_DBG_ERROR,
> - ("NdisMRegisterMiniport failed with
> status of %s", status) );
> + ("NdisMRegisterMiniport failed with
> status of %d\n", status) );
> NdisTerminateWrapper( ndis_handle, NULL );
> + return status;
> + }
> +
> +
> + status = ipoib_driver_up(ndis_handle);
> + if( status != NDIS_STATUS_SUCCESS )
> + {
> + IPOIB_TRACE( IPOIB_DBG_ERROR,
> + ("ipoib_driver_up failed with status of
> %d\n", status) );
> + //??? should we "undo the" NdisMRegisterMiniport ?
> + NdisTerminateWrapper( ndis_handle, NULL );
> + return status;
Don't fail the driver loading just because address translation won't work. We
can still provide full connectivity via IPoIB even if SDP/WSD don't work.
> }
>
> + NdisMRegisterUnloadHandler(ndis_handle, DriverUnload);
> +
> IPOIB_EXIT( IPOIB_DBG_INIT );
> return status;
> }
> Index: ulp/ipoib/kernel/ipoib_endpoint.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_endpoint.c (revision 385)
> +++ ulp/ipoib/kernel/ipoib_endpoint.c (working copy)
> @@ -30,7 +30,8 @@
> */
>
>
> -
> +#include <iba/ib_types.h>
> +#include "ip_addresses_shared.h"
> #include "ipoib_endpoint.h"
> #include "ipoib_port.h"
> #include "ipoib_debug.h"
Why was this change needed? Is it because of moving the HW_ADDR_LEN definition?
> Index: ulp/ipoib/kernel/ipoib_port.c
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_port.c (revision 385)
> +++ ulp/ipoib/kernel/ipoib_port.c (working copy)
> @@ -30,7 +30,8 @@
> */
>
>
> -
> +#include <iba/ib_types.h>
> +#include "ip_addresses_shared.h"
> #include "ipoib_port.h"
> #include "ipoib_adapter.h"
> #include "ipoib_debug.h"
> @@ -3856,8 +3857,39 @@
> IPOIB_EXIT( IPOIB_DBG_ENDPT );
> }
>
> +NDIS_STATUS
> +gid_from_mac(
> + IN ipoib_port_t*
> const p_port,
> + IN const mac_addr_t
> mac,
> + OUT ib_gid_t*
> p_dest_gid )
> +{
> + ipoib_endpt_t* p_endpt;
> + cl_map_item_t *p_item;
> + uint64_t key = 0;
> + cl_memcpy( &key, &mac, sizeof(mac_addr_t) );
>
> -static inline NDIS_STATUS
> + IPOIB_ENTER( IPOIB_DBG_ENDPT );
> +
> + cl_obj_lock( &p_port->obj );
> +
> + p_item = cl_qmap_get( &p_port->endpt_mgr.mac_endpts, key );
> + if( p_item == cl_qmap_end( &p_port->endpt_mgr.mac_endpts ) )
> + {
> + cl_obj_unlock( &p_port->obj );
> + IPOIB_TRACE_EXIT( IPOIB_DBG_ENDPT, ("Failed endpoint
> lookup.\n") );
> + return NDIS_STATUS_NO_ROUTE_TO_DESTINATION;
> + }
> +
> + p_endpt = PARENT_STRUCT( p_item, ipoib_endpt_t, mac_item );
> + *p_dest_gid = p_endpt->dgid;
> +
> + cl_obj_unlock( &p_port->obj );
> +
> + IPOIB_EXIT( IPOIB_DBG_ENDPT );
> + return NDIS_STATUS_SUCCESS;
> +}
> +
> +NDIS_STATUS
> __endpt_mgr_ref(
> IN ipoib_port_t*
> const p_port,
> IN const mac_addr_t
> mac,
Why is this no longer static inline?
> @@ -3872,6 +3904,7 @@
> IPOIB_ENTER( IPOIB_DBG_ENDPT );
>
> key = 0;
> + *pp_endpt = NULL;
> cl_memcpy( &key, &mac, sizeof(mac_addr_t) );
>
> cl_obj_lock( &p_port->obj );
This function should actually be changed to not touch pp_endpt in case of
non-STATUS_SUCCESS.
> Index: ulp/ipoib/kernel/ipoib_port.h
> ===================================================================
> --- ulp/ipoib/kernel/ipoib_port.h (revision 385)
> +++ ulp/ipoib/kernel/ipoib_port.h (working copy)
> @@ -584,5 +584,10 @@
> ipoib_port_resume(
> IN ipoib_port_t*
> const p_port );
>
> +NDIS_STATUS
> +gid_from_mac(
> + IN ipoib_port_t*
> const p_port,
> + IN const mac_addr_t
> mac,
> + OUT ib_gid_t*
> p_dest_gid );
All non-static functions in the driver start with 'ipoib_'. This one should
follow suit.
>
> #endif /* _IPOIB_PORT_H_ */
> Index: ulp/ipoib/kernel/SOURCES
> ===================================================================
> --- ulp/ipoib/kernel/SOURCES (revision 385)
> +++ ulp/ipoib/kernel/SOURCES (working copy)
> @@ -6,7 +6,8 @@
> ipoib_driver.c \
> ipoib_adapter.c \
> ipoib_endpoint.c \
> - ipoib_port.c
> + ipoib_port.c \
> + ..\ip_Addresses.c
The path for this file is wrong. It is a kernel only file and should be in the
local directory. The patch file indicates that the file is indeed in the
correct directory, so maybe it got picked up by the build.
>
> INCLUDES=..;..\..\..\inc;..\..\..\inc\kernel;
>
More information about the ofw
mailing list