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

modify DEV_ZERO config on Nuttx's built-in board #15223

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

luckyyaojin
Copy link

Note: Please adhere to Contributing Guidelines.

Summary

*/dev/zero is a very commonly used config, for example use
/dev/zero to test the filesystem, mtd or block devices performance.
So let's default enable it when not enable DEFAULT_SMALL

Impact

Nuttx's built-in boards' defconfig

Testing

CI

@nuttxpr
Copy link

nuttxpr commented Dec 17, 2024

[Experimental Bot, please feedback here]

No, this PR summary does not adequately meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is the change necessary? The summary mentions a common use case, but doesn't explain why it should be default enabled. Is it frequently requested? Does it solve a specific problem? Justify the change beyond just its utility. For example: "Enabling /dev/zero by default simplifies performance testing and reduces configuration overhead for users, as it's a commonly used device."
  • What functional part of the code is being changed? Vague reference to "Nuttx's built-in boards' defconfig". Be specific. Which files are modified? Is this a Kconfig change? Which subsystem is affected (Device drivers, core OS, etc.)? For example: "This change modifies the default configuration in the Kconfig files for various boards to enable the /dev/zero device driver when DEFAULT_SMALL is not enabled."
  • How does the change exactly work? While it states what will change (default enable), it doesn't describe how. Does it involve a Kconfig option modification? Is any code added or removed? For example: "The CONFIG_DEV_ZERO option is set to 'y' in the relevant defconfig files when CONFIG_DEFAULT_SMALL is not selected."

Missing Information in Impact:

  • All impact sections are answered too briefly or with placeholders. Be explicit.
    • Is new feature added? Is existing feature changed? "Yes, existing feature changed. The /dev/zero device driver, which was previously disabled by default (unless DEFAULT_SMALL was used), is now enabled by default."
    • Impact on user: Even if the answer is NO, explain why. "No, users will not need to adapt. The /dev/zero device will be available without any explicit configuration, simplifying common use cases." If a user might have to adapt (e.g., size changes), document that!
    • Impact on build: "Potentially YES. The resulting kernel image might be slightly larger due to the inclusion of the /dev/zero driver. This impact is expected to be minimal."
    • Impact on hardware: "NO. This change affects only the kernel configuration and does not introduce any new hardware dependencies."
    • Impact on documentation: If NO, confirm: "NO, documentation updates are not required as this is a default configuration change." If YES, describe what documentation is changed.
    • Impact on security: "NO, this change has no security implications. The /dev/zero device has a well-defined and limited functionality."
    • Impact on compatibility: "NO, this change does not affect backward or forward compatibility."

Missing Information in Testing:

  • Build Host(s): Provide details: "Linux (Ubuntu 20.04), x86_64, GCC 9.4.0"
  • Target(s): Be specific! "qemu-x86_64:nsh_net", "sim:nsh", "stm32f4discovery:nsh" List all tested targets.
  • Testing logs: Absolutely essential. Show commands used and relevant output before and after the change to demonstrate that the /dev/zero device is now available and functions correctly. Example:
Before:
$ ls /dev | grep zero
# No output

After:
$ ls /dev | grep zero
zero

$ dd if=/dev/zero of=/tmp/test bs=1k count=10
10+0 records in
10+0 records out
10240 bytes (10 kB) copied, 0.000180119 s, 56.8 MB/s

By providing this level of detail, reviewers can properly assess the PR and ensure it meets the project's standards. Conciseness is good, but not at the expense of clarity and completeness.

@luckyyaojin luckyyaojin force-pushed the dev_zero branch 2 times, most recently from 31a4d6e to b4d5e66 Compare December 17, 2024 07:37
@luckyyaojin luckyyaojin force-pushed the dev_zero branch 3 times, most recently from 4a4ab15 to f3596ad Compare December 18, 2024 05:18
@luckyyaojin luckyyaojin force-pushed the dev_zero branch 3 times, most recently from 835b9b8 to b4f93eb Compare December 18, 2024 08:55
@acassis
Copy link
Contributor

acassis commented Dec 18, 2024

@luckyyaojin /dev/null also should be enabled by default, it is very useful as /dev/zero

@xiaoxiang781216
Copy link
Contributor

@luckyyaojin /dev/null also should be enabled by default, it is very useful as /dev/zero

let's create a new patch for this, @luckyyaojin

@luckyyaojin
Copy link
Author

@luckyyaojin /dev/null also should be enabled by default, it is very useful as /dev/zero

let's create a new patch for this, @luckyyaojin

@xiaoxiang781216 @acassis /dev/null is already enabled by default

@luckyyaojin luckyyaojin force-pushed the dev_zero branch 4 times, most recently from eac2376 to 53f7eb6 Compare December 20, 2024 03:45
Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI error is in esp32c3-devkit/knsh config:

Configuration/Tool: esp32c3-devkit/knsh
2024-12-22 09:31:23
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
Error: ./chip/esp32c3_region.c:44:4: error: #error "ESP32C3_REGION_PROTECTION shall not be enabled with Protected Mode"
   44 | #  error "ESP32C3_REGION_PROTECTION shall not be enabled with Protected Mode"
      |    ^~~~~
ERROR: riscv-none-elf-gcc failed: 1

Any ideas?

/dev/zero is a very commonly used config, for example use
/dev/zero to test the filesystem, mtd or block devices performance.
So let's default enable it when not enable DEFAULT_SMALL

Signed-off-by: Bowen Wang <[email protected]>
@luckyyaojin
Copy link
Author

luckyyaojin commented Dec 23, 2024

The CI error is in esp32c3-devkit/knsh config:

Configuration/Tool: esp32c3-devkit/knsh
2024-12-22 09:31:23
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
Error: ./chip/esp32c3_region.c:44:4: error: #error "ESP32C3_REGION_PROTECTION shall not be enabled with Protected Mode"
   44 | #  error "ESP32C3_REGION_PROTECTION shall not be enabled with Protected Mode"
      |    ^~~~~
ERROR: riscv-none-elf-gcc failed: 1

Any ideas?

@hartmannathan
Due to incorrect modifications, I have now restored the file,
After another CI check, there should be no problem

@xiaoxiang781216
Copy link
Contributor

@luckyyaojin could you check your change locally before updating patch by tools/refresh.sh --silent?

@luckyyaojin
Copy link
Author

luckyyaojin commented Dec 24, 2024

@luckyyaojin could you check your change locally before updating patch by tools/refresh.sh --silent?

@xiaoxiang781216 I have already checked locally by the cmd 'tools/refresh.sh --silent all ' , let me check again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants