[ofw] RE: IBBUS - keep last HCA reference until IBAL is shutdown.

[Sean Hefty]

To highlight my confusion...  see inline comments

static void
fdo_release_resources(
	IN					DEVICE_OBJECT* const
p_dev_obj )
{
	bus_fdo_ext_t	*p_ext;
	NTSTATUS		status;
	bus_filter_t	*p_bfi;
	int				ic;
	ib_api_status_t	ib_status;

	BUS_ENTER( BUS_DBG_PNP );

	p_ext = p_dev_obj->DeviceExtension;
	ic = get_bfi_count();

	p_bfi = p_ext->bus_filter;
	CL_ASSERT( p_bfi );

	//TODO: Fail outstanding I/O operations.

	ib_status = ib_deregister_ca( p_ext->hca_ifc.Verbs.guid );

>> This references the hca_ifc.

	if( ib_status != IB_SUCCESS ) {
		BUS_PRINT( BUS_DBG_ERROR, ("ib_deregister_ca returned %s.\n",
			ib_get_err_str(ib_status)) );
	}

	if ( p_ext->p_port_mgr && p_bfi->p_port_mgr ) {
		cl_obj_destroy( &p_ext->p_port_mgr->obj );
		p_ext->p_port_mgr = NULL;
	}

	if ( p_ext->p_iou_mgr && p_bfi->p_iou_mgr ) {
	 	cl_obj_destroy( &p_ext->p_iou_mgr->obj );
		p_ext->p_iou_mgr = NULL;
	}

	if ( ic > 0 && p_ext->hca_ifc_taken ) {

>> This checks to see if we've taken the HCA interface.  But we just
>> accessed it above. 

		p_ext->hca_ifc.InterfaceHeader.InterfaceDereference(
			p_ext->hca_ifc.InterfaceHeader.Context);
		p_ext->hca_ifc_taken = FALSE;
	}

	BUS_TRACE( BUS_DBG_PNP, ("Releasing BusFilter %s\n", p_bfi->whoami ));
	if (p_bfi) {
		p_ext->bus_filter = NULL;
		p_bfi->p_bus_ext = NULL;
	}

	ic = free_bfi( p_bfi );

	/* if not last Buf Filter Instance, then exit, otherwise
cleanup/shutdown */
	if ( ic > 0 ) {
		BUS_TRACE( BUS_DBG_PNP, ("%d remaining BusFilters\n", ic ));
		return;
	}

	IoDeleteSymbolicLink( &g_CDO_dos_name );

	/* Disable any exported interfaces. */
	status = IoSetDeviceInterfaceState( &al_ifc_name, FALSE );
	ASSERT( NT_SUCCESS( status ) );
	status = IoSetDeviceInterfaceState( &ci_ifc_name, FALSE );
	ASSERT( NT_SUCCESS( status ) );
	status = IoSetDeviceInterfaceState( &cm_ifc_name, FALSE );
	ASSERT( NT_SUCCESS( status ) );

	/* Release the memory allocated for the interface symbolic names. */
	RtlFreeUnicodeString( &cm_ifc_name );
	RtlFreeUnicodeString( &ci_ifc_name );
	RtlFreeUnicodeString( &al_ifc_name );

	al_cleanup();
	cl_thread_suspend(50);	/* allow time for AL's async procs to run

>> Using a sleep is always racy.  How do we know that AL is done?

	ASSERT( !gp_async_proc_mgr && !gp_async_pnp_mgr && !gp_al_mgr );

	/* AL needs the HCA to stick around until AL cleanup has completed.
	 * Now that it's done, let the HCA fade away.
	 */
	if ( p_ext->hca_ifc_taken ) {

>> A second location where the interface is dereferenced.  This is the
>> only dereference that makes sense to me based on a brief inspection
>> of the code.  The if check implies that it's okay to call al_cleanup
>> without holding a reference on the interface.

>> The code would make more sense to me if we removed the dereference
>> above, simply returned if ic > 0, and only release it here.
>> The call above to ib_deregister_ca looks like it needs a check to
>> see if the interface is valid.

		p_ext->hca_ifc.InterfaceHeader.InterfaceDereference(
			p_ext->hca_ifc.InterfaceHeader.Context);
		p_ext->hca_ifc_taken = FALSE;
	}

	BUS_EXIT( BUS_DBG_PNP );
}

>> Again, I didn't try to understand everything that was happening.  The
>> overall handling just looks sketchy and seems like it could be simplified.

- Sean