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

SYSLOG_DEFAULT: wrap up_putc/up_nputs calls with critical section #14722

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 11, 2024

Summary

This would avoid the undesirable intertactions with the serial driver described in #14662.

Although I'm not entirely happy with this fix because it assumes the particular implementations of up_putc/up_nputc and its association to the serial devices, I haven't come up with better ideas for now.

An alternative is to place some serializations inside the target specific serial (and/or whatever provides up_putc api) implementaitons. But it isn't too attractive to put potentially complex logic into the low-level machinaries, especially when we have a lot of similar copies of it.

Another alternative is to deprecate up_putc. (at least for the purpose of syslog.) But it seems at least some of users are relying on what the current implementation provides heavily.

This commit also removes g_lowputs_lock because the critical section would serve the purpose of the lock as well.

Impact

SYSLOG_DEFAULT

Testing

Tested on esp32s3-devkitc with a several unrelated local patches.

This would avoid the undesirable intertactions with the serial driver
described in apache#14662.

Although I'm not entirely happy with this fix because it assumes
the particular implementations of up_putc/up_nputc and its association
to the serial devices, I haven't come up with better ideas for now.

An alternative is to place some serializations inside the target
specific serial (and/or whatever provides up_putc api) implementaitons.
But it isn't too attractive to put potentially complex logic into the
low-level machinaries, especially when we have a lot of similar copies
of it.

Another alternative is to deprecate up_putc. (at least for the purpose
of syslog.) But it seems at least some of users are relying on what
the current implementation provides heavily.

This commit also removes g_lowputs_lock because the critical section
would serve the purpose of the lock as well.
@github-actions github-actions bot added Area: Drivers Drivers issues Size: S The size of the change in this PR is small labels Nov 11, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 11, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. Here's why:

  • Incomplete Summary: While the summary explains the "why," it lacks sufficient detail on the "what" and "how." It mentions avoiding undesirable interactions and assumptions about up_putc/up_nputc, but doesn't concretely describe the code changes. What code was modified? How does the critical section prevent the undesirable interactions?

  • Missing Impact Details: Stating "SYSLOG_DEFAULT" is insufficient. While it might imply some things, the PR needs to explicitly address all impact points. Does this change the build process? Does it affect any specific architectures, boards, or drivers? Does it require documentation updates? Even if the answer is "NO" for most, it needs to be stated clearly for each item. The security and compatibility impacts are especially important to address.

  • Insufficient Testing Information: "Tested on esp32s3-devkitc with a several unrelated local patches" is inadequate. List the relevant local patches. More importantly, the "Testing logs before change" and "Testing logs after change" sections are empty. Provide concrete log output demonstrating the issue before the change and the successful resolution after the change. Also specify the build host details (OS, compiler version, etc.).

To make this PR acceptable, address the following:

  1. Expand the Summary: Clearly explain what code was changed and how the changes address the issue described in [BUG] low level console i/o (up_putc) can interfere interrupt-based uart i/o #14662. Be specific.

  2. Complete the Impact Assessment: Explicitly answer each impact question (build, hardware, documentation, security, compatibility) with either "NO" or "YES (and provide details)."

  3. Provide Thorough Testing Information:

    • List relevant local patches.
    • Fill in the "Testing logs before change" and "Testing logs after change" sections with actual log output.
    • Specify the build host details (OS, compiler, etc.). Ideally, test on more than one configuration if feasible.

By providing this missing information, the PR will be much more likely to be reviewed and accepted.

@xiaoxiang781216 xiaoxiang781216 merged commit f2aeb5e into apache:master Nov 12, 2024
27 checks passed
@pussuw
Copy link
Contributor

pussuw commented Nov 12, 2024

Question here; is syslog supposed to be usable before the OS has been initialized, i.e. nx_start has run to completion?

After updating to latest upstream a syslog trace I have very early on during the boot process (before nx_start) causes a crash in

cpu = this_cpu();
rtcb = current_task(cpu);
DEBUGASSERT(rtcb != NULL);

Obviously there is no task that is running since the OS has not been initialized yet.

If syslog is not supposed to be used in early init, which tracing works (if any) ?

@tmedicci
Copy link
Contributor

This commit raised a lot of issues on Espressif SoCs:

HW testing is failing for the following configs:

  • esp32-devkitc:wifi_smp
  • esp32-devkitc:sta_softap
  • esp32-devkitc:smp_hw
  • esp32-devkitc:psram_usrheap
  • esp32-devkitc:psram
  • esp32-devkitc:ble

The device fails to boot without any other log output. Can you please take a look, @yamt ?

@yamt
Copy link
Contributor Author

yamt commented Nov 13, 2024

@pussuw @tmedicci thank you. i submitted a revert for now. #14751

xiaoxiang781216 pushed a commit that referenced this pull request Nov 13, 2024
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Nov 13, 2024
So that it can be used in more situations.

The primary motivation here is to avoid crashes introduced by
apache#14722.

Tested:
- esp32-devkitc:wifi_smp (smp)
- esp32s3-devkit:smp (ostest, smp)
  (with apache#14755)
@yamt
Copy link
Contributor Author

yamt commented Nov 13, 2024

@pussuw @tmedicci thank you. i submitted a revert for now. #14751

#14761 contains a fix.

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.

8 participants