[Openib-windows] RE: Some more additions questions about the patch:

[Fab Tillier]

> From: Tzachi Dar [mailto:tzachid at mellanox.co.il]
> Sent: Sunday, September 18, 2005 1:05 PM
> 
> While working on checking my patch with a kernel mode client I have found
> out that there are some more things that should be changed in the IPOIB
> code.
> 
> 1)     gp_mem_tracker: It seems that there is a call to __cl_mem_track_start
> every time there is an adapter that is being created, and a similar call
> when an adapter is being destroyed. It is not clear to me, why this is so
> and you are not doing this in the Driver-Entry Driver Unload functions.

When I made complib a static library, I had forgotten that NDIS drivers could
register unload handlers.  I added code to let repeated calls to CL_INIT and
CL_DEINIT work as long as they were never done in parallel (which they aren't
due to how PnP IRPs are handled).

> In any case if you keep things as they are, please note that on
> __cl_mem_track_stop you have to set gp_mem_tracker to null if the reference
> has reached zero, otherwise if the reference has reached zero, things won't
> work again.

This most certainly is a bug, thanks for reporting it.
 
> (by the way, if you keep things as they are things won't work in
> any case, since there is a race between the reference count and the global
> variable :-(, it probably must be done in DriverEntry)

The race between reference and destruction isn't an issue due to how the devices
are initialized in Windows.  I don't think two instances of the same device can
receive PnP IRPs simultaneously.  It still sucks, though

I'm currently adding in functionality needed by the AT stuff that should be
there anyway - the first was to put in the list of adapters, which is already
committed.  The second is to add the unload handler (which I will commit
tomorrow), and from that unload handler call CL_DEINIT, and move the CL_INIT
call to driver entry (solving the race).  I think IPoIB is the only driver that
was doing this, and with this change I will remove the changes I had introduced
to allow nested calls to CL_INIT and CL_DEINIT, as they are buggy.

For the kernel, I'm considering going a step further an eliminating the memory
tracker - driver verifier has memory leak detection built in, and we might as
well just use that instead.  The memory leak detection provided by driver
verifier doesn't require any sort of recompile of the drivers, which is an extra
benefit.

> 2)     I have moved the place in which the adapter is being created. It
> should better be created when the number of adapters in the list goes from 0
> to 1, This is the case in which there is someone holding the device, the
> IPOIB ports are being disabled and re-enabled again.

You mean when you register the device with NDIS for IOCTL access, or the IPoIB
adapter object?  Can't we register the device from DriverEntry, and deregister
it from the unload handler?  The docs for the calls seem to indicate that's the
common usage model.  I don't think the system will invoke the unload handler
until all file handles have been closed.  This would simplify things a great
deal, wouldn't it?

> 3)     Please add the case of IRP_MJ_INTERNAL_DEVICE_CONTROL to the function
> __ipoib_dispatch. It should be handled just like IRP_MJ_DEVICE_CONTROL.

Why don't kernel drivers use IRP_MJ_DEVICE_CONTROL?  Why use
IRP_MJ_INTERNAL_DEVICE_CONTROL?

Thanks,

- Fab