MT1959 flashing support#377
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds MT1959 firmware flashing: new exported module implements drive-specific patching and chunked WRITE_BUFFER flashing, SCSI READ_BUFFER support, CLI registration, and build integration. (≤50 words) Changes
Sequence DiagramsequenceDiagram
participant CLI as "User / CLI"
participant Dispatcher as "Command Dispatcher"
participant MT1959 as "drive.flash.mt1959"
participant SPTD as "SPTD / SCSI Layer"
participant Drive as "MT1959 Device"
CLI->>Dispatcher: Issue "flash::mt1959"
Dispatcher->>MT1959: redumper_flash_mt1959(ctx, options)
MT1959->>MT1959: Load firmware, validate sizes, derive firmware_id
MT1959->>SPTD: cmd_read_buffer(mode, offset, length)
SPTD->>Drive: READ BUFFER CDB (0x3C)
Drive-->>SPTD: Return config data
SPTD-->>MT1959: Provide config bytes
MT1959->>MT1959: Patch firmware (FF fill, inject config)
MT1959->>SPTD: WRITE BUFFER chunks (DOWNLOAD_MICROCODE_WITH_OFFSETS)
SPTD->>Drive: Send chunk
Drive-->>SPTD: Acknowledge / status
SPTD-->>MT1959: Report status/progress
MT1959->>SPTD: cmd_drive_ready() (retry loop)
MT1959-->>Dispatcher: return 0 (on success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 36 minutes and 22 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drive/flash_mt1959.ixx`:
- Around line 33-55: The post-flash readiness check in flash_mt1959 is currently
ignored: call cmd_drive_ready(sptd), capture its status/return value and verify
it indicates the drive re-entered ready state, and on failure log or throw
similarly to the other SCSI failures (e.g. use throw_line("drive failed to
become ready after flash, SCSI ({})", SPTD::StatusMessage(status)) or an
equivalent processLogger.error path); ensure you reference cmd_drive_ready and
SPTD::StatusMessage to build the error message so flashing does not report
success when the device remains not-ready.
- Around line 69-73: The modify_firmware function currently assumes
firmware_data is large enough and can OOB-write; add explicit size validation at
the start of modify_firmware(std::span<uint8_t> firmware_data, uint32_t
config_offset, std::span<const uint8_t> drive_config): check
firmware_data.size() >= 0x10000 and firmware_data.size() >=
static_cast<size_t>(config_offset) + drive_config.size(), and if either check
fails, fail fast (throw a clear exception such as std::out_of_range or return an
error) instead of proceeding to std::fill and std::copy; keep the existing
fill/copy logic but only execute it after these validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 878ae1f4-188e-4320-8aec-3dc74cc4b422
📒 Files selected for processing (6)
CMakeLists.txtdrive/flash_mt1959.ixxoptions.ixxredumper.ixxscsi/cmd.ixxscsi/mmc.ixx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
drive/flash_mt1959.ixx (1)
63-72:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
i = wait_seconds - 2causes an infinite loop on successful flash.When
cmd_drive_readyreturns success,iis set towait_seconds - 2 = 13. The for-loop's own++ithen advancesito 14, which still satisfies14 < 15, so the loop body runs again. On that next iteration the drive is still ready,iis reset to 13 again,++i→ 14, and the cycle repeats forever. Every successful flash will hang the process indefinitely.A secondary problem that persists from the prior review: if the 15-second window expires with the drive still not ready, the function falls through and logs "flashing success" anyway.
Both are fixed by using
breakand checking the outcome:🐛 Proposed fix
- constexpr uint32_t wait_seconds = 15; - for(uint32_t i = 0; i < wait_seconds; ++i) - { - LOGC_RF("waiting for drive to become ready... [{}/{}]", i + 1, wait_seconds); - if(!cmd_drive_ready(sptd).status_code) - i = wait_seconds - 2; - std::this_thread::sleep_for(std::chrono::seconds(1)); - } - - LOGC_RF(""); - LOGC("flashing success"); + constexpr uint32_t wait_seconds = 15; + bool drive_ready = false; + for(uint32_t i = 0; i < wait_seconds; ++i) + { + LOGC_RF("waiting for drive to become ready... [{}/{}]", i + 1, wait_seconds); + if(!cmd_drive_ready(sptd).status_code) + { + drive_ready = true; + break; + } + std::this_thread::sleep_for(std::chrono::seconds(1)); + } + LOGC(""); + if(!drive_ready) + throw_line("drive did not become ready after flashing"); + LOGC("flashing success");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drive/flash_mt1959.ixx` around lines 63 - 72, The loop uses i = wait_seconds - 2 which creates an infinite loop; instead call cmd_drive_ready(sptd) once per iteration and if it indicates success, break out of the for-loop (do not mutate i). After the loop, check the final readiness result (store the cmd_drive_ready return in a variable) and only call LOGC("flashing success") when that result was successful; otherwise log an error/timeout and handle failure accordingly. Ensure you update references around wait_seconds, cmd_drive_ready, LOGC_RF, and LOGC to use the stored result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drive/flash_mt1959.ixx`:
- Around line 86-87: The error thrown when cmd_read_buffer fails omits the SCSI
status; update the failure path in the cmd_read_buffer call (the if that checks
status.status_code) to include the SPTD::StatusMessage(status) in the throw_line
message so the thrown error contains diagnostic details (referencing
cmd_read_buffer, SPTD::StatusMessage, throw_line, config, config_offset and
READ_BUFFER_Mode::DOWNLOAD_MICROCODE_WITH_OFFSETS).
---
Duplicate comments:
In `@drive/flash_mt1959.ixx`:
- Around line 63-72: The loop uses i = wait_seconds - 2 which creates an
infinite loop; instead call cmd_drive_ready(sptd) once per iteration and if it
indicates success, break out of the for-loop (do not mutate i). After the loop,
check the final readiness result (store the cmd_drive_ready return in a
variable) and only call LOGC("flashing success") when that result was
successful; otherwise log an error/timeout and handle failure accordingly.
Ensure you update references around wait_seconds, cmd_drive_ready, LOGC_RF, and
LOGC to use the stored result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
1 participant
Summary by CodeRabbit
New Features
Documentation