USB: Track object busy-ness and wait for an unbusy state before teardown.

This reverts and replaces hrev53141, hrev53200~1, and hrev53888.

Two years ago (in hrev53141), I added checks to validate that Devices
were not in the process of being torn down before using them, to fix
a race condition KDL. Further logic was added in hrev53200~1 and in
hrev53888 for Pipes.

Well, upon closer inspection following the reports of #16794 et al.,
it appears upon closer inspection there were still two more race
conditions lurking in there: the first between Get and InitCheck, and
the second between InitCheck and use.

To resolve both of these, a new atomic "busy" flag is added to objects,
which is incremented before unlocking the objects array, and then
waited on before actually proceeding with teardown.

The older checks about initialization status are now superfluous
and are removed in favor of an earlier PutUSBID() invocation in Device.

While #16794 was fixed by hrev55429, some of those or related KDLs
might have been caused by these races. This also re-resolves #15115,
along with #14949 and #15710.

Change-Id: Ifcae84945a81123af5ef4683a6e33dc1eec5b23c
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4421
Reviewed-by: waddlesplash <[email protected]>
This commit is contained in:
Augustin Cavalier
2021-09-17 18:25:00 +00:00
committed by waddlesplash
parent b3683d8aee
commit 5ee918678e
7 changed files with 149 additions and 111 deletions
@@ -326,9 +326,6 @@ Device::Device(Object* parent, int8 hubAddress, uint8 hubPort,
Device::~Device()
{
// Unset fInitOK to indicate we are tearing down.
fInitOK = false;
// Destroy open endpoints. Do not send a device request to unconfigure
// though, since we may be deleted because the device was unplugged already.
Unconfigure(false);
@@ -347,7 +344,11 @@ Device::~Device()
for (size_t k = 0; k < interfaceList->alt_count; k++) {
usb_interface_info* interface = &interfaceList->alt[k];
delete (Interface*)GetStack()->GetObject(interface->handle);
Interface* interfaceObject =
(Interface*)GetStack()->GetObject(interface->handle);
if (interfaceObject != NULL)
interfaceObject->SetBusy(false);
delete interfaceObject;
interface->handle = 0;
}
}
@@ -629,7 +630,10 @@ Device::ClearEndpoints(int32 interfaceIndex)
for (size_t i = 0; i < interfaceInfo->endpoint_count; i++) {
usb_endpoint_info* endpoint = &interfaceInfo->endpoint[i];
delete (Pipe*)GetStack()->GetObject(endpoint->handle);
Pipe* pipe = (Pipe*)GetStack()->GetObject(endpoint->handle);
if (pipe != NULL)
pipe->SetBusy(false);
delete pipe;
endpoint->handle = 0;
}
}
@@ -76,6 +76,7 @@ Hub::Hub(Object *parent, int8 hubAddress, uint8 hubPort,
} else {
TRACE_ALWAYS("no interrupt pipe found\n");
}
object->SetBusy(false);
// Wait some time before powering up the ports
if (!isRootHub)
+15 -4
View File
@@ -13,7 +13,8 @@ Object::Object(Stack *stack, BusManager *bus)
: fParent(NULL),
fBusManager(bus),
fStack(stack),
fUSBID(fStack->GetUSBID(this))
fUSBID(fStack->GetUSBID(this)),
fBusy(0)
{
}
@@ -22,7 +23,8 @@ Object::Object(Object *parent)
: fParent(parent),
fBusManager(parent->GetBusManager()),
fStack(parent->GetStack()),
fUSBID(fStack->GetUSBID(this))
fUSBID(fStack->GetUSBID(this)),
fBusy(0)
{
}
@@ -36,8 +38,17 @@ Object::~Object()
void
Object::PutUSBID()
{
if (fUSBID != UINT32_MAX)
fStack->PutUSBID(this);
if (fUSBID == UINT32_MAX)
return;
fStack->PutUSBID(this);
int32 retries = 20;
while (atomic_get(&fBusy) != 0 && retries--)
snooze(100);
if (retries <= 0)
panic("USB object did not become unbusy!");
fUSBID = UINT32_MAX;
}
+3 -15
View File
@@ -84,11 +84,7 @@ status_t
Pipe::SetFeature(uint16 selector)
{
TRACE("set feature %u\n", selector);
Device *device = (Device *)Parent();
if (device->InitCheck() != B_OK)
return B_NO_INIT;
return device->DefaultPipe()->SendRequest(
return ((Device *)Parent())->DefaultPipe()->SendRequest(
USB_REQTYPE_STANDARD | USB_REQTYPE_ENDPOINT_OUT,
USB_REQUEST_SET_FEATURE,
selector,
@@ -104,16 +100,12 @@ Pipe::SetFeature(uint16 selector)
status_t
Pipe::ClearFeature(uint16 selector)
{
Device *device = (Device *)Parent();
if (device->InitCheck() != B_OK)
return B_NO_INIT;
// clearing a stalled condition resets the data toggle
if (selector == USB_FEATURE_ENDPOINT_HALT)
SetDataToggle(false);
TRACE("clear feature %u\n", selector);
return device->DefaultPipe()->SendRequest(
return ((Device *)Parent())->DefaultPipe()->SendRequest(
USB_REQTYPE_STANDARD | USB_REQTYPE_ENDPOINT_OUT,
USB_REQUEST_CLEAR_FEATURE,
selector,
@@ -130,11 +122,7 @@ status_t
Pipe::GetStatus(uint16 *status)
{
TRACE("get status\n");
Device *device = (Device *)Parent();
if (device->InitCheck() != B_OK)
return B_NO_INIT;
return device->DefaultPipe()->SendRequest(
return ((Device *)Parent())->DefaultPipe()->SendRequest(
USB_REQTYPE_STANDARD | USB_REQTYPE_ENDPOINT_IN,
USB_REQUEST_GET_STATUS,
0,
@@ -227,6 +227,9 @@ Stack::GetObject(usb_id id)
Object *result = fObjectArray[id];
if (result != NULL)
result->SetBusy(true);
Unlock();
return result;
}
+112 -87
View File
@@ -194,6 +194,49 @@ bus_std_ops(int32 op, ...)
}
// #pragma mark - ObjectBusyReleaser
class ObjectBusyReleaser {
public:
ObjectBusyReleaser(Object* object) : fObject(object) {}
~ObjectBusyReleaser()
{
Release();
}
void Release()
{
if (fObject != NULL) {
fObject->SetBusy(false);
fObject = NULL;
}
}
inline bool IsSet() const
{
return fObject != NULL;
}
inline Object *Get() const
{
return fObject;
}
inline Object *operator->() const
{
return fObject;
}
private:
Object *fObject;
};
// #pragma mark - public methods
status_t
register_driver(const char *driverName,
const usb_support_descriptor *descriptors,
@@ -222,12 +265,10 @@ const usb_device_descriptor *
get_device_descriptor(usb_device dev)
{
TRACE_MODULE("get_device_descriptor(%" B_PRId32 ")\n", dev);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
return NULL;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return NULL;
Device *device = (Device *)object.Get();
return device->DeviceDescriptor();
}
@@ -238,12 +279,10 @@ get_nth_configuration(usb_device dev, uint32 index)
{
TRACE_MODULE("get_nth_configuration(%" B_PRId32 ", %" B_PRIu32 ")\n",
dev, index);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
return NULL;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return NULL;
Device *device = (Device *)object.Get();
return device->ConfigurationAt((int32)index);
}
@@ -253,12 +292,10 @@ const usb_configuration_info *
get_configuration(usb_device dev)
{
TRACE_MODULE("get_configuration(%" B_PRId32 ")\n", dev);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
return NULL;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return NULL;
Device *device = (Device *)object.Get();
return device->Configuration();
}
@@ -270,12 +307,10 @@ set_configuration(usb_device dev,
{
TRACE_MODULE("set_configuration(%" B_PRId32 ", %p)\n", dev,
configuration);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return B_DEV_INVALID_PIPE;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
return B_NO_INIT;
Device *device = (Device *)object.Get();
return device->SetConfiguration(configuration);
}
@@ -285,12 +320,10 @@ status_t
set_alt_interface(usb_device dev, const usb_interface_info *interface)
{
TRACE_MODULE("set_alt_interface(%" B_PRId32 ", %p)\n", dev, interface);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return B_DEV_INVALID_PIPE;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
return B_NO_INIT;
Device *device = (Device *)object.Get();
return device->SetAltInterface(interface);
}
@@ -300,8 +333,8 @@ status_t
set_feature(usb_id handle, uint16 selector)
{
TRACE_MODULE("set_feature(%" B_PRId32 ", %d)\n", handle, selector);
Object *object = gUSBStack->GetObject(handle);
if (!object)
ObjectBusyReleaser object(gUSBStack->GetObject(handle));
if (!object.IsSet())
return B_DEV_INVALID_PIPE;
return object->SetFeature(selector);
@@ -312,8 +345,8 @@ status_t
clear_feature(usb_id handle, uint16 selector)
{
TRACE_MODULE("clear_feature(%" B_PRId32 ", %d)\n", handle, selector);
Object *object = gUSBStack->GetObject(handle);
if (!object)
ObjectBusyReleaser object(gUSBStack->GetObject(handle));
if (!object.IsSet())
return B_DEV_INVALID_PIPE;
return object->ClearFeature(selector);
@@ -327,8 +360,8 @@ get_status(usb_id handle, uint16 *status)
if (!status)
return B_BAD_VALUE;
Object *object = gUSBStack->GetObject(handle);
if (!object)
ObjectBusyReleaser object(gUSBStack->GetObject(handle));
if (!object.IsSet())
return B_DEV_INVALID_PIPE;
return object->GetStatus(status);
@@ -342,12 +375,10 @@ get_descriptor(usb_device dev, uint8 type, uint8 index, uint16 languageID,
TRACE_MODULE("get_descriptor(%" B_PRId32 ", 0x%02x, 0x%02x, 0x%04x, %p, "
"%" B_PRIuSIZE ", %p)\n",
dev, type, index, languageID, data, dataLength, actualLength);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return B_DEV_INVALID_PIPE;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
return B_NO_INIT;
Device *device = (Device *)object.Get();
return device->GetDescriptor(type, index, languageID,
data, dataLength, actualLength);
@@ -361,12 +392,10 @@ send_request(usb_device dev, uint8 requestType, uint8 request,
TRACE_MODULE("send_request(%" B_PRId32 ", 0x%02x, 0x%02x, 0x%04x, 0x%04x, "
"%d, %p, %p)\n", dev, requestType, request, value, index, length,
data, actualLength);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return B_DEV_INVALID_PIPE;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
return B_NO_INIT;
Device *device = (Device *)object.Get();
return device->DefaultPipe()->SendRequest(requestType, request,
value, index, length, data, length, actualLength);
@@ -381,12 +410,10 @@ queue_request(usb_device dev, uint8 requestType, uint8 request,
TRACE_MODULE("queue_request(%" B_PRId32 ", 0x%02x, 0x%02x, 0x%04x, 0x%04x,"
" %u, %p, %p, %p)\n", dev, requestType, request, value, index,
length, data, callback, callbackCookie);
Object *object = gUSBStack->GetObject(dev);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(dev));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return B_DEV_INVALID_PIPE;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
return B_NO_INIT;
Device *device = (Device *)object.Get();
return device->DefaultPipe()->QueueRequest(requestType,
request, value, index, length, data, length, callback, callbackCookie);
@@ -399,12 +426,12 @@ queue_interrupt(usb_pipe pipe, void *data, size_t dataLength,
{
TRACE_MODULE("queue_interrupt(%" B_PRId32 ", %p, %ld, %p, %p)\n",
pipe, data, dataLength, callback, callbackCookie);
Object *object = gUSBStack->GetObject(pipe);
if (!object || (object->Type() & USB_OBJECT_INTERRUPT_PIPE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(pipe));
if (!object.IsSet() || (object->Type() & USB_OBJECT_INTERRUPT_PIPE) == 0)
return B_DEV_INVALID_PIPE;
return ((InterruptPipe *)object)->QueueInterrupt(data, dataLength, callback,
callbackCookie);
return ((InterruptPipe *)object.Get())->QueueInterrupt(data, dataLength,
callback, callbackCookie);
}
@@ -414,11 +441,11 @@ queue_bulk(usb_pipe pipe, void *data, size_t dataLength,
{
TRACE_MODULE("queue_bulk(%" B_PRId32 ", %p, %" B_PRIuSIZE ", %p, %p)\n",
pipe, data, dataLength, callback, callbackCookie);
Object *object = gUSBStack->GetObject(pipe);
if (!object || (object->Type() & USB_OBJECT_BULK_PIPE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(pipe));
if (!object.IsSet() || (object->Type() & USB_OBJECT_BULK_PIPE) == 0)
return B_DEV_INVALID_PIPE;
return ((BulkPipe *)object)->QueueBulk(data, dataLength, callback,
return ((BulkPipe *)object.Get())->QueueBulk(data, dataLength, callback,
callbackCookie);
}
@@ -429,12 +456,12 @@ queue_bulk_v(usb_pipe pipe, iovec *vector, size_t vectorCount,
{
TRACE_MODULE("queue_bulk_v(%" B_PRId32 ", %p, %" B_PRIuSIZE " %p, %p)\n",
pipe, vector, vectorCount, callback, callbackCookie);
Object *object = gUSBStack->GetObject(pipe);
if (!object || (object->Type() & USB_OBJECT_BULK_PIPE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(pipe));
if (!object.IsSet() || (object->Type() & USB_OBJECT_BULK_PIPE) == 0)
return B_DEV_INVALID_PIPE;
return ((BulkPipe *)object)->QueueBulkV(vector, vectorCount, callback,
callbackCookie, false);
return ((BulkPipe *)object.Get())->QueueBulkV(vector, vectorCount,
callback, callbackCookie, false);
}
@@ -444,12 +471,12 @@ queue_bulk_v_physical(usb_pipe pipe, iovec *vector, size_t vectorCount,
{
TRACE_MODULE("queue_bulk_v_physical(%" B_PRId32 ", %p, %" B_PRIuSIZE
", %p, %p)\n", pipe, vector, vectorCount, callback, callbackCookie);
Object *object = gUSBStack->GetObject(pipe);
if (!object || (object->Type() & USB_OBJECT_BULK_PIPE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(pipe));
if (!object.IsSet() || (object->Type() & USB_OBJECT_BULK_PIPE) == 0)
return B_DEV_INVALID_PIPE;
return ((BulkPipe *)object)->QueueBulkV(vector, vectorCount, callback,
callbackCookie, true);
return ((BulkPipe *)object.Get())->QueueBulkV(vector, vectorCount,
callback, callbackCookie, true);
}
@@ -463,11 +490,11 @@ queue_isochronous(usb_pipe pipe, void *data, size_t dataLength,
"%" B_PRId32 ", %p, 0x%08" B_PRIx32 ", %p, %p)\n",
pipe, data, dataLength, packetDesc, packetCount, startingFrameNumber,
flags, callback, callbackCookie);
Object *object = gUSBStack->GetObject(pipe);
if (!object || (object->Type() & USB_OBJECT_ISO_PIPE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(pipe));
if (!object.IsSet() || (object->Type() & USB_OBJECT_ISO_PIPE) == 0)
return B_DEV_INVALID_PIPE;
return ((IsochronousPipe *)object)->QueueIsochronous(data, dataLength,
return ((IsochronousPipe *)object.Get())->QueueIsochronous(data, dataLength,
packetDesc, packetCount, startingFrameNumber, flags, callback,
callbackCookie);
}
@@ -479,11 +506,11 @@ set_pipe_policy(usb_pipe pipe, uint8 maxQueuedPackets,
{
TRACE_MODULE("set_pipe_policy(%" B_PRId32 ", %d, %d, %d)\n", pipe,
maxQueuedPackets, maxBufferDurationMS, sampleSize);
Object *object = gUSBStack->GetObject(pipe);
if (!object || (object->Type() & USB_OBJECT_ISO_PIPE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(pipe));
if (!object.IsSet() || (object->Type() & USB_OBJECT_ISO_PIPE) == 0)
return B_DEV_INVALID_PIPE;
return ((IsochronousPipe *)object)->SetPipePolicy(maxQueuedPackets,
return ((IsochronousPipe *)object.IsSet())->SetPipePolicy(maxQueuedPackets,
maxBufferDurationMS, sampleSize);
}
@@ -492,11 +519,11 @@ status_t
cancel_queued_transfers(usb_pipe pipe)
{
TRACE_MODULE("cancel_queued_transfers(%" B_PRId32 ")\n", pipe);
Object *object = gUSBStack->GetObject(pipe);
if (!object || (object->Type() & USB_OBJECT_PIPE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(pipe));
if (!object.IsSet() || (object->Type() & USB_OBJECT_PIPE) == 0)
return B_DEV_INVALID_PIPE;
return ((Pipe *)object)->CancelQueuedTransfers(false);
return ((Pipe *)object.Get())->CancelQueuedTransfers(false);
}
@@ -508,12 +535,10 @@ usb_ioctl(uint32 opcode, void *buffer, size_t bufferSize)
switch (opcode) {
case 'DNAM': {
Object *object = gUSBStack->GetObject(*(usb_id *)buffer);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(*(usb_id *)buffer));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return B_BAD_VALUE;
Device *device = (Device *)object;
if (device->InitCheck() != B_OK)
return B_NO_INIT;
Device *device = (Device *)object.Get();
uint32 index = 0;
return device->BuildDeviceName((char *)buffer, &index,
@@ -550,11 +575,11 @@ get_nth_child(usb_device _hub, uint8 index, usb_device *childDevice)
if (!childDevice)
return B_BAD_VALUE;
Object *object = gUSBStack->GetObject(_hub);
if (!object || (object->Type() & USB_OBJECT_HUB) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(_hub));
if (!object.IsSet() || (object->Type() & USB_OBJECT_HUB) == 0)
return B_DEV_INVALID_PIPE;
Hub *hub = (Hub *)object;
Hub *hub = (Hub *)object.Get();
for (uint8 i = 0; i < 8; i++) {
if (hub->ChildAt(i) == NULL)
continue;
@@ -576,8 +601,8 @@ get_device_parent(usb_device _device, usb_device *parentHub, uint8 *portIndex)
if (!parentHub || !portIndex)
return B_BAD_VALUE;
Object *object = gUSBStack->GetObject(_device);
if (!object || (object->Type() & USB_OBJECT_DEVICE) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(_device));
if (!object.IsSet() || (object->Type() & USB_OBJECT_DEVICE) == 0)
return B_DEV_INVALID_PIPE;
Object *parent = object->Parent();
@@ -586,7 +611,7 @@ get_device_parent(usb_device _device, usb_device *parentHub, uint8 *portIndex)
Hub *hub = (Hub *)parent;
for (uint8 i = 0; i < 8; i++) {
if (hub->ChildAt(i) == object) {
if (hub->ChildAt(i) == object.Get()) {
*portIndex = i;
*parentHub = hub->USBID();
return B_OK;
@@ -600,11 +625,11 @@ get_device_parent(usb_device _device, usb_device *parentHub, uint8 *portIndex)
status_t
reset_port(usb_device _hub, uint8 portIndex)
{
Object *object = gUSBStack->GetObject(_hub);
if (!object || (object->Type() & USB_OBJECT_HUB) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(_hub));
if (!object.IsSet() || (object->Type() & USB_OBJECT_HUB) == 0)
return B_DEV_INVALID_PIPE;
Hub *hub = (Hub *)object;
Hub *hub = (Hub *)object.Get();
return hub->ResetPort(portIndex);
}
@@ -612,11 +637,11 @@ reset_port(usb_device _hub, uint8 portIndex)
status_t
disable_port(usb_device _hub, uint8 portIndex)
{
Object *object = gUSBStack->GetObject(_hub);
if (!object || (object->Type() & USB_OBJECT_HUB) == 0)
ObjectBusyReleaser object(gUSBStack->GetObject(_hub));
if (!object.IsSet() || (object->Type() & USB_OBJECT_HUB) == 0)
return B_DEV_INVALID_PIPE;
Hub *hub = (Hub *)object;
Hub *hub = (Hub *)object.Get();
return hub->DisablePort(portIndex);
}
@@ -124,6 +124,8 @@ public:
usb_id GetUSBID(Object *object);
void PutUSBID(Object *object);
// This sets the object as busy; the caller must set it un-busy.
Object * GetObject(usb_id id);
// only for the kernel debugger
@@ -264,6 +266,9 @@ virtual ~Object();
Stack * GetStack() const { return fStack; }
usb_id USBID() const { return fUSBID; }
void SetBusy(bool busy)
{ atomic_add(&fBusy, busy ? 1 : -1); }
virtual uint32 Type() const { return USB_OBJECT_NONE; }
virtual const char * TypeName() const { return "object"; }
@@ -280,6 +285,7 @@ private:
BusManager * fBusManager;
Stack * fStack;
usb_id fUSBID;
int32 fBusy;
};