You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/10/05 19:56:57 UTC
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6965: nxmutex replace nxsem when used as a lock
pkarashchenko commented on code in PR #6965:
URL: https://github.com/apache/incubator-nuttx/pull/6965#discussion_r985285801
##########
arch/arm/src/cxd56xx/cxd56_udmac.c:
##########
@@ -238,7 +239,7 @@ void cxd56_udmainitialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, CXD56_DMA_NCHANNELS);
Review Comment:
Let's add `nxsem_set_protocol(&g_dmac.chansem, SEM_PRIO_NONE);`? Or waiting on DMA channel should rise priority as well?
##########
arch/arm/src/lc823450/lc823450_adc.c:
##########
@@ -538,12 +507,12 @@ struct adc_dev_s *lc823450_adcinitialize(void)
inst->nchannels = CONFIG_LC823450_ADC_NCHANNELS;
inst->chanlist = lc823450_chanlist;
- nxsem_init(&inst->sem_excl, 0, 1);
+ nxmutex_init(&inst->lock);
#ifndef CONFIG_ADC_POLLED
nxsem_init(&inst->sem_isr, 0, 0);
Review Comment:
```suggestion
nxsem_init(&inst->sem_isr, 0, 0);
nxsem_set_protocol(&inst->sem_isr, SEM_PRIO_NONE);
```
##########
arch/arm/src/efm32/efm32_usbhost.c:
##########
@@ -5277,10 +5259,10 @@ static inline void efm32_sw_initialize(struct efm32_usbhost_s *priv)
usbhost_devaddr_initialize(&priv->rhport);
- /* Initialize semaphores */
+ /* Initialize semaphores & mutex */
nxsem_init(&priv->pscsem, 0, 0);
Review Comment:
```suggestion
nxsem_init(&priv->pscsem, 0, 0);
nxsem_set_protocol(&priv->pscsem, SEM_PRIO_NONE);
```
##########
arch/arm/src/rp2040/rp2040_dmac.c:
##########
@@ -160,7 +161,7 @@ void weak_function arm_dma_initialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, RP2040_DMA_NCHANNELS);
Review Comment:
```suggestion
nxsem_init(&g_dmac.chansem, 0, RP2040_DMA_NCHANNELS);
nxsem_set_protocol(&g_dmac.chansem, SEM_PRIO_NONE);
```
##########
arch/arm/src/sam34/sam_dmac.c:
##########
@@ -1380,9 +1341,9 @@ void weak_function arm_dma_initialize(void)
putreg32(DMAC_EN_ENABLE, SAM_DMAC_EN);
- /* Initialize semaphores */
+ /* Initialize mutex & semaphores */
- nxsem_init(&g_chsem, 0, 1);
+ nxmutex_init(&g_chlock);
nxsem_init(&g_dsem, 0, CONFIG_SAM34_NLLDESC);
Review Comment:
```suggestion
nxsem_init(&g_dsem, 0, CONFIG_SAM34_NLLDESC);
nxsem_set_protocol(&g_dsem, SEM_PRIO_NONE);
```
Optionally: we can do static init for both `g_dsem` and `g_chlock`
##########
arch/arm/src/lc823450/lc823450_i2s.c:
##########
@@ -686,7 +677,7 @@ static int lc823450_i2s_send(struct i2s_dev_s *dev, struct ap_buffer_s *apb,
/* Wait for Audio Buffer */
- ret = _i2s_semtake(&_sem_buf_under);
+ ret = nxsem_wait_uninterruptible(&_sem_buf_under);
Review Comment:
Lets add `nxsem_set_protocol(&XXX, SEM_PRIO_NONE);` for `_sem_rxdma`, `_sem_buf_over`, `_sem_txdma` and `_sem_buf_under`
##########
arch/arm/src/lc823450/lc823450_sddrv_dep.c:
##########
@@ -427,7 +418,7 @@ SINT_T sddep_read(void *src, void *dst, UI_32 size, SINT_T type,
}
lc823450_dmastart(_hrdma[ch], dma_callback, &_sem_rwait[ch]);
- return _sddep_semtake(&_sem_rwait[ch]);
+ return nxsem_wait_uninterruptible(&_sem_rwait[ch]);
Review Comment:
Lets add `nxsem_set_protocol(&XXX, SEM_PRIO_NONE);` for `_sem_rwait` and `_sem_wwait`
##########
arch/arm/src/efm32/efm32_dma.c:
##########
@@ -265,7 +266,7 @@ void weak_function arm_dma_initialize(void)
/* Initialize the channel list */
- nxsem_init(&g_dmac.exclsem, 0, 1);
+ nxmutex_init(&g_dmac.lock);
nxsem_init(&g_dmac.chansem, 0, EFM32_DMA_NCHANNELS);
Review Comment:
Let's add `nxsem_set_protocol(&g_dmac.chansem, SEM_PRIO_NONE);`? Or waiting on DMA channel should rise priority as well?
##########
arch/arm/src/sama5/sam_ssc.c:
##########
@@ -931,7 +884,7 @@ static struct sam_buffer_s *ssc_buf_allocate(struct sam_ssc_s *priv)
* have at least one free buffer container.
*/
- ret = ssc_bufsem_take(priv);
+ ret = nxsem_wait_uninterruptible(&priv->bufsem);
Review Comment:
Lets add `nxsem_set_protocol(&priv->bufsem, SEM_PRIO_NONE);`
##########
arch/arm/src/lpc54xx/lpc54_usb0_ohci.c:
##########
@@ -3884,10 +3829,10 @@ struct usbhost_connection_s *lpc54_usbhost_initialize(int controller)
usbhost_devaddr_initialize(&priv->rhport);
- /* Initialize semaphores */
+ /* Initialize semaphores & mutex */
nxsem_init(&priv->pscsem, 0, 0);
Review Comment:
```suggestion
nxsem_init(&priv->pscsem, 0, 0);
```
##########
arch/arm/src/sama5/sam_dmac.c:
##########
@@ -1873,9 +1855,9 @@ void sam_dmainitialize(struct sam_dmac_s *dmac)
sam_putdmac(dmac, DMAC_EN_ENABLE, SAM_DMAC_EN_OFFSET);
- /* Initialize semaphores */
+ /* Initialize muttex & semaphores */
- nxsem_init(&dmac->chsem, 0, 1);
+ nxmutex_init(&dmac->chlock);
nxsem_init(&dmac->dsem, 0, SAM_NDMACHAN);
Review Comment:
```suggestion
nxsem_init(&dmac->dsem, 0, SAM_NDMACHAN);
nxsem_set_protocol(&dmac->dsem, SEM_PRIO_NONE);
```
##########
arch/arm/src/rtl8720c/amebaz_driver.c:
##########
@@ -112,7 +112,7 @@ static void amebaz_state_deinit(struct amebaz_state_s *state)
static int amebaz_state_init(struct amebaz_state_s *state)
{
- if (nxsem_init(&state->mutex, 0, 0) != OK)
+ if (nxsem_init(&state->sem, 0, 0) != OK)
{
return -ENOMEM;
}
Review Comment:
```suggestion
if (nxsem_init(&state->sem, 0, 0) != OK)
{
return -ENOMEM;
}
nxsem_set_protocol(&state->sem, SEM_PRIO_NONE);
```
Also I do not think that `nxsem_init` can ever fail here. The error check can be removed
##########
arch/arm/src/sam34/sam_aes.c:
##########
@@ -59,8 +59,8 @@
* Private Data
****************************************************************************/
-static sem_t g_samaes_lock;
-static bool g_samaes_initdone = false;
+static mutex_t g_samaes_lock;
+static bool g_samaes_initdone = false;
Review Comment:
```suggestion
static bool g_samaes_initdone;
```
##########
arch/arm/src/sam34/sam_aes.c:
##########
@@ -169,7 +159,7 @@ static int samaes_setup_mr(uint32_t keysize, int mode, int encrypt)
static int samaes_initialize(void)
{
- nxsem_init(&g_samaes_lock, 0, 1);
+ nxmutex_init(&g_samaes_lock);
Review Comment:
We can do a static init instead
##########
arch/arm/src/sama5/sam_ehci.c:
##########
@@ -4881,7 +4828,7 @@ struct usbhost_connection_s *sam_ehci_initialize(int controller)
/* Initialize the EHCI state data structure */
- nxsem_init(&g_ehci.exclsem, 0, 1);
+ nxmutex_init(&g_ehci.lock);
nxsem_init(&g_ehci.pscsem, 0, 0);
Review Comment:
```suggestion
nxsem_init(&g_ehci.pscsem, 0, 0);
```
##########
arch/arm/src/sama5/sam_ohci.c:
##########
@@ -4016,7 +3963,7 @@ struct usbhost_connection_s *sam_ohci_initialize(int controller)
/* Initialize the state data structure */
nxsem_init(&g_ohci.pscsem, 0, 0);
Review Comment:
```suggestion
nxsem_init(&g_ohci.pscsem, 0, 0);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org