[dm][ufs] support Universal Flash Storage (UFS)#11302
[dm][ufs] support Universal Flash Storage (UFS)#11302GuEe-GUI wants to merge 1 commit intoRT-Thread:masterfrom
Conversation
Support UFS over PCI, too. Signed-off-by: GuEe-GUI <2991707448@qq.com>
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-03-29 22:20 CST)
📝 Review Instructions
|
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new RT-Thread DM storage driver stack for Universal Flash Storage (UFS), including core UTP/SCSI transport, basic power-management/link helpers, and an optional PCI bus glue driver.
Changes:
- Add UFS host controller driver (
ufs.c) implementing UTP transfer for a subset of SCSI commands. - Add UFS PM/link helper utilities (
ufs_pm.c) for DME/PA power mode, Hibern8, AHIT, and interrupt aggregation. - Add optional PCI glue (
ufs-pci.c) plus build/Kconfig wiring and public header exposure.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| components/drivers/ufs/ufs.c | Core UFS host registration and UTP-based SCSI transfer path |
| components/drivers/ufs/ufs_pm.c | UIC/DME helpers and post-link power/interrupt aggregation setup |
| components/drivers/ufs/ufs-pci.c | PCI probe/remove glue for UFS host controller |
| components/drivers/ufs/SConscript | Build integration for the new UFS sources |
| components/drivers/ufs/Kconfig | Kconfig options for enabling UFS and UFS-over-PCI |
| components/drivers/include/drivers/ufs.h | Public UFS register/UPIU/UTP definitions and APIs |
| components/drivers/include/rtdevice.h | Expose drivers/ufs.h under RT_USING_UFS |
| components/drivers/Kconfig | Hook UFS Kconfig into the drivers menu |
| err = -RT_ERROR; | ||
| goto _end; | ||
| } | ||
|
|
||
| /* Sense data: copy to cmd when check condition (BSP may use cmd->data.ptr for sense) */ | ||
| if (err != RT_EOK && rsp->sr.sense_data_len != 0 && cmd->data.ptr && cmd->data.size >= RT_UFS_SENSE_SIZE) | ||
| { | ||
| rt_uint16_t sense_len = rt_be16_to_cpu(rsp->sr.sense_data_len); | ||
|
|
||
| sense_len = rt_min_t(rt_uint16_t, sense_len, RT_UFS_SENSE_SIZE); | ||
| rt_memcpy(cmd->data.ptr, rsp->sr.sense_data, sense_len); | ||
| } | ||
|
|
There was a problem hiding this comment.
[bug/缺陷]: Sense-data copy block is effectively unreachable.
English: The code returns immediately when rsp->header.status != 0 (sets err and goto _end), so the later if (err != RT_EOK && rsp->sr.sense_data_len != 0 ...) never runs (err is RT_EOK on the success path). If you intend to copy sense data on CHECK CONDITION, set err before the copy and perform the copy before returning, or key the copy off rsp->header.status.
中文：当 rsp->header.status != 0 时会立刻设置 err 并跳转返回，因此后面的 if (err != RT_EOK && rsp->sr.sense_data_len != 0 ...) 实际不会被执行（成功路径下 err 仍为 RT_EOK）。若希望在 CHECK CONDITION 时拷贝 sense 数据，应在返回前完成拷贝，并以 rsp->header.status / sense_data_len 作为条件。
| err = -RT_ERROR; | |
| goto _end; | |
| } | |
| /* Sense data: copy to cmd when check condition (BSP may use cmd->data.ptr for sense) */ | |
| if (err != RT_EOK && rsp->sr.sense_data_len != 0 && cmd->data.ptr && cmd->data.size >= RT_UFS_SENSE_SIZE) | |
| { | |
| rt_uint16_t sense_len = rt_be16_to_cpu(rsp->sr.sense_data_len); | |
| sense_len = rt_min_t(rt_uint16_t, sense_len, RT_UFS_SENSE_SIZE); | |
| rt_memcpy(cmd->data.ptr, rsp->sr.sense_data, sense_len); | |
| } | |
| /* Sense data: copy to cmd when check condition (BSP may use cmd->data.ptr for sense) */ | |
| if (rsp->sr.sense_data_len != 0 && cmd->data.ptr && cmd->data.size >= RT_UFS_SENSE_SIZE) | |
| { | |
| rt_uint16_t sense_len = rt_be16_to_cpu(rsp->sr.sense_data_len); | |
| sense_len = rt_min_t(rt_uint16_t, sense_len, RT_UFS_SENSE_SIZE); | |
| rt_memcpy(cmd->data.ptr, rsp->sr.sense_data, sense_len); | |
| } | |
| err = -RT_ERROR; | |
| goto _end; | |
| } |
| LOG_E("%s: UTP PRDT[0]: addr=%#llx size(le32)=%#08x", | ||
| rt_dm_dev_get_name(ufs->parent.dev), | ||
| (rt_uint64_t)(&((struct rt_utp_transfer_cmd_desc *)ufs->ucd_base)->prd_table[0])->addr, | ||
| (&((struct rt_utp_transfer_cmd_desc *)ufs->ucd_base)->prd_table[0])->size); |
There was a problem hiding this comment.
[maintainability/可维护性]: Timeout PRDT debug log prints the address of the addr field (not the PRDT DMA address), and prints LE fields without converting.
English: &prd_table[0]->addr logs the pointer-to-field, which is misleading for post-mortem debugging. Consider logging rt_le64_to_cpu(prd_table[0].addr) and rt_le32_to_cpu(prd_table[0].size) (and avoid taking the address-of).
中文：超时场景下的 PRDT 日志打印的是 addr 字段自身的地址（指针），不是 PRDT 里保存的 DMA 地址；同时 LE 字段未做 endian 转换，容易误导定位问题。建议打印 rt_le64_to_cpu(prd_table[0].addr) / rt_le32_to_cpu(prd_table[0].size)，不要对字段取地址。
| LOG_E("%s: UTP PRDT[0]: addr=%#llx size(le32)=%#08x", | |
| rt_dm_dev_get_name(ufs->parent.dev), | |
| (rt_uint64_t)(&((struct rt_utp_transfer_cmd_desc *)ufs->ucd_base)->prd_table[0])->addr, | |
| (&((struct rt_utp_transfer_cmd_desc *)ufs->ucd_base)->prd_table[0])->size); | |
| LOG_E("%s: UTP PRDT[0]: addr=%#llx size=%#08x", | |
| rt_dm_dev_get_name(ufs->parent.dev), | |
| (rt_uint64_t)rt_le64_to_cpu(((struct rt_utp_transfer_cmd_desc *)ufs->ucd_base)->prd_table[0].addr), | |
| (rt_uint32_t)rt_le32_to_cpu(((struct rt_utp_transfer_cmd_desc *)ufs->ucd_base)->prd_table[0].size)); |
|
|
||
| return RT_EOK; | ||
|
|
||
| _fail: |
There was a problem hiding this comment.
[bug/缺陷]: pci_ufs_probe() failure path leaks resources.
English: If rt_ufs_host_register() fails after rt_pci_iomap()/rt_pci_set_master()/rt_pci_irq_unmask(), the _fail label only frees pci_ufs and does not: mask IRQ, clear bus master, or iounmap regs. Please add cleanup similar to components/drivers/ata/ahci-pci.c (disable/mask IRQ, rt_pci_clear_master, rt_iounmap).
中文：pci_ufs_probe() 在 rt_pci_iomap()/rt_pci_set_master()/rt_pci_irq_unmask() 之后若 rt_ufs_host_register() 失败，_fail 仅 rt_free(pci_ufs)，会泄露/遗留资源：未 mask IRQ、未 clear master、未 iounmap regs。建议参考 components/drivers/ata/ahci-pci.c 的清理流程（mask/disable IRQ、rt_pci_clear_master、rt_iounmap）。
| _fail: | |
| _fail: | |
| if (ufs->regs) | |
| { | |
| rt_pci_irq_mask(pdev); | |
| rt_pci_clear_master(pdev); | |
| rt_iounmap(ufs->regs); | |
| } |
| rt_err_t rt_ufs_dme_get(struct rt_ufs_host *ufs, rt_uint32_t attr_sel, rt_uint32_t *value) | ||
| { | ||
| rt_uint32_t arg2 = 0; | ||
| rt_err_t err; | ||
|
|
||
| if (!value) | ||
| { | ||
| return -RT_EINVAL; | ||
| } | ||
|
|
||
| if ((err = rt_ufs_uic_cmd_send(ufs, RT_UFS_CMDOP_DME_GET, attr_sel, &arg2, 0))) | ||
| { | ||
| return err; | ||
| } | ||
|
|
||
| if ((arg2 & RT_UFS_CMDRES_MASK) != RT_UFS_CMDRES_SUCCESS) | ||
| { | ||
| return -RT_ERROR; | ||
| } | ||
|
|
||
| *value = HWREG32(ufs->regs + RT_UFS_REG_UCMDARG3); | ||
|
|
There was a problem hiding this comment.
[bug/缺陷]: rt_ufs_dme_get() may dereference a NULL ufs / ufs->regs when reading UCMDARG3.
English: Even though rt_ufs_uic_cmd_send() checks regs, this function reads HWREG32(ufs->regs + ...) unconditionally after the call, which will crash if ufs or regs is NULL. Please add the same validation as other helpers (ufs && ufs->regs) before issuing the UIC command.
中文：rt_ufs_dme_get() 在读取 UCMDARG3 时可能解引用 NULL 的 ufs / ufs->regs。虽然 rt_ufs_uic_cmd_send() 会检查 regs，但本函数在调用后无条件执行 HWREG32(ufs->regs + ...)，若 ufs/regs 为空会崩溃。建议在发送 UIC 命令前增加与其他辅助函数一致的校验（ufs && ufs->regs）。
|
|
||
| rt_scsi_host_unregister(&ufs->parent); | ||
|
|
||
| if (ufs->ops->exit && (err = ufs->ops->exit(ufs))) | ||
| { | ||
| LOG_W("%s: UFS ops->exit failed: %s", rt_dm_dev_get_name(ufs->parent.dev), rt_strerror(err)); | ||
| } | ||
|
|
There was a problem hiding this comment.
[concurrency/并发]: rt_ufs_host_unregister() frees DMA buffers but does not mask/detach the IRQ (nor disable controller interrupts), which can lead to ISR use-after-free during hot-unplug/remove.
English: Please add NULL checks, stop/disable interrupts (e.g., clear IE, mask + detach IRQ like rt_ahci_host_unregister does), then free DMA buffers. Also consider checking/propagating the return value of rt_scsi_host_unregister().
中文：rt_ufs_host_unregister() 在释放 DMA 缓冲区前没有屏蔽/解绑中断（也未关闭控制器中断），在设备移除/热插拔时可能导致 ISR 访问已释放内存（UAF）。建议增加空指针检查，先关闭/屏蔽中断（例如清 IE，并像 rt_ahci_host_unregister 那样 mask + detach IRQ），再释放 DMA 缓冲区；同时建议检查/返回 rt_scsi_host_unregister() 的返回值。
| rt_scsi_host_unregister(&ufs->parent); | |
| if (ufs->ops->exit && (err = ufs->ops->exit(ufs))) | |
| { | |
| LOG_W("%s: UFS ops->exit failed: %s", rt_dm_dev_get_name(ufs->parent.dev), rt_strerror(err)); | |
| } | |
| rt_err_t ret = RT_EOK; | |
| if (ufs == RT_NULL) | |
| { | |
| return -RT_EINVAL; | |
| } | |
| /* First, stop controller and interrupts via ops->exit (if available). */ | |
| if (ufs->ops && ufs->ops->exit) | |
| { | |
| ret = ufs->ops->exit(ufs); | |
| if (ret != RT_EOK) | |
| { | |
| LOG_W("%s: UFS ops->exit failed: %s", | |
| rt_dm_dev_get_name(ufs->parent.dev), rt_strerror(ret)); | |
| err = ret; | |
| } | |
| } | |
| /* Unregister SCSI host and propagate error if it fails. */ | |
| ret = rt_scsi_host_unregister(&ufs->parent); | |
| if (ret != RT_EOK) | |
| { | |
| LOG_W("%s: rt_scsi_host_unregister failed: %s", | |
| rt_dm_dev_get_name(ufs->parent.dev), rt_strerror(ret)); | |
| if (err == RT_EOK) | |
| { | |
| err = ret; | |
| } | |
| } |
| goto _fail; | ||
| } | ||
|
|
||
| ufs->ops = quirk ? quirk->ops : &pci_ufs_std_ops; |
There was a problem hiding this comment.
[bug/缺陷]: pci_ufs_probe() chooses ufs->ops as quirk ? quirk->ops : &pci_ufs_std_ops, which can set ops to NULL when quirk exists but quirk->ops is not provided.
English: Please match the pattern used by AHCI/NVMe PCI drivers: ufs->ops = (quirk && quirk->ops) ? quirk->ops : &pci_ufs_std_ops; to avoid rt_ufs_host_register() failing/crashing.
中文：pci_ufs_probe() 使用 quirk ? quirk->ops : &pci_ufs_std_ops 选择 ops，当 quirk 存在但 quirk->ops 未设置时会把 ops 置为 NULL，导致后续注册失败/潜在崩溃。建议参考 AHCI/NVMe PCI 驱动的写法：(quirk && quirk->ops) ? quirk->ops : &pci_ufs_std_ops。
| ufs->ops = quirk ? quirk->ops : &pci_ufs_std_ops; | |
| ufs->ops = (quirk && quirk->ops) ? quirk->ops : &pci_ufs_std_ops; |
| { | ||
| rt_err_t err; | ||
| rt_uint32_t arg2 = ufs_uic_arg_attr_type(RT_UFS_DME_ATTR_SET_NOR); | ||
|
|
There was a problem hiding this comment.
[bug/缺陷]: rt_ufs_dme_set() does not validate ufs/ufs->regs before calling rt_ufs_uic_cmd_send().
English: If a caller passes a NULL ufs (or an uninitialized host), this will dereference ufs inside rt_ufs_uic_cmd_send() and crash. Please add parameter checks (ufs != RT_NULL && ufs->regs != RT_NULL) and return -RT_EINVAL, consistent with other UFS PM APIs in this file.
中文：rt_ufs_dme_set() 在调用 rt_ufs_uic_cmd_send() 前未检查 ufs/ufs->regs，若调用方传入 NULL 或未初始化的 host，会在 rt_ufs_uic_cmd_send() 内解引用导致崩溃。建议增加参数检查（ufs != RT_NULL 且 ufs->regs != RT_NULL）并返回 -RT_EINVAL，与本文件其他 UFS PM API 的风格保持一致。
| if ((ufs == RT_NULL) || (ufs->regs == RT_NULL)) | |
| { | |
| return -RT_EINVAL; | |
| } |
| #define __BIT_FIELD(v, h, l) ((v & RT_GENMASK(h, l)) >> l) | ||
|
|
||
| /* Host Capabilities */ | ||
| #define RT_UFS_REG_CAP 0x00 /* Host Controller Capabiities */ | ||
| #define RT_UFS_REG_CAP_NUTRS(v) __BIT_FIELD(v, 4, 0) /* Number of UTP Transfer Request Slots */ | ||
| #define RT_UFS_REG_CAP_NORTT(v) __BIT_FIELD(v, 15, 8) /* Number of outstanding READY TO TRANSFER requests supported */ |
There was a problem hiding this comment.
[best_practices/最佳实践]: __BIT_FIELD uses a double-underscore identifier reserved for the implementation.
English: Please rename this helper macro to a non-reserved name (e.g. RT_UFS_BIT_FIELD/RT_BIT_FIELD_GET) to avoid undefined behavior / namespace clashes with toolchains.
中文：__BIT_FIELD 使用了双下划线开头的标识符，属于 C 标准保留命名，可能与编译器/库发生命名冲突。建议改为非保留名称（例如 RT_UFS_BIT_FIELD / RT_BIT_FIELD_GET）。
| scsi->ops = &ufs_host_ops; | ||
| scsi->max_id = rt_max_t(rt_size_t, scsi->max_id, 1); | ||
| scsi->max_lun = rt_max_t(rt_size_t, scsi->max_lun, 1); | ||
| scsi->parallel_io = RT_TRUE; |
There was a problem hiding this comment.
[concurrency/并发]: scsi->parallel_io is set to RT_TRUE, but this UFS implementation uses a single shared slot (RT_UFS_SLOT_ID=0) and shared UTRD/UCD buffers, so concurrent block I/O can race/corrupt descriptors.
English: Please set scsi->parallel_io to RT_FALSE (so blk layer serializes via usr_lock), or add proper per-request resources + locking to make ufs_utp_transfer re-entrant.
中文：这里将 scsi->parallel_io 设为 RT_TRUE，但当前实现只使用单个共享 slot（RT_UFS_SLOT_ID=0）且 UTRD/UCD 缓冲区共享，并发读写会产生竞态并可能破坏描述符/数据。建议将 scsi->parallel_io 设为 RT_FALSE（由块层串行化），或实现可重入的请求资源与锁。
| scsi->parallel_io = RT_TRUE; | |
| /* Use single-threaded I/O: UFS driver shares a single UTP slot/UTRD/UCD. */ | |
| scsi->parallel_io = RT_FALSE; |
拉取/合并请求描述：(PR description)
[
Support UFS over PCI, too.
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up