Skip to content

libusbclient/stm32n6: fix interrupt handler races #650

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

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

libusbclient/stm32n6: fix interrupt handler races #650

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
5 changes: 4 additions & 1 deletion usb/libusbclient/stm32n6-usbc/client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* %LICENSE%
*/

#include <stdatomic.h>
#include "phy.h"
#include "client.h"

Expand Down Expand Up @@ -65,8 +66,10 @@ static void usbclient_irqThread(void *arg)
while (stm_common.dc.runIrqThread == 1U) {
condWait(stm_common.dc.irqCond, stm_common.dc.irqLock, 0);

uint32_t pending = atomic_exchange(&stm_common.dc.pending_event, 0U);

/* low-speed IRQ handling */
ctrl_lifiq_handler(&stm_common.dc.pending_event);
ctrl_lifiq_handler(pending);

if ((stm_common.dc.currEvent != stm_common.dc.prevEvent) && (stm_common.dc.cbEvent != NULL)) {
stm_common.dc.cbEvent(stm_common.dc.currEvent, stm_common.dc.ctxUser);
Expand Down
8 changes: 4 additions & 4 deletions usb/libusbclient/stm32n6-usbc/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,10 @@ typedef struct _usb_dc_t {
uint8_t deviceState;
uint8_t ep0State;

uint32_t pending_event;
_Atomic uint32_t pending_event;
_Atomic uint32_t daintClear;
uint32_t irqPendingDIEPINT[ENDPOINTS_NUMBER];
uint32_t irqPendingDOEPINT[ENDPOINTS_NUMBER];
_Atomic uint32_t irqPendingDIEPINT[ENDPOINTS_NUMBER];

@jmaksymowicz jmaksymowicz Apr 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use the atomic_uint type from stdatomic.h instead? It's best to avoid using attributes that start with with underscores directly.

@jakubsmolaga jakubsmolaga Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to clearly communicate the intention that "this must be a 32bit unsigned integer", since it reflects the value of hw registers. Obviously atomic_uint will be the exact same type since unsigned int is 32bit on pretty much every sensible platform but I just wasn't sure if the intention is clear enough that way.

From my understanding the main problem with using _Atomic is that it breaks compatibility with C++ since C++ uses std::atomic<T> instead of _Atomic. I'm not sure how much of an issue that is for us.

Anyway, I'm ok with changing it but I just wanted to express my thoughts and hear what you have to say before committing the change blindly.

_Atomic uint32_t irqPendingDOEPINT[ENDPOINTS_NUMBER];
Comment thread
jakubsmolaga marked this conversation as resolved.
uint32_t diepmsk;
} usb_dc_t;

Expand Down Expand Up @@ -228,7 +228,7 @@ void *usbclient_allocBuff(uint32_t size);

extern void ctrl_init(usb_common_data_t *usb_data_in, usb_dc_t *dc_in);
extern void ctrl_hifiq_handler(uint32_t *irqStatus);
extern void ctrl_lifiq_handler(uint32_t *irqStatus);
extern void ctrl_lifiq_handler(uint32_t irqStatus);
extern void ctrl_setAddress(uint32_t addr);

extern void clbc_init(usb_common_data_t *usb_data_in, usb_dc_t *dc_in);
Expand Down
25 changes: 6 additions & 19 deletions usb/libusbclient/stm32n6-usbc/controller.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,32 +181,27 @@ void ctrl_hifiq_handler(uint32_t *irqStatus)
}


void ctrl_lifiq_handler(uint32_t *irqStatus)
void ctrl_lifiq_handler(uint32_t irqStatus)
{
uint8_t epNum;

/* USBRST */
if (IS_IRQ(*irqStatus, GINTSTS_USBRST)) {

*irqStatus &= ~(1UL << GINTSTS_USBRST);
if (IS_IRQ(irqStatus, GINTSTS_USBRST)) {
ctrl_common.dc->currEvent = USBCLIENT_EV_RESET;

_clbc_USBRST();
}

/* ENUMDNE */
if (IS_IRQ(*irqStatus, GINTSTS_ENUMDNE)) {

if (IS_IRQ(irqStatus, GINTSTS_ENUMDNE)) {
ctrl_common.dc->deviceState = USBD_STATE_DEFAULT;
*irqStatus &= ~(1UL << GINTSTS_ENUMDNE);
ctrl_common.dc->currEvent = USBCLIENT_EV_CONNECT;

_clbc_ENUMDNE();
}

/* USBSUSP */
if (IS_IRQ(*irqStatus, GINTSTS_USBSUSP)) {

if (IS_IRQ(irqStatus, GINTSTS_USBSUSP)) {
ctrl_common.dc->deviceState = USBD_STATE_DEFAULT;
ctrl_common.dc->ep0State = USBD_EP0_IDLE;
ctrl_common.dc->currEvent = USBCLIENT_EV_DISCONNECT;
Expand All @@ -223,23 +218,15 @@ void ctrl_lifiq_handler(uint32_t *irqStatus)
if (ctrl_common.dc->semBulkTx.v == 0U) {
semaphoreUp(&ctrl_common.dc->semBulkTx);
}

*irqStatus &= ~(1UL << GINTSTS_USBSUSP);
}

/* OEPINT */
if (IS_IRQ(*irqStatus, GINTSTS_OEPINT)) {

*irqStatus &= ~(1UL << GINTSTS_OEPINT);

if (IS_IRQ(irqStatus, GINTSTS_OEPINT)) {
_clbc_OEPINT();
}

/* IEPINT */
if (IS_IRQ(*irqStatus, GINTSTS_IEPINT)) {

*irqStatus &= ~(1UL << GINTSTS_IEPINT);

if (IS_IRQ(irqStatus, GINTSTS_IEPINT)) {
_clbc_IEPINT();
}
}
47 changes: 2 additions & 45 deletions usb/libusbclient/stm32n6-usbc/controller_callbacks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,13 +693,11 @@ void _clbc_OEPINT(void)
ep = &clbc_common.data->endpts[epNum];

if ((daintClear & 0x1U) != 0U) {
epInt = clbc_common.dc->irqPendingDOEPINT[epNum];
epInt = atomic_exchange(&clbc_common.dc->irqPendingDOEPINT[epNum], 0U);
Comment thread
jakubsmolaga marked this conversation as resolved.
epInt &= USB_REG(DOEPMSK);

/* XFRC */
if (IS_IRQ(epInt, DOEPINT_XFRC)) {
clbc_common.dc->irqPendingDOEPINT[epNum] &= ~(1UL << DOEPINT_XFRC);

/* Control transfer */
if (epNum == 0U) {
/* if STATUS OUT received */
Expand Down Expand Up @@ -734,42 +732,20 @@ void _clbc_OEPINT(void)

/* STUP */
if (IS_IRQ(epInt, DOEPINT_STUP)) {
clbc_common.dc->irqPendingDOEPINT[epNum] &= ~(1UL << DOEPINT_STUP);

if (((epInt >> DOEPINT_STSPHSRX) & 0x1U) == 0x1U) {
clbc_common.dc->irqPendingDOEPINT[epNum] &= ~(1UL << DOEPINT_STSPHSRX);
}

clbc_common.dc->ep0State = USBD_EP0_SETUP;

desc_setup(&clbc_common.dc->setupPacket);
}

/* OTEPDIS */
if (IS_IRQ(epInt, DOEPINT_OTEPDIS)) {
clbc_common.dc->irqPendingDOEPINT[epNum] &= ~(1UL << DOEPINT_OTEPDIS);
}

/* EPDISD */
if (IS_IRQ(epInt, DOEPINT_EPDISD)) {
clbc_common.dc->irqPendingDOEPINT[epNum] &= ~(1UL << DOEPINT_EPDISD);

if ((USB_REG(GINTSTS) >> 7) & 1) {
USB_REG(DCTL) |= (1 << 10);
}
}

/* STSPHSRX / OTEPSPR */
if (IS_IRQ(epInt, DOEPINT_STSPHSRX)) {

clbc_common.dc->irqPendingDOEPINT[epNum] &= ~(1UL << DOEPINT_STSPHSRX);
}

/* NAK */
if (IS_IRQ(epInt, DOEPINT_NAK)) {

clbc_common.dc->irqPendingDOEPINT[epNum] &= ~(1UL << DOEPINT_NAK);

/* check if more data are to be received */
if (epNum == 0U) {
if ((USB_REG(DOEPTSIZ0 + epNum * EP_STRIDE) & (0x3FFUL << DOEPTSIZ_PKTCNT)) != 0UL) {
Expand Down Expand Up @@ -810,13 +786,11 @@ void _clbc_IEPINT(void)

epMsk |= ((epEmp >> (epNum & 0xFU)) & 0x1U) << 7;

epInt = clbc_common.dc->irqPendingDIEPINT[epNum];
epInt = atomic_exchange(&clbc_common.dc->irqPendingDIEPINT[epNum], 0U);
epInt &= epMsk;

/* XFRC */
if (IS_IRQ(epInt, DIEPINT_XFRC)) {
clbc_common.dc->irqPendingDIEPINT[epNum] &= ~(1UL << DIEPINT_XFRC);

/* mask TX FIFO empty interrupt */
USB_REG(DIEPEMPMSK) &= ~(1UL << epNum);

Expand Down Expand Up @@ -852,25 +826,8 @@ void _clbc_IEPINT(void)
}
}

/* TOC */
if (IS_IRQ(epInt, DIEPINT_TOC)) {
clbc_common.dc->irqPendingDIEPINT[epNum] &= ~(1UL << DIEPINT_TOC);
}

/* ITTXFE */
if (IS_IRQ(epInt, DIEPINT_ITTXFE)) {
clbc_common.dc->irqPendingDIEPINT[epNum] &= ~(1UL << DIEPINT_ITTXFE);
}

/* INEPNE */
if (IS_IRQ(epInt, DIEPINT_INEPNE)) {
clbc_common.dc->irqPendingDIEPINT[epNum] &= ~(1UL << DIEPINT_INEPNE);
}

/* EPDISD */
if (IS_IRQ(epInt, DIEPINT_EPDISD)) {
clbc_common.dc->irqPendingDIEPINT[epNum] &= ~(1UL << DIEPINT_EPDISD);

clbc_flushTxFifo(epNum);
}
}
Expand Down
Loading