From f8a8e56595d56f7c48dfa6da35ba2a40822a8d93 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Tue, 12 Nov 2024 21:58:16 -0500 Subject: [PATCH] USB: Always delete transfers even when force-canceling. We can't call the callbacks (as the comments correctly indicated), but we can certainly delete the transfer objects, since they rightfully belong to us. Should fix #19242, #19241, #19180 and possibly other recent regressions. Also fixes a long-standing memory leak from this scenario. --- src/add-ons/kernel/busses/usb/ehci.cpp | 27 +++++------ src/add-ons/kernel/busses/usb/ohci.cpp | 62 +++++++++++++------------- src/add-ons/kernel/busses/usb/uhci.cpp | 27 +++++------ src/add-ons/kernel/busses/usb/xhci.cpp | 15 ++++--- 4 files changed, 68 insertions(+), 63 deletions(-) diff --git a/src/add-ons/kernel/busses/usb/ehci.cpp b/src/add-ons/kernel/busses/usb/ehci.cpp index 7f3c3c10df..d7c16694dd 100644 --- a/src/add-ons/kernel/busses/usb/ehci.cpp +++ b/src/add-ons/kernel/busses/usb/ehci.cpp @@ -1761,18 +1761,13 @@ EHCI::CancelQueuedTransfers(Pipe *pipe, bool force) descriptor = descriptor->next_log; } - if (!force) { - // if the transfer is canceled by force, the one causing the - // cancel is probably not the one who initiated the transfer - // and the callback is likely not safe anymore - transfer_entry *entry - = (transfer_entry *)malloc(sizeof(transfer_entry)); - if (entry != NULL) { - entry->transfer = current->transfer; - current->transfer = NULL; - entry->next = list; - list = entry; - } + transfer_entry *entry + = (transfer_entry *)malloc(sizeof(transfer_entry)); + if (entry != NULL) { + entry->transfer = current->transfer; + current->transfer = NULL; + entry->next = list; + list = entry; } current->canceled = true; @@ -1785,7 +1780,13 @@ EHCI::CancelQueuedTransfers(Pipe *pipe, bool force) while (list != NULL) { transfer_entry *next = list->next; - list->transfer->Finished(B_CANCELED, 0); + + // if the transfer is canceled by force, the one causing the + // cancel is possibly not the one who initiated the transfer + // and the callback is likely not safe anymore + if (!force) + list->transfer->Finished(B_CANCELED, 0); + delete list->transfer; free(list); list = next; diff --git a/src/add-ons/kernel/busses/usb/ohci.cpp b/src/add-ons/kernel/busses/usb/ohci.cpp index 26a1b7f302..257974ffa1 100644 --- a/src/add-ons/kernel/busses/usb/ohci.cpp +++ b/src/add-ons/kernel/busses/usb/ohci.cpp @@ -704,38 +704,34 @@ OHCI::CancelQueuedTransfers(Pipe *pipe, bool force) current->endpoint->head_physical_descriptor = current->endpoint->tail_physical_descriptor; - if (!force) { - if (pipe->Type() & USB_OBJECT_ISO_PIPE) { - ohci_isochronous_td *descriptor - = (ohci_isochronous_td *)current->first_descriptor; - while (descriptor) { - uint16 frame = OHCI_ITD_GET_STARTING_FRAME( - descriptor->flags); - _ReleaseIsochronousBandwidth(frame, - OHCI_ITD_GET_FRAME_COUNT(descriptor->flags)); - if (descriptor - == (ohci_isochronous_td*)current->last_descriptor) - // this is the last ITD of the transfer - break; + if (pipe->Type() & USB_OBJECT_ISO_PIPE) { + ohci_isochronous_td *descriptor + = (ohci_isochronous_td *)current->first_descriptor; + while (descriptor) { + uint16 frame = OHCI_ITD_GET_STARTING_FRAME( + descriptor->flags); + _ReleaseIsochronousBandwidth(frame, + OHCI_ITD_GET_FRAME_COUNT(descriptor->flags)); + if (descriptor + == (ohci_isochronous_td*)current->last_descriptor) + // this is the last ITD of the transfer + break; - descriptor - = (ohci_isochronous_td *) - descriptor->next_done_descriptor; - } - } - - // If the transfer is canceled by force, the one causing the - // cancel is probably not the one who initiated the transfer - // and the callback is likely not safe anymore - transfer_entry *entry - = (transfer_entry *)malloc(sizeof(transfer_entry)); - if (entry != NULL) { - entry->transfer = current->transfer; - current->transfer = NULL; - entry->next = list; - list = entry; + descriptor + = (ohci_isochronous_td *) + descriptor->next_done_descriptor; } } + + transfer_entry *entry + = (transfer_entry *)malloc(sizeof(transfer_entry)); + if (entry != NULL) { + entry->transfer = current->transfer; + current->transfer = NULL; + entry->next = list; + list = entry; + } + current->canceled = true; } current = current->link; @@ -745,7 +741,13 @@ OHCI::CancelQueuedTransfers(Pipe *pipe, bool force) while (list != NULL) { transfer_entry *next = list->next; - list->transfer->Finished(B_CANCELED, 0); + + // If the transfer is canceled by force, the one causing the + // cancel is possibly not the one who initiated the transfer + // and the callback is likely not safe anymore + if (!force) + list->transfer->Finished(B_CANCELED, 0); + delete list->transfer; free(list); list = next; diff --git a/src/add-ons/kernel/busses/usb/uhci.cpp b/src/add-ons/kernel/busses/usb/uhci.cpp index 8ffaf0b0f5..227cac1c87 100644 --- a/src/add-ons/kernel/busses/usb/uhci.cpp +++ b/src/add-ons/kernel/busses/usb/uhci.cpp @@ -1006,18 +1006,13 @@ UHCI::CancelQueuedTransfers(Pipe *pipe, bool force) descriptor = (uhci_td *)descriptor->link_log; } - if (!force) { - // if the transfer is canceled by force, the one causing the - // cancel is probably not the one who initiated the transfer - // and the callback is likely not safe anymore - transfer_entry *entry - = (transfer_entry *)malloc(sizeof(transfer_entry)); - if (entry != NULL) { - entry->transfer = current->transfer; - current->transfer = NULL; - entry->next = list; - list = entry; - } + transfer_entry *entry + = (transfer_entry *)malloc(sizeof(transfer_entry)); + if (entry != NULL) { + entry->transfer = current->transfer; + current->transfer = NULL; + entry->next = list; + list = entry; } current->canceled = true; @@ -1029,7 +1024,13 @@ UHCI::CancelQueuedTransfers(Pipe *pipe, bool force) while (list != NULL) { transfer_entry *next = list->next; - list->transfer->Finished(B_CANCELED, 0); + + // if the transfer is canceled by force, the one causing the + // cancel is possibly not the one who initiated the transfer + // and the callback is likely not safe anymore + if (!force) + list->transfer->Finished(B_CANCELED, 0); + delete list->transfer; free(list); list = next; diff --git a/src/add-ons/kernel/busses/usb/xhci.cpp b/src/add-ons/kernel/busses/usb/xhci.cpp index 598d8344f2..754da4a565 100644 --- a/src/add-ons/kernel/busses/usb/xhci.cpp +++ b/src/add-ons/kernel/busses/usb/xhci.cpp @@ -1117,12 +1117,8 @@ XHCI::CancelQueuedTransfers(Pipe *pipe, bool force) if (td->transfer == NULL) continue; - // We can't cancel or delete transfers under "force", as they probably - // are not safe to use anymore. - if (!force) { - transfers[transfersCount] = td->transfer; - transfersCount++; - } + transfers[transfersCount] = td->transfer; + transfersCount++; td->transfer = NULL; } @@ -1179,7 +1175,12 @@ XHCI::CancelQueuedTransfers(Pipe *pipe, bool force) endpointLocker.Unlock(); for (int32 i = 0; i < transfersCount; i++) { - transfers[i]->Finished(B_CANCELED, 0); + // If the transfer is canceled by force, the one causing the + // cancel is possibly not the one who initiated the transfer + // and the callback is likely not safe anymore. + if (!force) + transfers[i]->Finished(B_CANCELED, 0); + delete transfers[i]; }