@Tim Roberts:
Doesn’t an IRP built by IoBuildDeviceIoControlRequest have an extra stack location with a completion routine that does all the cleanup, copies the status and sets the event for you?
For the OP: iostatus will only be set when the IRP completion is allowed to run to the end. If your completion routine returns STATUS_MORE_COMPLETION_REQUIRED, iostatus is unchanged.
xxxxx@broadcom.com wrote:
@Tim Roberts:
Doesn’t an IRP built by IoBuildDeviceIoControlRequest have an extra stack location with a completion routine that does all the cleanup, copies the status and sets the event for you?
It certainly sets up a default completion routine for the “current”
stack location, so that the creating driver doesn’t usually need to
supply one. I don’t know whether it is implemented as an “extra” stack
location that can be used even if I override it with my own. If you
look at the samples that use IoBuildDeviceIoControlRequest, none of them
ever complete the IRP. I do see some of the completion routines
returning STATUS_SUCCESS instead of STATUS_MORE_PROCESSING_REQUIRED, so
perhaps you are correct.
For the OP: iostatus will only be set when the IRP completion is allowed to run to the end. If your completion routine returns STATUS_MORE_COMPLETION_REQUIRED, iostatus is unchanged.
Yes, for the parameter passed to IoBuildDeviceIoControlRequest, that’s
so. Irp->IoStatus.Status will already be updated.
–
Tim Roberts, xxxxx@probo.com
Providenza & Boekelheide, Inc.
> Doesn’t an IRP built by IoBuildDeviceIoControlRequest have an extra stack location with a
completion routine that does all the cleanup, copies the status and sets the event for you?
No, IopCompleteRequest APC does this.
–
Maxim S. Shatskih
Windows DDK MVP
xxxxx@storagecraft.com
http://www.storagecraft.com
IoBuildXxx IRPs are threaded irps. As such they do need to be completed back to the io manager so that the io manager can free them.
d
> IoBuildXxx IRPs are threaded irps.
IoBuildAsychronousXxx is not.
–
Maxim S. Shatskih
Windows DDK MVP
xxxxx@storagecraft.com
http://www.storagecraft.com
Hi all
Thanks again, the last few comments have gone over my head I’m afraid… I have reviewed our SendAwaitUrb code and with the help of the msdn cheat sheets, reworked the function. It seems to work fine but if anyone has any further comments, I’d be pleased to hear them.
Sean
typedef enum {
IRPLOCK_CANCELABLE,
IRPLOCK_CANCEL_STARTED,
IRPLOCK_CANCEL_COMPLETE,
IRPLOCK_COMPLETED
} IRPLOCK;
NTSTATUS SendAwaitUrbOnComplete(PDEVICE_OBJECT pdo, PIRP Irp, PVOID pev)
{
PLONG lock;
lock = (PLONG) pev;
if (InterlockedExchange((PVOID)&lock, IRPLOCK_COMPLETED) == IRPLOCK_CANCEL_STARTED) {
//
// Main line code has got the control of the IRP. It will
// now take the responsibility of completing the IRP.
// Therefore…
return STATUS_MORE_PROCESSING_REQUIRED;
}
return STATUS_CONTINUE_COMPLETION ;
}
NTSTATUS SendAwaitUrb(IN PDEVICE_OBJECT LowerDeviceObject, PURB urb)
{
KEVENT event;
IO_STATUS_BLOCK iostatus={0};
PIRP Irp;
PIO_STACK_LOCATION stack;
NTSTATUS ntStatus;
IRPLOCK lock = IRPLOCK_CANCELABLE;;
KeInitializeEvent(&event, NotificationEvent, FALSE);
Irp = IoBuildDeviceIoControlRequest(IOCTL_INTERNAL_USB_SUBMIT_URB, LowerDeviceObject, NULL, 0, NULL, 0, TRUE, &event, &iostatus);
if (!Irp)
{
ser_string(“Unable to allocate IRP for sending URB\n”);
return STATUS_INSUFFICIENT_RESOURCES;
}
IoSetCompletionRoutine(Irp, SendAwaitUrbOnComplete, (PVOID) &event, TRUE, TRUE, TRUE);
stack = IoGetNextIrpStackLocation(Irp);
ASSERT(stack != NULL);
stack->Parameters.Others.Argument1 = (PVOID) urb;
ntStatus = IoCallDriver(LowerDeviceObject, Irp);
if(!NT_SUCCESS(ntStatus))
{
dlDbgPrint(FUNCTION", IoCallDriver failed ntStatus %lx, iostatus.Status %lx, urb->UrbHeader.Status %lx\r\n",ntStatus,iostatus.Status,urb->UrbHeader.Status);
}
if (ntStatus == STATUS_PENDING)
{
LARGE_INTEGER Timeout;
Timeout.QuadPart = -1 * 10000000;
dlDbgPrint(FUNCTION", IoCallDriver returned STATUS_PENDING\r\n");
ASSERT_KEWAIT_IRQL(Timeout.QuadPart);
ntStatus = KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, &Timeout);
if ( ntStatus == STATUS_TIMEOUT)
{
dlDbgPrint(FUNCTION", KeWaitForSingleObject returned STATUS_TIMEOUT\r\n");
if (InterlockedExchange((PVOID)&lock, IRPLOCK_CANCEL_STARTED) == IRPLOCK_CANCELABLE)
{
dlDbgPrint(FUNCTION", lock is IRPLOCK_CANCELABLE, calling IoCancelIrp\r\n");
IoCancelIrp(Irp); // okay in this context
if (InterlockedExchange((PVOID) &lock, IRPLOCK_CANCEL_COMPLETE) == IRPLOCK_COMPLETED)
{
dlDbgPrint(FUNCTION", lock is IRPLOCK_COMPLETED, calling IoCompleteRequest\r\n");
IoCompleteRequest(Irp, IO_NO_INCREMENT);
}
}
KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL);
iostatus.Status = ntStatus; // Return STATUS_TIMEOUT
}
else
{
ntStatus = iostatus.Status;
}
if (!NT_SUCCESS(ntStatus))
{
ser_print(FUNCTION", iostatus 0x%08lx\r\n",ntStatus);
}
}
return ntStatus;
}
your code is way too complex. no need for any of the interlock logic (and if you kept it, you should really use InterlockedCompareExchange to make sure you are changing from a well known and expected state vs just smashing in a new state value without regard for the old state).
since the request is synchronous, the completion routine should always set the event and return STATUS_MORE_PROCESSING_REQUIRED. in the send routine, if the initial timeout expries, cancel the request and then infinitely wait. Always complete the request in the send routine, not the completion routine.
d
Hi Doron
I used the code from “Scenario 7: Send a synchronous device-control (IOCTL) request and cancel it if not completed in a certain time period” at http://support.microsoft.com/kb/326315 as it seemed to encapsulated all that we wanted to do and managed the failures that we can experience with the device.
My previous version of this function always returned STATUS_MORE_PROCESSING_REQ and completed in the send routine but feedback suggested this was not the best way forward and I’m now even more confused (if possible!).
Sean