[STM32 FSDEV] Fix ISR race conditions

[Okarss]

Doing RMW (read-modify-write) operations on interrupt status bits is always broken because the interrupts, which become pending during the RMW operation, are cleared without being processed. The same issue is discussed in details in libopencm3/libopencm3#392.

This particular bug was introduced with commit 412b557 by @HiFiPhile . I'm noting it to just serve as a reminder for everyone to be careful with RMW operations, especially for interrupt and other status bits. Because of this bug for me TinyUSB v0.16 on STM32L432 was failing completely 95% of the time. As this is a critical bug and affects about half of the STM32 devices, maybe this fix even deserves a v0.16.1 release?

[HiFiPhile]

Thanks for your PR, indeed I missed ISTR register is rc_w0.

Could you give me more details about your usage case, because the issue is not caught in stm32l412 HIL test setup and our stm32l072 board in mass production ?

[Okarss]

In my case it's a custom self-powered device based on STM32L432KBU6, which implements a single MSC class device. There is no RTOS, just a simple cooperative scheduler. The compiler used is GCC v10.3. With O0 the USB often (but not always) worked, but with Og, O1 or anything higher it almost never worked. The TinyUSB log showed that the stack issues too many resets (more than the 3 typical ones). It could be that the fine timing nuances depend on the host device. In my case it is a rather old HP OEM PC with Core 2 Duo and Windows 7 x64.

[Okarss]

Took a look on that failed test for F1 series. It fails because the registers on F1 are defined as 16-bit. Just adding a cast to uint16_t doesn't seem future proof and actually H5 series already have some status bits above the bit 15. Maybe it's worth introducing some typedef fsdev_register_t, which should be defined to uint16_t for some series and uint32_t for all the other by default?

[HiFiPhile]

Maybe it's worth introducing some typedef fsdev_register_t, which should be defined to uint16_t for some series and uint32_t for all the other by default?

I agree. I believe since G0 the IP upgraded to support host operation and ISTR grows to 32 bit.

[Okarss]

Reviewed all of the related ST reference manuals and header files and found out that the following factors are equal for all devices:

• Register width in the header file
• Register width in the reference manual
• PMA access width in the reference manual

As registers and PMA are accessed through the same bus interface, most likely access width is limited by the bus interface itself. The access width is 16-bit for all older devices and 32-bit for the newer G0, H5, U5. That means the PMA_32BIT_ACCESS macro already serves for this purpose. Also it seems that the PMA related code in future could also benefit from the new typedef. For example pcd_set_endpoint() and pcd_get_endpoint() can easily be unified/simplified.

Added a commit with a new fsdev_bus_t typedef. As now those identifiers would serve not just for PMA description, but as a generic FSDEV hardware implementation parameters, for the sake of consistency I would also rename these:

• PMA_32BIT_ACCESS => FSDEV_BUS_32BIT
• PMA_LENGTH => FSDEV_PMA_SIZE

Should I add such a commit also or leave those as is?

[hathach]

Added a commit with a new fsdev_bus_t typedef. As now those identifiers would serve not just for PMA description, but as a generic FSDEV hardware implementation parameters, for the sake of consistency I would also rename these:

• PMA_32BIT_ACCESS => FSDEV_BUS_32BIT
• PMA_LENGTH => FSDEV_PMA_SIZE

Should I add such a commit also or leave those as is?

yeah, please do it while we are here

As registers and PMA are accessed through the same bus interface, most likely access width is limited by the bus interface itself. The access width is 16-bit for all older devices and 32-bit for the newer G0, H5, U5. That means the PMA_32BIT_ACCESS macro already serves for this purpose. Also it seems that the PMA related code in future could also benefit from the new typedef. For example pcd_set_endpoint() and pcd_get_endpoint() can easily be unified/simplified.

You are right, we could refactor/cleanup those bus-width related function later to get rid of #ifdef.

[Okarss]

Renamed the macros along with a few minor changes. I guess this PR is now ready to go.

By the way... @hathach and everyone involved, thanks for a decent USB stack! In my opinion this project is on a path to become what the FreeRTOS is for RTOSes and lwIP for IP stacks - the number one choice.

[hathach]

perfect, thank you for the fix