nxmutex replace nxsem when used as a lock by anjiahao1 · Pull Request #6965 · apache/nuttx

anjiahao1 · 2022-08-31T03:32:23Z

Signed-off-by: anjiahao anjiahao@xiaomi.com

Summary

pull5070 is the first step, this is step 2, then change sem default behavior

Impact

schedule

Testing

CI

anjiahao1 · 2022-08-31T03:58:01Z

this error can't fix

xiaoxiang781216 · 2022-09-03T13:52:25Z

@anjiahao1 please fix the warning:
https://github.com/apache/incubator-nuttx/runs/8127486548?check_suite_focus=true
and fix the conflict.

xiaoxiang781216 · 2022-10-14T02:20:28Z

The change is already simple and consistent now, it's very error prone and time consuming to split the change into two commit again. @pkarashchenko and I already spend a lot of time to review and refine the change. Please respect our work, especially many code is vendor specific. I already spend many time to improve the code that I don't really use. If you insist I have to drop all code that I don't care about from this patch.

@xiaoxiang781216 @pkarashchenko Thank you for your comments and efforts so far. OK, I understand the current situation. However, when you create a new PR next time, please consider dividing a commit into smaller ones that are independent. Reviewing each commit file will be much easier to find any simple mistake.

Sure, thanks for suggestion.

jerpelea · 2022-10-14T04:06:08Z

The change is already simple and consistent now, it's very error prone and time consuming to split the change into two commit again. @pkarashchenko and I already spend a lot of time to review and refine the change. Please respect our work, especially many code is vendor specific. I already spend many time to improve the code that I don't really use. If you insist I have to drop all code that I don't care about from this patch.

@xiaoxiang781216 @pkarashchenko Thank you for your comments and efforts so far. OK, I understand the current situation. However, when you create a new PR next time, please consider dividing a commit into smaller ones that are independent. Reviewing each commit file will be much easier to find any simple mistake.

Sure, thanks for suggestion.

considering the current PR and the work spent on it I think that we should do our best to merge it then we should think how we can avoid this situation by working with smaller PR.

xiaoxiang781216 · 2022-10-14T08:29:41Z

@pkarashchenko @masayuki2009 @jerpelea all comment get addressed, please review again.

masayuki2009 · 2022-10-14T13:46:19Z

@pkarashchenko @masayuki2009 @jerpelea all comment get addressed, please review again.

@xiaoxiang781216

I think it would be better to merge the following style change in the second commit into the first commit because the first commit also changes the lines.

diff --git a/arch/arm/src/cxd56xx/cxd56_udmac.c b/arch/arm/src/cxd56xx/cxd56_udmac.c
index 69d2c87aa0..6a490e9cd0 100644
--- a/arch/arm/src/cxd56xx/cxd56_udmac.c
+++ b/arch/arm/src/cxd56xx/cxd56_udmac.c
@@ -68,8 +68,8 @@ struct dma_channel_s

 struct dma_controller_s
 {
-  mutex_t lock;     /* Protects channel table */
-  sem_t chansem;    /* Count of free channels */
+  mutex_t lock;                 /* Protects channel table */
+  sem_t chansem;                /* Count of free channels */
 };

Also, I can see similar changes in the second commit.

diff --git a/arch/arm/src/rp2040/rp2040_dmac.c b/arch/arm/src/rp2040/rp2040_dmac.c
index 1604c9d829..7d54c70124 100644
--- a/arch/arm/src/rp2040/rp2040_dmac.c
+++ b/arch/arm/src/rp2040/rp2040_dmac.c
@@ -62,8 +62,8 @@ struct dma_channel_s

 struct dma_controller_s
 {
-  mutex_t lock;     /* Protects channel table */
-  sem_t chansem;    /* Count of free channels */
+  mutex_t lock;                 /* Protects channel table */
+  sem_t chansem;                /* Count of free channels */
 };

masayuki2009 · 2022-10-14T13:48:46Z

@xiaoxiang781216

The latest code passed my automatic tests.

xiaoxiang781216 · 2022-10-14T13:59:53Z

@pkarashchenko @masayuki2009 @jerpelea all comment get addressed, please review again.

@xiaoxiang781216

I think it would be better to merge the following style change in the second commit into the first commit because the first commit also changes the lines.

diff --git a/arch/arm/src/cxd56xx/cxd56_udmac.c b/arch/arm/src/cxd56xx/cxd56_udmac.c
index 69d2c87aa0..6a490e9cd0 100644
--- a/arch/arm/src/cxd56xx/cxd56_udmac.c
+++ b/arch/arm/src/cxd56xx/cxd56_udmac.c
@@ -68,8 +68,8 @@ struct dma_channel_s

 struct dma_controller_s
 {
-  mutex_t lock;     /* Protects channel table */
-  sem_t chansem;    /* Count of free channels */
+  mutex_t lock;                 /* Protects channel table */
+  sem_t chansem;                /* Count of free channels */
 };

Also, I can see similar changes in the second commit.

diff --git a/arch/arm/src/rp2040/rp2040_dmac.c b/arch/arm/src/rp2040/rp2040_dmac.c
index 1604c9d829..7d54c70124 100644
--- a/arch/arm/src/rp2040/rp2040_dmac.c
+++ b/arch/arm/src/rp2040/rp2040_dmac.c
@@ -62,8 +62,8 @@ struct dma_channel_s

 struct dma_controller_s
 {
-  mutex_t lock;     /* Protects channel table */
-  sem_t chansem;    /* Count of free channels */
+  mutex_t lock;                 /* Protects channel table */
+  sem_t chansem;                /* Count of free channels */
 };

Done, move to the first patch.

pkarashchenko

570 files reviewed

pkarashchenko · 2022-10-14T14:55:08Z

 {
-  irqstate_t irqs;
-
+  struct stm32l4_i2c_priv_s *priv = ((struct stm32l4_i2c_inst_s *)dev)->priv;


Please fix again. Here and all similar places. Move after DEBUGASSERT(dev);

xiaoxiang781216 · 2022-10-15T18:32:22Z

@pkarashchenko do you have more comment for this patch?

pkarashchenko · 2022-10-15T18:35:49Z

@pkarashchenko do you have more comment for this patch?

Please give me 1 more day to take a final look. Then we can move forward

pkarashchenko

The new batch of comments mostly related to inconsistency during initialization. I do not expect that all will be changed in this PR, but rather use this review as an opportunity to highlight the things.
All major comments like missing of = NXMUTEX_INITIALIZER; or double free I'm expecting to be fixed of course. Or maybe a separate PR should be submitted for double free fix and some other critical issues, I'm not sure.
@masayuki2009 what do you think?

pkarashchenko

Few last changes

pkarashchenko · 2022-10-16T17:32:46Z

@@ -67,7 +68,7 @@ struct st7032_dev_s
  uint8_t    col;               /* Current col position to write on display  */
  uint8_t    buffer[ST7032_MAX_ROW * ST7032_MAX_COL];
  bool       pendscroll;
-  sem_t sem_excl;
+  mutex_t lock;


Signed-off-by: anjiahao <anjiahao@xiaomi.com> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

…zation Signed-off-by: anjiahao <anjiahao@xiaomi.com> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

also correct the order to ensure the memory free is last step Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

xiaoxiang781216 · 2022-10-16T19:11:59Z

@pkarashchenko done.

pkarashchenko

LGTM!

xiaoxiang781216 · 2022-10-17T06:37:12Z

@pkarashchenko @masayuki2009 could we merge this patch now?

lupyuen · 2022-10-26T10:59:03Z

Hi @anjiahao1 BL602 I2C Read seems to hang after this commit, I think it might be caused by the Semaphore Init Value.

Refer to the changes for arch/risc-v/src/bl602/bl602_i2c.c:

d1d4633#diff-b9e7242d24fc5f206885bdcd9fa90409e9c8356d041a968e786cfb8c8db32aeb

Previously the Semaphore sem_isr was set to Init Value 1:

static void bl602_i2c_sem_init(struct bl602_i2c_priv_s *priv)
{
  ...
  nxsem_init(&priv->sem_isr, 0, 1);

Now sem_isr is set to Init Value 0:

static struct bl602_i2c_priv_s bl602_i2c0_priv =
{
  ...
  .sem_isr  = SEM_INITIALIZER(0),

This causes nxsem_wait_uninterruptible to hang during I2C Transfer:

static int bl602_i2c_transfer(struct i2c_master_s *dev,
                              struct i2c_msg_s *   msgs,
                              int                      count)
{
  ...
  // Hangs here
  ret = nxsem_wait_uninterruptible(&priv->sem_isr);

When I changed sem_isr to Init Value 1:

  .sem_isr  = SEM_INITIALIZER(1),

The I2C Transfer now works OK. (Here's the log)

Can you verify if this fix SEM_INITIALIZER(1) is correct? Thanks :-)

anjiahao1 · 2022-10-27T03:35:06Z

Sorry, I'll check all the SEM modifications later.

Hi @anjiahao1 BL602 I2C Read seems to hang after this commit, I think it might be caused by the Semaphore Init Value.

Refer to the changes for arch/risc-v/src/bl602/bl602_i2c.c:

d1d4633#diff-b9e7242d24fc5f206885bdcd9fa90409e9c8356d041a968e786cfb8c8db32aeb

Previously the Semaphore sem_isr was set to Init Value 1:
static void bl602_i2c_sem_init(struct bl602_i2c_priv_s *priv)
{
  ...
  nxsem_init(&priv->sem_isr, 0, 1);
Now sem_isr is set to Init Value 0:
static struct bl602_i2c_priv_s bl602_i2c0_priv =
{
  ...
  .sem_isr  = SEM_INITIALIZER(0),
This causes nxsem_wait_uninterruptible to hang during I2C Transfer:
static int bl602_i2c_transfer(struct i2c_master_s *dev,
                              struct i2c_msg_s *   msgs,
                              int                      count)
{
  ...
  // Hangs here
  ret = nxsem_wait_uninterruptible(&priv->sem_isr);
When I changed sem_isr to Init Value 1:
  .sem_isr  = SEM_INITIALIZER(1),
The I2C Transfer now works OK. (Here's the log)

Can you verify if this fix SEM_INITIALIZER(1) is correct? Thanks :-)

Sorry, I'll check all the SEM modifications later.

anjiahao1 force-pushed the nxmutex branch from 5a99b47 to dfaaa09 Compare August 31, 2022 03:54

xiaoxiang781216 requested a review from pkarashchenko August 31, 2022 05:52

anjiahao1 force-pushed the nxmutex branch 7 times, most recently from ef3fe2a to 6ce12d3 Compare August 31, 2022 14:14

xiaoxiang781216 reviewed Aug 31, 2022

View reviewed changes

anjiahao1 force-pushed the nxmutex branch from 6ce12d3 to 5fefcc5 Compare September 1, 2022 04:04

anjiahao1 force-pushed the nxmutex branch 17 times, most recently from 9b179fd to e93f6a4 Compare September 7, 2022 03:43

masayuki2009 approved these changes Oct 14, 2022

View reviewed changes

pkarashchenko reviewed Oct 14, 2022

View reviewed changes

pkarashchenko reviewed Oct 16, 2022

View reviewed changes

pkarashchenko approved these changes Oct 16, 2022

View reviewed changes

anjiahao1 and others added 3 commits October 17, 2022 03:10

Replace nxsem API when used as a lock with nxmutex API

ef3919e

Signed-off-by: anjiahao <anjiahao@xiaomi.com> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

arch: Replace critical section with nxmutex in i2c/spi/1wire initiali…

590e0d5

…zation Signed-off-by: anjiahao <anjiahao@xiaomi.com> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

drivers: Destroy mutex and sem in the error path

5db565c

also correct the order to ensure the memory free is last step Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

pkarashchenko approved these changes Oct 16, 2022

View reviewed changes

anjiahao1 mentioned this pull request Oct 18, 2022

sem:remove sem default protocl #7344

Closed

SPRESENSE mentioned this pull request Oct 19, 2022

Build error caused by nxmutex replace nxsem when used as a lock #7353

Closed

pkarashchenko mentioned this pull request Oct 19, 2022

ostest:add sem protocl to priority inheritance apache/nuttx-apps#1358

Merged

masayuki2009 mentioned this pull request Oct 20, 2022

esp32s2: Fix SPI #7363

Merged

anjiahao1 mentioned this pull request Oct 20, 2022

libc/semaphore:sem_init change defult protocol #5070

Merged

xiaoxiang781216 mentioned this pull request Oct 22, 2022

Fix vfs/fs_timerfd.c:73:17: error: duplicate member 'lock' #7395

Merged

anjiahao1 mentioned this pull request Oct 27, 2022

fix bl602 i2c sem init mistake #7445

Merged

xiaoxiang781216 mentioned this pull request Oct 29, 2022

Initialize global mutext/sem by NXMUTEX_INITIALIZER and SEM_INITIALIZER #7472

Merged

anjiahao1 mentioned this pull request Nov 3, 2022

fix a mistake about nxmutex & sem #7516

Merged

xiaoxiang781216 mentioned this pull request Jan 14, 2023

Support fragmentation and reassembly #8059

Merged

Uh oh!

Conversation

anjiahao1 commented Aug 31, 2022 • edited by xiaoxiang781216 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Impact

Testing

Uh oh!

anjiahao1 commented Aug 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Sep 3, 2022

Uh oh!

xiaoxiang781216 commented Oct 14, 2022

Uh oh!

jerpelea commented Oct 14, 2022

Uh oh!

xiaoxiang781216 commented Oct 14, 2022

Uh oh!

masayuki2009 commented Oct 14, 2022

Uh oh!

masayuki2009 commented Oct 14, 2022

Uh oh!

xiaoxiang781216 commented Oct 14, 2022

Uh oh!

pkarashchenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pkarashchenko Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Oct 15, 2022

Uh oh!

pkarashchenko commented Oct 15, 2022

Uh oh!

pkarashchenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pkarashchenko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pkarashchenko Oct 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Oct 16, 2022

Uh oh!

pkarashchenko left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 commented Oct 17, 2022

Uh oh!

lupyuen commented Oct 26, 2022

anjiahao1 commented Aug 31, 2022 •

edited by xiaoxiang781216

Loading

pkarashchenko left a comment •

edited

Loading