From 4e967c67b46bf717ef23121fdbc8ca8c441e363c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=81yszczek?= Date: Wed, 3 Aug 2022 14:57:50 +0200 Subject: [PATCH] stm32wl5: fix unbuffered mode and other possible bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch fixes unbuffered mode so it actually works. Also, patch contains fixes for possible bugs that could result in deadlock or system hang in certain situations. Signed-off-by: Michał Łyszczek --- arch/arm/src/stm32wl5/stm32wl5_ipcc.c | 85 ++++++++++++++++++--------- drivers/ipcc/ipcc_poll.c | 3 + drivers/ipcc/ipcc_read.c | 33 ++++++++++- drivers/ipcc/ipcc_write.c | 9 ++- include/nuttx/ipcc.h | 4 ++ 5 files changed, 101 insertions(+), 33 deletions(-) diff --git a/arch/arm/src/stm32wl5/stm32wl5_ipcc.c b/arch/arm/src/stm32wl5/stm32wl5_ipcc.c index 8f4565e5c0..af20acbc68 100644 --- a/arch/arm/src/stm32wl5/stm32wl5_ipcc.c +++ b/arch/arm/src/stm32wl5/stm32wl5_ipcc.c @@ -68,15 +68,17 @@ struct stm32wl5_ipcc_s unsigned rxlen; -#if CONFIG_IPCC_BUFFERED /* Number of bytes copied from IPCC memory to buffer. Can be less than * stm32wl5_ipcc_chan_mem_s.len after copy operation when buffer is full. * Value can persist between multiple ISR and stm32wl5_ipcc_buffer_data() * calls, until all data from IPCC memory is successfully buffered. + * + * When unbuffered version is used, this holds number of bytes already + * read from IPCC memory. Consecutive call to read() will read from + * this position. */ unsigned rxcopied; -#endif /* Physical memory address where we will write data for the second * CPU to read. @@ -99,10 +101,12 @@ static ssize_t stm32wl5_ipcc_read(struct ipcc_lower_s *ipcc, char *buffer, size_t buflen); static ssize_t stm32wl5_ipcc_write(struct ipcc_lower_s *ipcc, const char *buffer, size_t buflen); +#ifdef CONFIG_IPCC_BUFFERED static ssize_t stm32wl5_ipcc_buffer_data(struct ipcc_lower_s *ipcc, struct circbuf_s *rxbuf); static ssize_t stm32wl5_ipcc_copy_to_buffer(int chan, struct circbuf_s *rxbuf); +#endif static int stm32wl5_ipcc_rx_isr(int irq, void *context, void *arg); static int stm32wl5_ipcc_tx_isr(int irq, void *context, void *arg); @@ -206,16 +210,18 @@ struct stm32wl5_ipcc_s g_ipccpriv[IPCC_NCHAN] = static int stm32wl5_ipcc_tx_isr(int irq, void *context, void *arg) { int chan; - size_t nwritten; uint32_t mr; uint32_t sr; uint32_t status; struct stm32wl5_ipcc_s *priv; +#ifdef CONFIG_IPCC_BUFFERED + size_t nwritten; struct stm32wl5_ipcc_chan_mem_s *txmem; +#endif - (void)context; - (void)arg; - (void)irq; + UNUSED(context); + UNUSED(arg); + UNUSED(irq); mr = getreg32(STM32WL5_IPCC_C1MR) >> STM32WL5_IPCC_TX_SHIFT; sr = getreg32(STM32WL5_IPCC_C1TOC2SR); @@ -241,9 +247,10 @@ static int stm32wl5_ipcc_tx_isr(int irq, void *context, void *arg) /* Get internal data structure for current chan */ priv = &g_ipccpriv[chan]; + +#ifdef CONFIG_IPCC_BUFFERED txmem = (struct stm32wl5_ipcc_chan_mem_s *)priv->txmem; -#if CONFIG_IPCC_BUFFERED /* Copy as much as we can into IPCC memory, circbuf won't copy * more than there is in the buffer. */ @@ -259,17 +266,6 @@ static int stm32wl5_ipcc_tx_isr(int irq, void *context, void *arg) txmem->len = nwritten; modifyreg32(STM32WL5_IPCC_C1SCR, 0, STM32WL5_IPCC_SCR_CHNS(chan)); } -#else /* CONFIG_IPCC_BUFFERED */ - /* In unbuffered operations we only notify blocked writers, these - * writers will write to IPCC memory directly. - */ -#endif /* CONFIG_IPCC_BUFFERED */ - - /* Wake up all blocked writers that there is free space available - * in IPCC memory (or txbuffer) to write. - */ - - ipcc_txfree_notify(priv->ipcc->upper); if (circbuf_used(&priv->ipcc->txbuf) == 0) { @@ -280,6 +276,19 @@ static int stm32wl5_ipcc_tx_isr(int irq, void *context, void *arg) modifyreg32(STM32WL5_IPCC_C1MR, 0, STM32WL5_IPCC_MR_CHNFM(chan)); } +#else /* CONFIG_IPCC_BUFFERED */ + /* In unbuffered operations we never write anything to IPCC + * memory from interrupt context, so we have to mask interrupts, + * or else we will constantly get TX interrupts + */ + + modifyreg32(STM32WL5_IPCC_C1MR, 0, STM32WL5_IPCC_MR_CHNFM(chan)); +#endif /* CONFIG_IPCC_BUFFERED */ + /* Wake up all blocked writers that there is free space available + * in IPCC memory (or txbuffer) to write. + */ + + ipcc_txfree_notify(priv->ipcc->upper); } return OK; @@ -381,15 +390,17 @@ static ssize_t stm32wl5_ipcc_write(struct ipcc_lower_s *ipcc, static int stm32wl5_ipcc_rx_isr(int irq, void *context, void *arg) { int chan; - ssize_t nread; uint32_t mr; uint32_t sr; uint32_t status; struct stm32wl5_ipcc_s *priv; +#ifdef CONFIG_IPCC_BUFFERED + ssize_t nread; +#endif - (void)context; - (void)arg; - (void)irq; + UNUSED(context); + UNUSED(arg); + UNUSED(irq); mr = getreg32(STM32WL5_IPCC_C1MR); sr = getreg32(STM32WL5_IPCC_C2TOC1SR); @@ -415,15 +426,11 @@ static int stm32wl5_ipcc_rx_isr(int irq, void *context, void *arg) priv = &g_ipccpriv[chan]; -#if CONFIG_IPCC_BUFFERED +#ifdef CONFIG_IPCC_BUFFERED nread = stm32wl5_ipcc_copy_to_buffer(chan, &priv->ipcc->rxbuf); -#else /* CONFIG_IPCC_BUFFERED */ - /* In unbuffered operations we only notify blocked readers, these - * readers will read from IPCC memory directly. - */ -#endif /* CONFIG_IPCC_BUFFERED */ if (nread) +#endif /* CONFIG_IPCC_BUFFERED */ { /* Wake up all blocked readers that there is data * available to read @@ -431,6 +438,14 @@ static int stm32wl5_ipcc_rx_isr(int irq, void *context, void *arg) ipcc_rxfree_notify(priv->ipcc->upper); } + +#ifndef CONFIG_IPCC_BUFFERED + /* In unbuffered mode, we never read anything in interrupt, so + * we have to mask rxirq so we don't get that irq again. + */ + + modifyreg32(STM32WL5_IPCC_C1MR, 0, STM32WL5_IPCC_MR_CHNOM(chan)); +#endif } return OK; @@ -510,6 +525,10 @@ static ssize_t stm32wl5_ipcc_read(struct ipcc_lower_s *ipcc, modifyreg32(STM32WL5_IPCC_C1SCR, 0, STM32WL5_IPCC_SCR_CHNC(ipcc->chan)); + + /* Unmask RX interrupt to know when second CPU sends us a message */ + + modifyreg32(STM32WL5_IPCC_C1MR, STM32WL5_IPCC_MR_CHNOM(ipcc->chan), 0); } /* Reenable interrupt */ @@ -538,6 +557,7 @@ static ssize_t stm32wl5_ipcc_read(struct ipcc_lower_s *ipcc, * ****************************************************************************/ +#ifdef CONFIG_IPCC_BUFFERED static ssize_t stm32wl5_ipcc_copy_to_buffer(int chan, struct circbuf_s *rxbuf) { @@ -616,6 +636,7 @@ static ssize_t stm32wl5_ipcc_copy_to_buffer(int chan, return to_copy; } +#endif /**************************************************************************** * Name: stm32wl5_ipcc_buffer_data @@ -634,6 +655,7 @@ static ssize_t stm32wl5_ipcc_copy_to_buffer(int chan, * ****************************************************************************/ +#ifdef CONFIG_IPCC_BUFFERED static ssize_t stm32wl5_ipcc_buffer_data(struct ipcc_lower_s *ipcc, struct circbuf_s *rxbuf) { @@ -655,6 +677,7 @@ static ssize_t stm32wl5_ipcc_buffer_data(struct ipcc_lower_s *ipcc, return ret; } +#endif /**************************************************************************** * Name: stm32wl5_ipcc_write_notify @@ -673,11 +696,13 @@ static ssize_t stm32wl5_ipcc_buffer_data(struct ipcc_lower_s *ipcc, * ****************************************************************************/ +#ifdef CONFIG_IPCC_BUFFERED static ssize_t stm32wl5_ipcc_write_notify(struct ipcc_lower_s *ipcc) { modifyreg32(STM32WL5_IPCC_C1MR, STM32WL5_IPCC_MR_CHNFM(ipcc->chan), 0); return 0; } +#endif /**************************************************************************** * Name: stm32wl5_ipcc_cleanup @@ -761,9 +786,11 @@ struct ipcc_lower_s *stm32wl5_ipcc_init(int chan) ipcc->ops.read = stm32wl5_ipcc_read; ipcc->ops.write = stm32wl5_ipcc_write; + ipcc->ops.cleanup = stm32wl5_ipcc_cleanup; +#ifdef CONFIG_IPCC_BUFFERED ipcc->ops.buffer_data = stm32wl5_ipcc_buffer_data; ipcc->ops.write_notify = stm32wl5_ipcc_write_notify; - ipcc->ops.cleanup = stm32wl5_ipcc_cleanup; +#endif ipcc->chan = chan; diff --git a/drivers/ipcc/ipcc_poll.c b/drivers/ipcc/ipcc_poll.c index 775682e1e8..ccea384e78 100644 --- a/drivers/ipcc/ipcc_poll.c +++ b/drivers/ipcc/ipcc_poll.c @@ -132,6 +132,8 @@ int ipcc_poll(FAR struct file *filep, FAR struct pollfd *fds, /* Should immediately notify on any of the requested events? */ eventset = 0; + +#ifdef CONFIG_IPCC_BUFFERED if (circbuf_used(&priv->ipcc->rxbuf) > 0) { eventset |= (fds->events & POLLIN); @@ -141,6 +143,7 @@ int ipcc_poll(FAR struct file *filep, FAR struct pollfd *fds, { eventset |= (fds->events & POLLOUT); } +#endif if (eventset) { diff --git a/drivers/ipcc/ipcc_read.c b/drivers/ipcc/ipcc_read.c index 733575f147..7084d9e7d4 100644 --- a/drivers/ipcc/ipcc_read.c +++ b/drivers/ipcc/ipcc_read.c @@ -122,6 +122,7 @@ ssize_t ipcc_read(FAR struct file *filep, FAR char *buffer, FAR struct ipcc_driver_s *priv; ssize_t nread; int ret; + int flags; /* Get our private data structure */ @@ -141,11 +142,33 @@ ssize_t ipcc_read(FAR struct file *filep, FAR char *buffer, for (; ; ) { + /* Disable interrupts, or we might get in situation when we: + * - read 0 bytes from circbuf + * - interrupt comes in + * - it copies data to buffer and notifies blocked readers + * (in this case sem count is 0, so no sem_post() is called) + * - interrupt ends + * - since we are in blocking mode, and we read 0 from buffer + * we call sem_wait() and we hang in there cause rx interrupt + * will not be triggered again. + */ + + flags = enter_critical_section(); #ifdef CONFIG_IPCC_BUFFERED /* Data is buffered in interrupt handler, so we simply * have to return buffers content to the user */ + if (circbuf_used(&priv->ipcc->rxbuf)) + { + /* There is some data on buffer, we are sure we won't block + * so immediately leave critical section to not block other + * more important drivers + */ + + leave_critical_section(flags); + } + if ((nread = circbuf_read(&priv->ipcc->rxbuf, buffer, buflen)) > 0) { /* got some data */ @@ -176,18 +199,18 @@ ssize_t ipcc_read(FAR struct file *filep, FAR char *buffer, nxsem_post(&priv->exclsem); return nread; } - #else /* CONFIG_IPCC_BUFFERED */ /* Unbuffered read, read data directly from lower driver */ - if ((nread = priv->ipcc->ops.read(&priv->ipcc, buffer, buflen)) != 0) + if ((nread = priv->ipcc->ops.read(priv->ipcc, buffer, buflen)) != 0) { /* Got some data, return number of bytes read to the caller * --or-- * read() returned error in which case return errno value */ + leave_critical_section(flags); nxsem_post(&priv->exclsem); return nread; } @@ -201,11 +224,15 @@ ssize_t ipcc_read(FAR struct file *filep, FAR char *buffer, * no data could be read */ + leave_critical_section(flags); nxsem_post(&priv->exclsem); return -EAGAIN; } - /* We are in blocking mode, so we have to wait for data to arrive */ + /* We are in blocking mode, so we have to wait for data to arrive. + * nxsem_wait() will atomically leave critical section for us so + * we don't have to do it. + */ nxsem_post(&priv->exclsem); if ((ret = nxsem_wait(&priv->rxsem))) diff --git a/drivers/ipcc/ipcc_write.c b/drivers/ipcc/ipcc_write.c index bfe420f96d..d1ec717461 100644 --- a/drivers/ipcc/ipcc_write.c +++ b/drivers/ipcc/ipcc_write.c @@ -124,6 +124,7 @@ ssize_t ipcc_write(FAR struct file *filep, FAR const char *buffer, FAR struct ipcc_driver_s *priv; ssize_t nwritten; int ret; + int flags; /* Get our private data structure */ @@ -143,6 +144,7 @@ ssize_t ipcc_write(FAR struct file *filep, FAR const char *buffer, for (nwritten = 0; ; ) { + flags = enter_critical_section(); #ifdef CONFIG_IPCC_BUFFERED /* Buffered write, if buffer is empty try to write directly to * IPCC memory, else buffer data in circbuf - it will be written @@ -169,6 +171,7 @@ ssize_t ipcc_write(FAR struct file *filep, FAR const char *buffer, */ nxsem_post(&priv->exclsem); + leave_critical_section(flags); return nwritten; } } @@ -191,13 +194,15 @@ ssize_t ipcc_write(FAR struct file *filep, FAR const char *buffer, /* All outstanding data has been copied to txbuffer, we're done */ nxsem_post(&priv->exclsem); + leave_critical_section(flags); return nwritten; } #else /* CONFIG_IPCC_BUFFERED */ /* Unbuffered write, write data directly to lower driver */ - nwritten += priv->ipcc->ops.write(&priv->ipcc, buffer, buflen); + nwritten += priv->ipcc->ops.write(priv->ipcc, buffer + nwritten, + buflen - nwritten); if (nwritten == (ssize_t)buflen) { /* We've managed to write whole buffer to IPCC memory, @@ -210,6 +215,7 @@ ssize_t ipcc_write(FAR struct file *filep, FAR const char *buffer, */ nxsem_post(&priv->exclsem); + leave_critical_section(flags); return nwritten; } @@ -224,6 +230,7 @@ ssize_t ipcc_write(FAR struct file *filep, FAR const char *buffer, */ nxsem_post(&priv->exclsem); + leave_critical_section(flags); return nwritten ? nwritten : -EAGAIN; } diff --git a/include/nuttx/ipcc.h b/include/nuttx/ipcc.h index 863513e3c9..6e1d70dd0f 100644 --- a/include/nuttx/ipcc.h +++ b/include/nuttx/ipcc.h @@ -114,8 +114,10 @@ struct ipcc_ops_s * **************************************************************************/ +#ifdef CONFIG_IPCC_BUFFERED CODE ssize_t (*buffer_data)(FAR struct ipcc_lower_s *ipcc, FAR struct circbuf_s *rxbuf); +#endif /************************************************************************** * Name: write_notify @@ -133,7 +135,9 @@ struct ipcc_ops_s * **************************************************************************/ +#ifdef CONFIG_IPCC_BUFFERED CODE ssize_t (*write_notify)(FAR struct ipcc_lower_s *ipcc); +#endif /************************************************************************** * Name: cleanup