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 2021/09/14 16:12:46 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #4543: esp32: Use device specific lock as much as possible.
Ouss4 opened a new pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543
## Summary
This MR, whenever possible, replaces calls to enter/leave_critical_section with spin_lock_irqsave/restore using a device specific lock.
- See #2213
## Impact
ESP32 drivers in SMP mode.
## Testing
All of ESP32 defconfigs were tested.
--
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#issuecomment-944459916
@masayuki2009 this PR should be ready to be merged. Other issues are addressed by #4670 and #4669
--
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#issuecomment-922547779
@masayuki2009 I'm seeing an issue with this defconfig with ESP32_WIFI_SAVE_PARAM enabled, which is also present in the master branch. I'll be taking a look tomorrow morning.
--
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
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#issuecomment-922539677
@Ouss4
It looks good to me but esp32-devkitc:wapi_smp caused a deadlock when booting.
--
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
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r710054028
##########
File path: arch/xtensa/src/esp32/esp32_rt_timer.c
##########
@@ -60,17 +61,27 @@
#define ESP32_RT_TIMER 0 /* Timer 0 */
/****************************************************************************
- * Private Data
+ * Private Types
****************************************************************************/
-static int s_pid;
+struct esp32_rt_priv_s
+{
+ int pid;
+
Review comment:
These spaces were added deliberately to separate members of different purpose.
--
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#issuecomment-922446033
@masayuki2009 can you please take a look?
--
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
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r710061800
##########
File path: arch/xtensa/src/esp32/esp32_wdt_lowerhalf.c
##########
@@ -72,14 +75,15 @@
struct esp32_wdt_lowerhalf_s
{
- FAR const struct watchdog_ops_s *ops; /* Lower half operations */
- FAR struct esp32_wdt_dev_s *wdt; /* esp32 watchdog driver */
- uint32_t timeout; /* The current timeout */
- enum wdt_peripherals peripheral; /* Indicates if it is from RTC or Timer Module */
- uint32_t lastreset; /* The last reset time */
- bool started; /* True: Timer has been started */
- xcpt_t handler; /* User Handler */
- void *upper; /* Pointer to watchdog_upperhalf_s */
+ FAR const struct watchdog_ops_s *ops; /* Lower half operations */
+ FAR struct esp32_wdt_dev_s *wdt; /* esp32 watchdog driver */
+ uint32_t timeout; /* The current timeout */
+ enum wdt_peripherals peripheral; /* Indicates if it is from RTC or Timer Module */
+ uint32_t lastreset; /* The last reset time */
+ bool started; /* True: Timer has been started */
+ xcpt_t handler; /* User Handler */
+ void *upper; /* Pointer to watchdog_upperhalf_s */
+ spinlock_t lock; /* Device specific lock */
Review comment:
I initially haven't changed the original alignment, I just reduced the spaces. But now I've changed it to make the members aligned as other source codes.
--
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
[GitHub] [incubator-nuttx] Ouss4 removed a comment on pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 removed a comment on pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#issuecomment-919301289
PS. I'll squash all of those "device specific lock" commits when the review is done.
--
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#issuecomment-919301289
PS. I'll squash all of those "device specific lock" commits when the review is done.
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r708454727
##########
File path: arch/xtensa/src/esp32/esp32_rt_timer.c
##########
@@ -216,24 +219,22 @@ static void stop_rt_timer(FAR struct rt_timer_s *timer)
if (ishead)
{
- if (!list_is_empty(&s_runlist))
+ if (!list_is_empty(&priv->runlist))
{
/* Set the value from the next timer as the new hardware timer
* alarm value
*/
- next_timer = container_of(s_runlist.next,
+ next_timer = container_of(priv->runlist.next,
struct rt_timer_s,
list);
alarm = next_timer->alarm;
- ESP32_TIM_SETALRVL(tim, alarm);
- ESP32_TIM_SETALRM(tim, true);
+ ESP32_TIM_SETALRVL(priv->timer, alarm);
+ ESP32_TIM_SETALRM(priv->timer, true);
}
}
}
-
- leave_critical_section(flags);
Review comment:
Ok, I just saw in the end of the file you're externally protecting it.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r708444717
##########
File path: arch/xtensa/src/esp32/esp32_rt_timer.c
##########
@@ -60,17 +61,27 @@
#define ESP32_RT_TIMER 0 /* Timer 0 */
/****************************************************************************
- * Private Data
+ * Private Types
****************************************************************************/
-static int s_pid;
+struct esp32_rt_priv_s
+{
+ int pid;
+
+ sem_t toutsem;
+
Review comment:
```suggestion
```
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r708444582
##########
File path: arch/xtensa/src/esp32/esp32_rt_timer.c
##########
@@ -60,17 +61,27 @@
#define ESP32_RT_TIMER 0 /* Timer 0 */
/****************************************************************************
- * Private Data
+ * Private Types
****************************************************************************/
-static int s_pid;
+struct esp32_rt_priv_s
+{
+ int pid;
+
Review comment:
```suggestion
```
--
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#issuecomment-922543079
> It looks good to me but esp32-devkitc:wapi_smp caused a deadlock when booting.
It did? I tested that defconfig multiple times. Let me try once again.
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r708452671
##########
File path: arch/xtensa/src/esp32/esp32_rt_timer.c
##########
@@ -216,24 +219,22 @@ static void stop_rt_timer(FAR struct rt_timer_s *timer)
if (ishead)
{
- if (!list_is_empty(&s_runlist))
+ if (!list_is_empty(&priv->runlist))
{
/* Set the value from the next timer as the new hardware timer
* alarm value
*/
- next_timer = container_of(s_runlist.next,
+ next_timer = container_of(priv->runlist.next,
struct rt_timer_s,
list);
alarm = next_timer->alarm;
- ESP32_TIM_SETALRVL(tim, alarm);
- ESP32_TIM_SETALRM(tim, true);
+ ESP32_TIM_SETALRVL(priv->timer, alarm);
+ ESP32_TIM_SETALRM(priv->timer, true);
}
}
}
-
- leave_critical_section(flags);
Review comment:
Why did you remove the lock from the `stop_rt_timer` and `start_rt_timer`?
Can't we have an error in case one rt timer client call one of these 2 functions at the same time that another rt timer client does?
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r708456272
##########
File path: arch/xtensa/src/esp32/esp32_rt_timer.c
##########
@@ -631,15 +646,17 @@ uint64_t IRAM_ATTR rt_timer_get_alarm(void)
void IRAM_ATTR rt_timer_calibration(uint64_t time_us)
{
uint64_t counter;
- struct esp32_tim_dev_s *tim = s_esp32_tim_dev;
+ struct esp32_rt_priv_s *priv = &g_rt_priv;
irqstate_t flags;
- flags = enter_critical_section();
- ESP32_TIM_GETCTR(tim, &counter);
+ flags = spin_lock_irqsave(&priv->lock);
+
Review comment:
```suggestion
```
--
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
[GitHub] [incubator-nuttx] saramonteiro commented on a change in pull request #4543: esp32: Use device specific lock as much as possible.
Posted by GitBox <gi...@apache.org>.
saramonteiro commented on a change in pull request #4543:
URL: https://github.com/apache/incubator-nuttx/pull/4543#discussion_r708460816
##########
File path: arch/xtensa/src/esp32/esp32_wdt_lowerhalf.c
##########
@@ -72,14 +75,15 @@
struct esp32_wdt_lowerhalf_s
{
- FAR const struct watchdog_ops_s *ops; /* Lower half operations */
- FAR struct esp32_wdt_dev_s *wdt; /* esp32 watchdog driver */
- uint32_t timeout; /* The current timeout */
- enum wdt_peripherals peripheral; /* Indicates if it is from RTC or Timer Module */
- uint32_t lastreset; /* The last reset time */
- bool started; /* True: Timer has been started */
- xcpt_t handler; /* User Handler */
- void *upper; /* Pointer to watchdog_upperhalf_s */
+ FAR const struct watchdog_ops_s *ops; /* Lower half operations */
+ FAR struct esp32_wdt_dev_s *wdt; /* esp32 watchdog driver */
+ uint32_t timeout; /* The current timeout */
+ enum wdt_peripherals peripheral; /* Indicates if it is from RTC or Timer Module */
+ uint32_t lastreset; /* The last reset time */
+ bool started; /* True: Timer has been started */
+ xcpt_t handler; /* User Handler */
+ void *upper; /* Pointer to watchdog_upperhalf_s */
+ spinlock_t lock; /* Device specific lock */
Review comment:
nit: alignment.
--
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