USB host MSC implementation at PoC stage according to SoW Block 4 (REQ-VND-USB-006) - #4
Conversation
Signed-off-by: Mattia Maldini <mattia.m@embedd.it> Signed-off-by: Valentyn Gololobov <val@embedd.it>
according to SoW Block 4 (REQ-VND-USB-006) - Added test cases for the USB MSC driver targetting ek_ra8m1, ek_ra8m2 and native_sim. - Added configuration options to exclude the synchronization command, seeing as many devices don't support it. - Included checks to verify whether the medium is actually present (e.g. for USB SD card readers), returning proper error codes if not. - Added Mode Sense (6) fallback - Complete SCSI BOT implementation - Support for multiple logical units - The Host MSC driver keeps track of each LUN advertised by the device and creates a corresponding disk name. - Added checks for write protected drives, BOT error recovery and the sync command. Signed-off-by: Mattia Maldini <mattia.m@embedd.it> Signed-off-by: Valentyn Gololobov <val@embedd.it>
thenguyenyf
left a comment
thenguyenyf
left a comment
There was a problem hiding this comment.
I left some comments after a quick scan. I still need further investigation and inspection of the MSC specs and source code for the second round of reviews.
| if(CONFIG_USBD_VIDEO_CLASS OR CONFIG_USBH_VIDEO_CLASS OR CONFIG_USBD_MSC_CLASS OR CONFIG_USBH_MSC_CLASS) | ||
| zephyr_include_directories(.) | ||
| zephyr_sources(uvc.c) | ||
| endif() |
There was a problem hiding this comment.
Could you please explain why the MSC class build includes uvc.c?
There was a problem hiding this comment.
The MSC drivers doesn't strictly need uvc.c, just the directory inclusion to access subsus/usb/common and include msc.h. I'll update the file reflect this.
| /** | ||
| * @brief Synchronization callback to wait for completion of asynchronous transfers | ||
| * | ||
| * Should be passed to `usbh_xfer_alloc_with_buf` or `usbh_xfer_alloc` before queuing the | ||
| * transfer, to then block on `driver_data->sync` in order to wait for completion. This | ||
| * function only gives way to the semaphore; it doesn't analyze or deallocate anything. | ||
| * | ||
| * @param udev Pointer to the connected USB device structure | ||
| * @param xfer Pointer to completed transfer structure | ||
| * | ||
| * @return 0 | ||
| */ |
There was a problem hiding this comment.
You should use simple C comment block instead of a Doxygen comment for an internal helper function.
| static int scsi_transfer_data(struct driver_data *driver_data, size_t data_length, | ||
| uint8_t data[data_length], enum scsi_direction direction) |
There was a problem hiding this comment.
This function violate https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_18_08.c
Fyi, Zephyr follows a subset of MISRA-C https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#main-rules
| result = scsi_transaction(driver_data, | ||
| (struct scsi_cbw){ | ||
| .lun = lun_index, | ||
| .direction = SCSI_DIRECTION_DATA_IN, | ||
| .command_block_length = sizeof(command_block), | ||
| .command_block = command_block, | ||
| .data_transfer_length = sizeof(buffer), | ||
| }, | ||
| buffer, true); |
There was a problem hiding this comment.
This piece of code violate https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_09_05.c
| return 0; | ||
| } | ||
|
|
||
| #if !CONFIG_USBH_MSC_IGNORE_SYNC |
There was a problem hiding this comment.
#ifndef CONFIG_USBH_MSC_IGNORE_SYNC
| { | ||
| (void)disk; | ||
| (void)start_sector; | ||
| (void)num_sector; |
There was a problem hiding this comment.
ARG_UNUSED(disk);
ARG_UNUSED(start_sector);
ARG_UNUSED(num_sector);
| * | ||
| * @return 0 on success, negative errno value on failure. | ||
| */ | ||
| static int usbh_class_probe(struct usbh_class_data *const c_data, struct usb_device *const udev, |
There was a problem hiding this comment.
You should follow the naming space
static int usbh_msc_probe(struct usbh_class_data *const c_data, struct usb_device *const udev,
There was a problem hiding this comment.
Same for other usbh_class_api ops
| /** | ||
| * @brief USB Host class filters | ||
| */ | ||
| static struct usbh_class_filter usbh_uvc_filters[] = { |
There was a problem hiding this comment.
Wrong copy?
static struct usbh_class_filter usbh_msc_filters[] = {
There was a problem hiding this comment.
Please check the other incorrect copies of the UVC elsewhere in this PR.
| .flags = USBH_CLASS_MATCH_CODE_TRIPLE, | ||
| .class = USB_BCC_MASS_STORAGE, | ||
| .sub = SCSI_TRANSPARENT_COMMAND_SET, | ||
| .proto = BULK_ONLY_TRANSPORT, |
There was a problem hiding this comment.
It's fine to me to have only BULK_ONLY_TRANSPORT for initial support. But have you taken the time to check out other protocols, such as CBI? Could the current implementation easily be extended to support other MSC protocols?
There was a problem hiding this comment.
Yes, support for other protocols like CBI could be added in a relatively short time. Did you have specific devices in mind?
| config USBH_MSC_CLASS | ||
| bool "USB host Mass Storage Class support [EXPERIMENTAL]" | ||
| select EXPERIMENTAL |
There was a problem hiding this comment.
Missing select DISK_ACCESS
- Added note about limitations - Fixed coding guidelines and MISRA rule compliance Signed-off-by: Mattia Maldini <mattia.m@embedd.it> Signed-off-by: Valentyn Gololobov <val@embedd.it>
- Small extension to MSC test case - Fixed error paths and code style in driver - Fixed a deallocation bug, tested with MAX3421E - If a transfer is cancelled it shouldn't be deallocated by the upper driver, the host driver may still need to use it. It must be freed by the callback. - Other host drivers use `-EPIPE` to signal a stall, aligned Renesas driver. - Verified the MSC test with the MAX3421E. Signed-off-by: Mattia Maldini <mattia.m@embedd.it> Signed-off-by: Valentyn Gololobov <val@embedd.it>
Signed-off-by: Mattia Maldini <mattia.m@embedd.it>
6 participants
USB host Mass Storage Class (MSC) implementation at PoC stage
according to SoW Block 4 (REQ-VND-USB-006)
native_sim.
seeing as many devices don't support it.
for USB SD card readers), returning proper error codes if not.
and creates a corresponding disk name.
and the sync command.