Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nsyshid: Fix SetProtocol and SetReport for Libusb Backend #1027

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 64 additions & 26 deletions src/Cafe/OS/libs/nsyshid/BackendLibusb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -694,22 +694,58 @@ namespace nsyshid::backend::libusb
return false;
}

// ToDo: implement this
#if 0
// is this correct? Discarding "ifIndex" seems like a bad idea
int ret = libusb_set_configuration(handleLock->getHandle(), protocol);
if (ret == 0) {
cemuLog_logDebug(LogType::Force,
"nsyshid::DeviceLibusb::setProtocol(): success");
return true;
struct libusb_config_descriptor* conf = nullptr;
libusb_device* dev = libusb_get_device(handleLock->GetHandle());
int getConfig = libusb_get_active_config_descriptor(dev, &conf);
if (getConfig == LIBUSB_SUCCESS)
{
for (uint8 i = 0; i < conf->bNumInterfaces; ++i)
{
int releaseSuccess = libusb_release_interface(handleLock->GetHandle(), i);
if (releaseSuccess < LIBUSB_SUCCESS)
{
cemuLog_logDebug(LogType::Force, "nsyshid::DeviceLibusb::SetProtocol(): Failed to release interface %i", i);
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning here is problematic: We still need to tell libusb to free the struct that conf points to!
Since you are only using conf->bNumInterfaces from that struct, how about calling libusb_get_active_config_descriptor(), making a copy of conf->bNumInterfaces and then immediately calling libusb_free_config_descriptor()?

}
}
}
else
{
cemuLog_logDebug(LogType::Force, "nsyshid::DeviceLibusb::SetProtocol(): Failed to Get Active Config");
}

const int setConfig = libusb_set_configuration(handleLock->GetHandle(), protocol);
if (setConfig == LIBUSB_SUCCESS)
{
getConfig = libusb_get_active_config_descriptor(dev, &conf);
Copy link
Contributor

@ssievert42 ssievert42 Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (in its current form) causes us to lose all references to the struct that conf previously pointed to.
Just made a small test program, to confirm that I am not talking utter nonsense, and valgrind agrees with me that the following snippet leaks:

struct libusb_config_descriptor *conf = nullptr;
libusb_get_active_config_descriptor(dev, &conf);
libusb_get_active_config_descriptor(dev, &conf);
libusb_free_config_descriptor(conf);

if (getConfig == LIBUSB_SUCCESS)
{
for (uint8 i = 0; i < conf->bNumInterfaces; ++i)
{
int detachSuccess = libusb_detach_kernel_driver(handleLock->GetHandle(), i);
if (detachSuccess < LIBUSB_SUCCESS && detachSuccess != LIBUSB_ERROR_NOT_FOUND &&
detachSuccess != LIBUSB_ERROR_NOT_SUPPORTED)
{
cemuLog_logDebug(LogType::Force, "nsyshid::DeviceLibusb::SetProtocol(): failed to detach kernel driver");
return false;
}
int claimInterface = libusb_claim_interface(handleLock->GetHandle(), i);
if (claimInterface < LIBUSB_SUCCESS)
{
cemuLog_logDebug(LogType::Force, "nsyshid::DeviceLibusb::SetProtocol(): failed to claim interface");
return false;
}
}
}

libusb_free_config_descriptor(conf);
}
else
{
cemuLog_logDebug(LogType::Force, "nsyshid::DeviceLibusb::SetProtocol(): Failed to set config %i", protocol);
return false;
}
cemuLog_logDebug(LogType::Force,
"nsyshid::DeviceLibusb::setProtocol(): failed with error code: {}",
ret);
return false;
#endif

// pretend that everything is fine
return true;
}

Expand All @@ -723,18 +759,20 @@ namespace nsyshid::backend::libusb
return false;
}

// ToDo: implement this
#if 0
// not sure if libusb_control_transfer() is the right candidate for this
int ret = libusb_control_transfer(handleLock->getHandle(),
bmRequestType,
bRequest,
wValue,
wIndex,
reportData,
length,
timeout);
#endif
int ret = libusb_control_transfer(handleLock->GetHandle(),
LIBUSB_REQUEST_TYPE_CLASS | LIBUSB_RECIPIENT_INTERFACE | LIBUSB_ENDPOINT_OUT,
LIBUSB_REQUEST_SET_CONFIGURATION,
512 /* This may be skylander specific, but other games don't use this method anyway */,
0,
originalData,
originalLength,
0);

if (ret != originalLength)
{
cemuLog_log(LogType::Force, "nsyshid::DeviceLibusb::SetReport(): Control Transfer Failed: {}", libusb_error_name(ret));
return false;
}

// pretend that everything is fine
return true;
Expand Down
1 change: 0 additions & 1 deletion src/Cafe/OS/libs/nsyshid/nsyshid.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,6 @@ namespace nsyshid
{
sprintf(debugOutput + i * 3, "%02x ", data[i]);
}
fmt::print("{} Data: {}\n", prefix, debugOutput);
cemuLog_logDebug(LogType::Force, "[{}] Data: {}", prefix, debugOutput);
}

Expand Down