Add a simple re-entrancy lock for tu_printf.

[HiFiPhile]

Describe the PR
Add a simple re-entrancy lock for TU_LOG, should resolve #2652 in most cases. An interlocked version is needed for multicore MCU.

[tlyu]

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.

[HiFiPhile]

Could you make a real example with disassembly ?

[ceedriic]

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()  

[ceedriic]

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);

[HiFiPhile]

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.

[ceedriic]

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

[ceedriic]

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

[ceedriic]

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....

[HiFiPhile]

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.

[hathach]

Thanks @HiFiPhile, though I think printf lock should be done in user space by implementing (nano)libc lock in _write_r(). Since not all retarget needs lock e.g. segger rtt

[HiFiPhile]

though I think printf lock should be done in user space by implementing (nano)libc lock in _write_r(). Since not all retarget needs lock e.g. segger rtt

The problem is libc's printf() itself is not reentrant as it works on global data, not just the I/O part.

https://github.com/bminor/newlib/blob/1ed15161362b2eb5649b049b7ab0aaf8097bd43a/newlib/libc/stdio/printf.c#L52

[hathach]

though I think printf lock should be done in user space by implementing (nano)libc lock in _write_r(). Since not all retarget needs lock e.g. segger rtt

The problem is libc's printf() itself is not reentrant as it works on global data, not just the I/O part.

https://github.com/bminor/newlib/blob/1ed15161362b2eb5649b049b7ab0aaf8097bd43a/newlib/libc/stdio/printf.c#L52

I may miss a thing though: the newlib reentrant implementation seems to the the exact same thing as this. For logging within ISR, we only do log in isr with level 3, which I used mainly with segger RTT. I will revise the code to see if there is any log1/log2 in the isr.

PS: oh, I see you mean the global _REENT is also used by other libc like malloc() and not only printf(). For this, how about moving this guard into the _write(), I still think it should be done at user level.

[HiFiPhile]

Eh... I think reentrant is kind of implementation dependent, I just read IAR's manual:

Most parts of the DLIB runtime environment are reentrant, but the following functions
and parts are not reentrant because they need static data:
Heap functions—malloc, free, realloc, calloc, etc. and the C++ operators new and delete
...
Functions that use files or the heap in some way. This includes scanf, sscanf, getchar, getwchar,
putchar, and putwchar. In addition, if you are using the options --printf_multibytes and --dlib_config=Full,
the printf and sprintf functions (or any variants) can also use the heap.
...
Remedies for this are:
● Do not use non-reentrant functions in interrupt service routines
● Guard calls to a non-reentrant function by a mutex, or a secure region, etc