Kernel: Add a basic USB Attached SCSI (UAS) driver #25067

Hendiadyoin1 · 2024-10-02T18:50:37Z

Kernel/USBMS: Move the BOT/BBB driver to its own directory

Kernel/USB: Save all USB descriptors and allow iterating over them

This makes it easier to access class specific descriptors, which
are sometimes dependent on the device configuration immediately before
them.

Kernel/USBMS: Add a basic UAS driver

For now we only support USB <3.0 devices, as we don't lissten for
ERDY transaction packets.
We also don't leverage the benefits of UAS, as we pretend to have a
queue depth of 1, ie are single threaded.

To test this driver, you can use the following command:

SERENITY_BOOT_DRIVE=usb-uas Meta/serenity.sh run x86_64 Clang

SideNote: This can't legally fly drones

CC: @spholz

kleinesfilmroellchen

last commit idk, rest is fine

spholz

Wow, that's a lot of FIXMEs

Kernel/Devices/Storage/USB/BOT/BulkSCSIInterface.h

Kernel/Bus/USB/USBConfiguration.h

Kernel/Bus/USB/Drivers/MassStorage/MassStorageDriver.cpp

Kernel/Bus/USB/Drivers/MassStorage/MassStorageDriver.h

Kernel/Devices/Storage/USB/UAS/UASInterface.cpp

spholz · 2024-10-04T11:50:20Z

Kernel/Devices/Storage/USB/UAS/UASInterface.h

+ // Note: The maximum packet size for the endpoints is 512 bytes on USB2 and 1024 bytes on USB3
+ // As the SenseIU is of flexible size we need to forcefully allocate some data here
+ // The actual max size of the sense data is controlled by the MAXIMUM SENSE DATA LENGTH field in the
+ // Control extension mode page of the device
+ // The maximum size of the sense data is 252 bytes
+ // Meaning the maximum size of the IU is 252+16=268 bytes
+ // Just to be safe we allocate 512 bytes here


Nit: This should probably be formatted into a normal sentence, like what we usually do for comments.
Same for all other comments in this commit?

Is the new text better?

Yeah, I guess it's ok.

Kernel/Devices/Storage/USB/UAS/UASInterface.h

Kernel/Devices/Storage/USB/UAS/Structures.h

spholz · 2024-10-07T13:11:27Z

Kernel/Bus/USB/USBDevice.cpp

@@ -97,4 +97,37 @@ ErrorOr<size_t> Device::control_transfer(u8 request_type, u8 request, u16 value,
 return TRY(m_default_pipe->submit_control_transfer(request_type, request, value, index, length, data));
 }

+ErrorOr<void> Device::activate_configuration(USBConfiguration const& configuration)
+{
+ if (m_was_configured.was_set() && m_current_configuration != configuration.configuration_id())


I still don't think this check is necessary. Only one driver can be attached to a USB device. So unless a driver attempts to change configurations more than once, this will never happen.
And if we think so such a check is necessary, shouldn't we also do a similar check for alternate settings?

Tracking interface usage is a bit harder to track with minimal invasiveness, but might be missing something
I can remove the check but prefer to be better safe now then forgetting it later

Sure, let's keep that check for now then. We can maybe remove it once we have a better way of probing USB drivers instead of trying all drivers one by one. That theoretical new abstraction would probably have to keep track of configurations itself.

spholz · 2024-10-07T13:14:00Z

Kernel/Bus/USB/USBDevice.cpp

@@ -97,4 +97,37 @@ ErrorOr<size_t> Device::control_transfer(u8 request_type, u8 request, u16 value,
 return TRY(m_default_pipe->submit_control_transfer(request_type, request, value, index, length, data));
 }

+ErrorOr<void> Device::activate_configuration(USBConfiguration const& configuration)


I would personally just call this function like the actual USB request it runs, set_configuration.
And activate_interface can probably be called something like set_configuration_and_interface so it's clearer what it actually does.

spholz · 2024-10-07T13:14:17Z

Kernel/Devices/Storage/USB/UAS/UASInterface.h

+ // Note: The maximum packet size for the endpoints is 512 bytes on USB2 and 1024 bytes on USB3
+ // As the SenseIU is of flexible size we need to forcefully allocate some data here
+ // The actual max size of the sense data is controlled by the MAXIMUM SENSE DATA LENGTH field in the
+ // Control extension mode page of the device
+ // The maximum size of the sense data is 252 bytes
+ // Meaning the maximum size of the IU is 252+16=268 bytes
+ // Just to be safe we allocate 512 bytes here


Yeah, I guess it's ok.

spholz

Oops, I didn't mean to finish my review (Adding a response to a comment appears to submit reviews?).

spholz · 2024-10-07T13:22:39Z

Kernel/Bus/USB/USBDevice.cpp

+ // FIXME: When we use the default alternate_setting of interface/the current alternate setting, we don't need to SET_INTERFACE it
+ // but that gets a bit difficult to track
+ TRY(control_transfer(USB_REQUEST_TRANSFER_DIRECTION_HOST_TO_DEVICE | USB_REQUEST_TYPE_STANDARD | USB_REQUEST_RECIPIENT_INTERFACE, USB_REQUEST_SET_INTERFACE,
+ interface.descriptor().interface_id, interface.descriptor().alternate_setting, 0, nullptr));


This should be interface.descriptor().alternate_setting, interface.descriptor().interface_id, 0, nullptr));

spholz · 2024-10-07T13:22:48Z

Kernel/Bus/USB/USBRequest.h

@@ -41,6 +41,7 @@ static constexpr u8 USB_REQUEST_GET_DESCRIPTOR = 0x06;
 static constexpr u8 USB_REQUEST_SET_DESCRIPTOR = 0x07;
 static constexpr u8 USB_REQUEST_GET_CONFIGURATION = 0x08;
 static constexpr u8 USB_REQUEST_SET_CONFIGURATION = 0x09;
+static constexpr u8 USB_REQUEST_SET_INTERFACE = 0x11;


spholz · 2024-10-13T17:07:02Z

Kernel/Bus/USB/Drivers/MassStorage/MassStorageDriver.cpp

+
+ if (uas_interface.has_value() && device.speed() != USB::Device::DeviceSpeed::SuperSpeed) {
+ // FIXME: We only support UAS on version < 3.0 devices
+ // as we don't support waiting ERDY transactions yet


These references to ERDY should probably replaced with streams (also in the commit message).

This makes it easier to access class specific descriptors, which are sometimes dependent on the descriptors immediately before them.

For now we only support USB <3.0 devices, as we don't support streams. We also don't leverage the benefits of UAS, as we pretend to have a queue depth of 1, ie are single threaded. To test this driver, you can use the following command: ``` SERENITY_BOOT_DRIVE=usb-uas Meta/serenity.sh run x86_64 Clang ```

kiruthikpurpose

Great work!

kiruthikpurpose

I like the way you added the offset to the function parameter. Make sure it has no leaks thoroughly.

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 2, 2024

Hendiadyoin1 force-pushed the UAS branch 4 times, most recently from 45b6666 to d6236f5 Compare October 3, 2024 17:49

kleinesfilmroellchen reviewed Oct 3, 2024

View reviewed changes

Hendiadyoin1 force-pushed the UAS branch from d6236f5 to 1c466ec Compare October 3, 2024 22:05

spholz reviewed Oct 4, 2024

View reviewed changes

Hendiadyoin1 force-pushed the UAS branch from 1c466ec to 0cb3b2f Compare October 4, 2024 21:34

Hendiadyoin1 requested a review from spholz October 4, 2024 21:37

Hendiadyoin1 force-pushed the UAS branch 2 times, most recently from 3e94004 to e1d0d3d Compare October 6, 2024 14:36

spholz reviewed Oct 7, 2024

View reviewed changes

timschumi self-requested a review October 7, 2024 14:34

spholz reviewed Oct 13, 2024

View reviewed changes

Hendiadyoin1 added 4 commits October 14, 2024 23:59

Kernel/USBMS: Move the BOT/BBB driver to its own directory

d6f1ae1

Kernel/USB: Save all USB descriptors and allow iterating over them

2700e5a

This makes it easier to access class specific descriptors, which are sometimes dependent on the descriptors immediately before them.

Kernel/USB: Add and use a shared function to set a device's interface

af8975b

Hendiadyoin1 force-pushed the UAS branch from e1d0d3d to af8975b Compare October 14, 2024 22:00

spholz approved these changes Oct 16, 2024

View reviewed changes

kiruthikpurpose reviewed Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel: Add a basic USB Attached SCSI (UAS) driver #25067

Kernel: Add a basic USB Attached SCSI (UAS) driver #25067

Hendiadyoin1 commented Oct 2, 2024

kleinesfilmroellchen left a comment

spholz left a comment

spholz Oct 4, 2024

Hendiadyoin1 Oct 4, 2024

spholz Oct 7, 2024

spholz Oct 7, 2024

Hendiadyoin1 Oct 14, 2024

spholz Oct 16, 2024

spholz Oct 7, 2024

Hendiadyoin1 Oct 14, 2024

spholz Oct 7, 2024

spholz left a comment

spholz Oct 7, 2024

spholz Oct 7, 2024

spholz Oct 13, 2024

kiruthikpurpose left a comment

kiruthikpurpose left a comment

Kernel: Add a basic USB Attached SCSI (UAS) driver #25067

Are you sure you want to change the base?

Kernel: Add a basic USB Attached SCSI (UAS) driver #25067

Conversation

Hendiadyoin1 commented Oct 2, 2024

Kernel/USBMS: Move the BOT/BBB driver to its own directory

Kernel/USB: Save all USB descriptors and allow iterating over them

Kernel/USBMS: Add a basic UAS driver

kleinesfilmroellchen left a comment

Choose a reason for hiding this comment

spholz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spholz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiruthikpurpose left a comment

Choose a reason for hiding this comment

kiruthikpurpose left a comment

Choose a reason for hiding this comment