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

drivers/bch: Add CONFIG_BCH_FORCE_INDIRECT #16121

Merged
merged 1 commit into from
Apr 3, 2025

Conversation

jpaali
Copy link
Contributor

@jpaali jpaali commented Apr 2, 2025

Summary

The PR adds CONFIG_BCH_FORCE_INDIRECT which makes possible to force indirect writes in BCH driver.

The implementation is similar than CONFIG_FAT_FORCE_INDIRECT.
This is needed because in some use cases (e.g. when CONFIG_BUILD_KERNEL) it is not possible to write directly from user buffer.

Impact

This PR adds configuration option CONFIG_BCH_FORCE_INDIRECT which is disabled by default.
Enabling CONFIG_BCH_FORCE_INDIRECT forces indirect write in bchlib_write().

Testing

Tested with mkfatfs utility on the custom MPFS board, BUILD_KERNEL enabled.

@github-actions github-actions bot added Area: Drivers Drivers issues Size: S The size of the change in this PR is small labels Apr 2, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 2, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it's missing some crucial details. While the summary and impact sections provide a good starting point, the testing section is insufficient.

Here's what needs improvement:

  • Summary: While the "what" is described, the "why" needs more detail. Why is it impossible to write directly from the user buffer in some cases (e.g., CONFIG_BUILD_KERNEL)? Explain the underlying issue that necessitates forcing indirect writes. A link to a related NuttX issue would significantly strengthen this section.

  • Impact: While the impact on the user, build, and hardware is addressed, other areas are missing:

    • Documentation: Does this new configuration option require documentation updates? If so, have those been included in the PR?
    • Security: Are there any security implications of forcing indirect writes? Even if the answer is no, explicitly stating it is important.
    • Compatibility: Are there any backward compatibility concerns? Does this change break existing configurations?
  • Testing: This is the weakest part of the PR description. "Tested with mkfatfs utility" is far too vague.

    • Specifics: Which version of mkfatfs? What specific tests were run? What were the expected results?
    • Logs: The template explicitly requests "Testing logs before change" and "Testing logs after change". These are essential for demonstrating the effectiveness of the change and should include relevant output demonstrating the issue before the change and the corrected behavior after the change. Simply stating "Tested" without providing evidence is not sufficient.
    • Target Details: "custom MPFS board" is not descriptive enough. Provide the specific architecture, board configuration, and any other relevant details about the target hardware.
    • Build Host Details: Completely missing. Specify the host OS, CPU architecture, and compiler version used for testing.

In short, the PR description needs more detail and concrete evidence to demonstrate that the change is necessary, correctly implemented, and thoroughly tested. Providing this information will greatly increase the likelihood of the PR being accepted.

This implements similar functionality than CONFIG_FAT_FORCE_INDIRECT
because in some use cases (e.g. when CONFIG_BUILD_KERNEL) it is not possible to
write directly from user buffer.

Signed-off-by: Jani Paalijarvi <[email protected]>
@jpaali jpaali force-pushed the up_bch_indirect_write branch from 271e1c7 to cf7c13f Compare April 2, 2025 12:16
@jpaali jpaali requested a review from acassis April 3, 2025 07:02
@xiaoxiang781216 xiaoxiang781216 merged commit 02c9030 into apache:master Apr 3, 2025
39 checks passed
@jpaali jpaali deleted the up_bch_indirect_write branch April 3, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants