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/03/11 20:14:06 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #5721: Remove the unnecessary code from nx_start
xiaoxiang781216 opened a new pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721
## Summary
- fs: Initialize g_inode_sem at the definition place
- mm/shm: Initialize shm_info_s at the definition place
- sched/wdog: Remove wd_initialize which isn't really used anymore
- mm/iob: Remove initialized static variable inside iob_initialize
- sched/irq: Remove the code which zero out g_irqvector fields
- sched/init: Move binfmt_initialize before hardware initialization
## Impact
Minor, code refactor
## Testing
Pass CI and local test
--
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 commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825242870
##########
File path: sched/init/nx_start.c
##########
@@ -628,15 +628,6 @@ void nx_start(void)
irq_initialize();
}
- /* Initialize the watchdog facility (if included in the link) */
-
-#ifdef CONFIG_HAVE_WEAKFUNCTIONS
- if (wd_initialize != NULL)
-#endif
Review comment:
> `wd_initialize` is defined as a weak, so the user is able to override it with a custom initialization procedure. So, to a certain extent, this is a breaking change, isn't it?
>
As I understand, the weak used here want to avoid the linker bring the unused code/data if nobody call wd_xxx function, that's why the code check wd_initialize before invocation.
> It will impact anyone that may be relying on this.
The better approach is modify wd_initialize directly.
--
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 commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825242594
##########
File path: sched/wdog/wd_initialize.c
##########
@@ -50,29 +50,3 @@ clock_t g_wdtickbase;
/****************************************************************************
* Public Functions
****************************************************************************/
-
-/****************************************************************************
- * Name: wd_initialize
- *
- * Description:
- * This function initializes the watchdog data structures
- *
- * Input Parameters:
- * None
- *
- * Returned Value:
- * None
- *
- * Assumptions:
- * This function must be called early in the initialization sequence
- * before the timer interrupt is attached and before any watchdog
- * services are used.
- *
- ****************************************************************************/
-
-void wd_initialize(void)
-{
- /* Initialize watchdog lists */
-
- sq_init(&g_wdactivelist);
Review comment:
Yes, please look sq_init definition:
https://github.com/apache/incubator-nuttx/blob/master/include/queue.h#L34-L40
--
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] pkarashchenko commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825310381
##########
File path: mm/iob/iob_initialize.c
##########
@@ -95,54 +95,46 @@ sem_t g_qentry_sem; /* Counts free I/O buffer queue containers */
void iob_initialize(void)
{
- static bool initialized = false;
int i;
- /* Perform one-time initialization */
+ /* Add each I/O buffer to the free list */
- if (!initialized)
+ for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
{
- /* Add each I/O buffer to the free list */
+ FAR struct iob_s *iob = &g_iob_pool[i];
- for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
- {
- FAR struct iob_s *iob = &g_iob_pool[i];
+ /* Add the pre-allocate I/O buffer to the head of the free list */
- /* Add the pre-allocate I/O buffer to the head of the free list */
-
- iob->io_flink = g_iob_freelist;
- g_iob_freelist = iob;
- }
+ iob->io_flink = g_iob_freelist;
+ g_iob_freelist = iob;
+ }
- g_iob_committed = NULL;
+ g_iob_committed = NULL;
- nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
+ nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
#if CONFIG_IOB_THROTTLE > 0
- nxsem_init(&g_throttle_sem,
- 0,
- CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
+ nxsem_init(&g_throttle_sem,
+ 0,
+ CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
#endif
#if CONFIG_IOB_NCHAINS > 0
/* Add each I/O buffer chain queue container to the free list */
- for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
- {
- FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
+ for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
+ {
+ FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
- /* Add the pre-allocate buffer container to the head of the free
- * list
- */
+ /* Add the pre-allocate buffer container to the head of the free
+ * list
+ */
- iobq->qe_flink = g_iob_freeqlist;
- g_iob_freeqlist = iobq;
- }
+ iobq->qe_flink = g_iob_freeqlist;
+ g_iob_freeqlist = iobq;
+ }
- g_iob_qcommitted = NULL;
+ g_iob_qcommitted = NULL;
- nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
+ nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
Review comment:
Maybe we should use
```
sem_t g_iob_sem = SEM_INITIALIZER(CONFIG_IOB_NBUFFERS);
#if CONFIG_IOB_THROTTLE > 0
sem_t g_throttle_sem = SEM_INITIALIZER(CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
#endif
#if CONFIG_IOB_NCHAINS > 0
sem_t g_qentry_sem = SEM_INITIALIZER(CONFIG_IOB_NCHAINS);
#endif
```
instead after we inited `initialized` is removed
--
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] pkarashchenko commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825101900
##########
File path: mm/iob/iob_initialize.c
##########
@@ -95,54 +95,48 @@ sem_t g_qentry_sem; /* Counts free I/O buffer queue containers */
void iob_initialize(void)
{
- static bool initialized = false;
int i;
- /* Perform one-time initialization */
+ /* Add each I/O buffer to the free list */
- if (!initialized)
+ for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
{
- /* Add each I/O buffer to the free list */
+ FAR struct iob_s *iob = &g_iob_pool[i];
- for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
- {
- FAR struct iob_s *iob = &g_iob_pool[i];
+ /* Add the pre-allocate I/O buffer to the head of the free list */
- /* Add the pre-allocate I/O buffer to the head of the free list */
-
- iob->io_flink = g_iob_freelist;
- g_iob_freelist = iob;
- }
+ iob->io_flink = g_iob_freelist;
+ g_iob_freelist = iob;
+ }
- g_iob_committed = NULL;
+ g_iob_committed = NULL;
- nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
+ nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
#if CONFIG_IOB_THROTTLE > 0
- nxsem_init(&g_throttle_sem,
- 0,
- CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
+ nxsem_init(&g_throttle_sem,
+ 0,
+ CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
#endif
#if CONFIG_IOB_NCHAINS > 0
/* Add each I/O buffer chain queue container to the free list */
- for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
- {
- FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
+ for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
+ {
+ FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
- /* Add the pre-allocate buffer container to the head of the free
- * list
- */
+ /* Add the pre-allocate buffer container to the head of the free
+ * list
+ */
- iobq->qe_flink = g_iob_freeqlist;
- g_iob_freeqlist = iobq;
- }
+ iobq->qe_flink = g_iob_freeqlist;
+ g_iob_freeqlist = iobq;
+ }
- g_iob_qcommitted = NULL;
+ g_iob_qcommitted = NULL;
- nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
+ nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
#endif
- initialized = true;
- }
+ initialized = true;
Review comment:
Why do we need to keep this line?
##########
File path: sched/wdog/wd_initialize.c
##########
@@ -50,29 +50,3 @@ clock_t g_wdtickbase;
/****************************************************************************
* Public Functions
****************************************************************************/
-
-/****************************************************************************
- * Name: wd_initialize
- *
- * Description:
- * This function initializes the watchdog data structures
- *
- * Input Parameters:
- * None
- *
- * Returned Value:
- * None
- *
- * Assumptions:
- * This function must be called early in the initialization sequence
- * before the timer interrupt is attached and before any watchdog
- * services are used.
- *
- ****************************************************************************/
-
-void wd_initialize(void)
-{
- /* Initialize watchdog lists */
-
- sq_init(&g_wdactivelist);
Review comment:
So we do nor need this call anymore? Why?
--
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] gustavonihei commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825080609
##########
File path: sched/init/nx_start.c
##########
@@ -628,15 +628,6 @@ void nx_start(void)
irq_initialize();
}
- /* Initialize the watchdog facility (if included in the link) */
-
-#ifdef CONFIG_HAVE_WEAKFUNCTIONS
- if (wd_initialize != NULL)
-#endif
Review comment:
`wd_initialize` is defined as a weak, so the user is able to override it with a custom initialization procedure.
So, to a certain extent, this is a breaking change, isn't it?
It will impact anyone that may be relying on this.
--
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] gustavonihei commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825080609
##########
File path: sched/init/nx_start.c
##########
@@ -628,15 +628,6 @@ void nx_start(void)
irq_initialize();
}
- /* Initialize the watchdog facility (if included in the link) */
-
-#ifdef CONFIG_HAVE_WEAKFUNCTIONS
- if (wd_initialize != NULL)
-#endif
Review comment:
`wd_initialize` is defined as a weak function, so the user is able to override it with a custom initialization procedure.
So, to a certain extent, this is a breaking change, isn't it?
It will impact anyone that may be relying on this.
--
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 commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825314176
##########
File path: mm/iob/iob_initialize.c
##########
@@ -95,54 +95,46 @@ sem_t g_qentry_sem; /* Counts free I/O buffer queue containers */
void iob_initialize(void)
{
- static bool initialized = false;
int i;
- /* Perform one-time initialization */
+ /* Add each I/O buffer to the free list */
- if (!initialized)
+ for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
{
- /* Add each I/O buffer to the free list */
+ FAR struct iob_s *iob = &g_iob_pool[i];
- for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
- {
- FAR struct iob_s *iob = &g_iob_pool[i];
+ /* Add the pre-allocate I/O buffer to the head of the free list */
- /* Add the pre-allocate I/O buffer to the head of the free list */
-
- iob->io_flink = g_iob_freelist;
- g_iob_freelist = iob;
- }
+ iob->io_flink = g_iob_freelist;
+ g_iob_freelist = iob;
+ }
- g_iob_committed = NULL;
+ g_iob_committed = NULL;
- nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
+ nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
#if CONFIG_IOB_THROTTLE > 0
- nxsem_init(&g_throttle_sem,
- 0,
- CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
+ nxsem_init(&g_throttle_sem,
+ 0,
+ CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
#endif
#if CONFIG_IOB_NCHAINS > 0
/* Add each I/O buffer chain queue container to the free list */
- for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
- {
- FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
+ for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
+ {
+ FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
- /* Add the pre-allocate buffer container to the head of the free
- * list
- */
+ /* Add the pre-allocate buffer container to the head of the free
+ * list
+ */
- iobq->qe_flink = g_iob_freeqlist;
- g_iob_freeqlist = iobq;
- }
+ iobq->qe_flink = g_iob_freeqlist;
+ g_iob_freeqlist = iobq;
+ }
- g_iob_qcommitted = NULL;
+ g_iob_qcommitted = NULL;
- nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
+ nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
Review comment:
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] gustavonihei commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825080609
##########
File path: sched/init/nx_start.c
##########
@@ -628,15 +628,6 @@ void nx_start(void)
irq_initialize();
}
- /* Initialize the watchdog facility (if included in the link) */
-
-#ifdef CONFIG_HAVE_WEAKFUNCTIONS
- if (wd_initialize != NULL)
-#endif
Review comment:
`wd_initialize` is defined as a weak, so the user is able to override it with a custom initialization procedure.
So, to a certain extent, this is a breaking change, isn't 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 commented on a change in pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721#discussion_r825242548
##########
File path: mm/iob/iob_initialize.c
##########
@@ -95,54 +95,48 @@ sem_t g_qentry_sem; /* Counts free I/O buffer queue containers */
void iob_initialize(void)
{
- static bool initialized = false;
int i;
- /* Perform one-time initialization */
+ /* Add each I/O buffer to the free list */
- if (!initialized)
+ for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
{
- /* Add each I/O buffer to the free list */
+ FAR struct iob_s *iob = &g_iob_pool[i];
- for (i = 0; i < CONFIG_IOB_NBUFFERS; i++)
- {
- FAR struct iob_s *iob = &g_iob_pool[i];
+ /* Add the pre-allocate I/O buffer to the head of the free list */
- /* Add the pre-allocate I/O buffer to the head of the free list */
-
- iob->io_flink = g_iob_freelist;
- g_iob_freelist = iob;
- }
+ iob->io_flink = g_iob_freelist;
+ g_iob_freelist = iob;
+ }
- g_iob_committed = NULL;
+ g_iob_committed = NULL;
- nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
+ nxsem_init(&g_iob_sem, 0, CONFIG_IOB_NBUFFERS);
#if CONFIG_IOB_THROTTLE > 0
- nxsem_init(&g_throttle_sem,
- 0,
- CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
+ nxsem_init(&g_throttle_sem,
+ 0,
+ CONFIG_IOB_NBUFFERS - CONFIG_IOB_THROTTLE);
#endif
#if CONFIG_IOB_NCHAINS > 0
/* Add each I/O buffer chain queue container to the free list */
- for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
- {
- FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
+ for (i = 0; i < CONFIG_IOB_NCHAINS; i++)
+ {
+ FAR struct iob_qentry_s *iobq = &g_iob_qpool[i];
- /* Add the pre-allocate buffer container to the head of the free
- * list
- */
+ /* Add the pre-allocate buffer container to the head of the free
+ * list
+ */
- iobq->qe_flink = g_iob_freeqlist;
- g_iob_freeqlist = iobq;
- }
+ iobq->qe_flink = g_iob_freeqlist;
+ g_iob_freeqlist = iobq;
+ }
- g_iob_qcommitted = NULL;
+ g_iob_qcommitted = NULL;
- nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
+ nxsem_init(&g_qentry_sem, 0, CONFIG_IOB_NCHAINS);
#endif
- initialized = true;
- }
+ initialized = true;
Review comment:
Fixed.
--
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] gustavonihei merged pull request #5721: Remove the unnecessary code from nx_start
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #5721:
URL: https://github.com/apache/incubator-nuttx/pull/5721
--
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