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.

Sign up for GitHub

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

Jump to bottom

Add a simple re-entrancy lock for tu_printf. #2653

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add a simple re-entrancy lock for tu_printf. #2653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/common/tusb_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
#include <stddef.h>
#include <string.h>
#include <stdio.h>
#include <stdarg.h>

// Tinyusb Common Headers
#include "tusb_option.h"
Expand Down
4 changes: 3 additions & 1 deletion src/common/tusb_debug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,14 @@ extern char const* const tu_str_xfer_result[];
#endif

void tu_print_mem(void const *buf, uint32_t count, uint8_t indent);
int tu_printf_r(const char *format, ...);

#ifdef CFG_TUSB_DEBUG_PRINTF
// External debug print function, must be ISR-safe
extern int CFG_TUSB_DEBUG_PRINTF(const char *format, ...);
#define tu_printf CFG_TUSB_DEBUG_PRINTF
#else
#define tu_printf printf
#define tu_printf tu_printf_r
#endif

static inline void tu_print_buf(uint8_t const* buf, uint32_t bufsize) {
Expand Down
16 changes: 16 additions & 0 deletions src/tusb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,22 @@ void tu_print_mem(void const* buf, uint32_t count, uint8_t indent) {
dump_str_line(buf8 - nback, nback);
}

int tu_printf_r(const char *format, ...)
{
// Simple re-entrancy lock, not safe for multi-core MCU
static volatile bool lock = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Even on a single core, volatile is not sufficient to prevent inconsistencies due to ISR execution interrupting another instance executing from user space. This needs an actual critical section, or similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you make a real example with disassembly ?

Copy link

Choose a reason for hiding this comment

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

The problem is with this sequence:

  if (lock) {
    return -1;
  }
  // <--- ISR COMES HERE
  lock = true;

To make that work on uniprocessor with interrupt, then interrupt must be disabled between the test and the setting to true:

something like:

  disable_interrupts();
  if (lock) {
    return -1;
  }
  lock = true;
  enable_interrupts()

Copy link

Choose a reason for hiding this comment

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

And this is of course wrong, more like:

  bool is_locked;
  disable_interrupts();
  is_locked = lock;
  if (!is_locked)
      lock = true;
  enable_interrupts()  
  if (is_locked)
      return (-1);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi,

The problem is with this sequence:

  if (lock) {
    return -1;
  }
  // <--- ISR COMES HERE
  lock = true;

When ISR happened here, vprintf hasn't been executed, so ISR will execute vprintf in it's context then return to normal context. There is no case where the execution of vprintf is interrupted.

Copy link

Choose a reason for hiding this comment

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

Actually you're right, sorry. need more coffee.

Copy link

Choose a reason for hiding this comment

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

But: what protect your IRQ from interrupting another vprintf() call, outside tinyusb?

Copy link

Choose a reason for hiding this comment

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

BTW: I'm using a stm32h747 with tinyusb, and I would love to see your DWC2 DMA patch going in, which is why I was lurking....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But: what protect your IRQ from interrupting another vprintf() call, outside tinyusb?

Good question, currently TU_LOG_* are available for user space logging.

BTW: I'm using a stm32h747 with tinyusb, and I would love to see your DWC2 DMA patch going in

We are few maintaining the repo, if you don't want to wait you can try the testing branch.

if (lock) {
return -1;
}
lock = true;
va_list args;
va_start(args, format);
int ret = vprintf(format, args);
va_end(args);
lock = false;
return ret;
}

#endif

#endif // host or device enabled
Loading