[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204: URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r822280662 ## File path: drivers/input/button_upper.c ## @@ -887,7 +801,6 @@ int btn_register(FAR const char *devname, return OK; errout_with_priv: Review comment: Done. ## File path: drivers/input/ajoystick.c ## @@ -835,7 +748,6 @@ int ajoy_register(FAR const char *devname, return OK; errout_with_priv: 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204: URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821871849 ## File path: drivers/input/ajoystick.c ## @@ -467,22 +398,20 @@ static int ajoy_close(FAR struct file *filep) priv->au_open = opriv->ao_flink; } + /* Enable/disable interrupt handling */ + + ajoy_enable(priv); + + leave_critical_section(flags); Review comment: It don't need since nobody should touch opriv once ajoy_close is enter, otherwise we can't fix the race condition as discuss above. -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821870255
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204: URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821869783 ## File path: drivers/input/ajoystick.c ## @@ -380,14 +344,12 @@ static int ajoy_open(FAR struct file *filep) ajoy_enable(priv); + leave_critical_section(flags); 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821720907
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
No problem, I enjoy very much this kind of technical conversation.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821674599
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> So what is not handled in this case?
>
> 1. Thread A just enter ajoy_close and switch to thread B. -- I assume that
this is done either before `ajoy_takesem(>au_exclsem);` or inside the
`ajoy_takesem(>au_exclsem);` call.
I mean before take au_exclsem.
> 2. Thread B just enter ajoy_read and switch to thread A again. -- This
will either switch before `ret = ajoy_takesem(>au_exclsem);` or inside
the `ret = ajoy_takesem(>au_exclsem);` call.
I mean before take au_exclsem too.
> 3. Thread A continue ajoy_close and release opriv -- Yes, so in this case
we need to access `filep->f_priv` (`opriv`) after `ret =
ajoy_takesem(>au_exclsem);` is successful and `filep->f_priv` should be
set to `NULL` after memory is freed.
Yes.
> 4. Thread B resume and touch the freed memory -- this will not happen if
`ajoy_takesem(>au_exclsem);` is successfully taken and `filep->f_priv` is
accessed after that.
>
Once ajoy_close return at the step 3, filep may be reused by VFS layer and
then f_priv may point to a different struct, which make the check of
filep->f_priv useless.
> My assumption is next: The `priv->au_exclsem` protects any modification of
`node->i_private` data and `filep->f_priv` data. If there is no modification,
then we do not need to take `priv->au_exclsem`. We can try to leverage critical
section approach. In other words the `ajoy_read` should look like:
>
> ```
> /* Get exclusive access to the driver structure */
>
> if (ret >= 0)
> {
> flags = enter_critical_section();
>
> DEBUGASSERT(filep && filep->f_inode);
> opriv = filep->f_priv;
>
> if (opriv != NULL)
> {
> opriv->ao_pollpending = false;
> ret = sizeof(struct ajoy_sample_s);
> }
> else
> {
> ret = -EPIPE;
> }
>
> leave_critical_section(flags);
> }
>
> return (ssize_t)ret;
> }
> ```
neither semaphore nor critical section can resolve the above sequence.
To handle the race condition between destruct and other paired functions,
the best approach is the reference count and delay release, other methods may
look like work, but not work really.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821674599
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> So what is not handled in this case?
>
> 1. Thread A just enter ajoy_close and switch to thread B. -- I assume that
this is done either before `ajoy_takesem(>au_exclsem);` or inside the
`ajoy_takesem(>au_exclsem);` call.
I mean before take au_exclsem.
> 2. Thread B just enter ajoy_read and switch to thread A again. -- This
will either switch before `ret = ajoy_takesem(>au_exclsem);` or inside
the `ret = ajoy_takesem(>au_exclsem);` call.
I mean before take au_exclsem too.
> 3. Thread A continue ajoy_close and release opriv -- Yes, so in this case
we need to access `filep->f_priv` (`opriv`) after `ret =
ajoy_takesem(>au_exclsem);` is successful and `filep->f_priv` should be
set to `NULL` after memory is freed.
Yes.
> 4. Thread B resume and touch the freed memory -- this will not happen if
`ajoy_takesem(>au_exclsem);` is successfully taken and `filep->f_priv` is
accessed after that.
>
Once ajoy_close return at the step 3, filep may be reused by VFS layer and
then f_priv may point to a different struct, which make the check of
filep->f_priv useless.
> My assumption is next: The `priv->au_exclsem` protects any modification of
`node->i_private` data and `filep->f_priv` data. If there is no modification,
then we do not need to take `priv->au_exclsem`. We can try to leverage critical
section approach. In other words the `ajoy_read` should look like:
>
> ```
> /* Get exclusive access to the driver structure */
>
> if (ret >= 0)
> {
> flags = enter_critical_section();
>
> DEBUGASSERT(filep && filep->f_inode);
> opriv = filep->f_priv;
>
> if (opriv != NULL)
> {
> opriv->ao_pollpending = false;
> ret = sizeof(struct ajoy_sample_s);
> }
> else
> {
> ret = -EPIPE;
> }
>
> leave_critical_section(flags);
> }
>
> return (ssize_t)ret;
> }
> ```
neither semaphore nor critical section can resolve the above sequence.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821674599
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> So what is not handled in this case?
>
> 1. Thread A just enter ajoy_close and switch to thread B. -- I assume that
this is done either before `ajoy_takesem(>au_exclsem);` or inside the
`ajoy_takesem(>au_exclsem);` call.
I mean before take au_exclsem.
> 2. Thread B just enter ajoy_read and switch to thread A again. -- This
will either switch before `ret = ajoy_takesem(>au_exclsem);` or inside
the `ret = ajoy_takesem(>au_exclsem);` call.
I mean before take au_exclsem too.
> 3. Thread A continue ajoy_close and release opriv -- Yes, so in this case
we need to access `filep->f_priv` (`opriv`) after `ret =
ajoy_takesem(>au_exclsem);` is successful and `filep->f_priv` should be
set to `NULL` after memory is freed.
Yes.
> 4. Thread B resume and touch the freed memory -- this will not happen if
`ajoy_takesem(>au_exclsem);` is successfully taken and `filep->f_priv` is
accessed after that.
>
Once ajoy_close return at the step 3, filep may be reused by VFS layer and
then f_priv may point to a different struct, which make the check of
filep->f_priv useless.
> My assumption is next: The `priv->au_exclsem` protects any modification of
`node->i_private` data and `filep->f_priv` data. If there is no modification,
then we do not need to take `priv->au_exclsem`. We can try to leverage critical
section approach. In other words the `ajoy_read` should look like:
>
> ```
> /* Get exclusive access to the driver structure */
>
> if (ret >= 0)
> {
> flags = enter_critical_section();
>
> DEBUGASSERT(filep && filep->f_inode);
> opriv = filep->f_priv;
>
> if (opriv != NULL)
> {
> opriv->ao_pollpending = false;
> ret = sizeof(struct ajoy_sample_s);
> }
> else
> {
> ret = -EPIPE;
> }
>
> leave_critical_section(flags);
> }
>
> return (ssize_t)ret;
> }
> ```
neither semaphore nor critical section can resolve the above sequency.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821634793
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
You can't fix the race condition in all case. How to handle this
sequence?
1. Thread A just enter ajoy_close and switch to thread B
2. Thread B just enter ajoy_read and switch to thread A again
3. Thread A continue ajoy_close and release opriv
4. Thread B resume and touch the freed memory
Basically, the resource release is very very special, you can't protect the
resource by the normal approach. That's why I remove the related code from
these patches, because it can't work, and just make the confusion.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821634793
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
You can't fix the race condition in all case. How to handle this
sequence?
1. Thread A just enter ajoy_close and switch to thread B
2. Thread B just enter ajoy_read and switch to thread A again
3. Thread A continue ajoy_close and release opriv
4. Thread B resume and touch the freed memory
Basically, the resource release is very very special, you can't protect the
resource by the normal approach.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821592496
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
In this case, opriv is already freed by ajoy_close, how to detect it
with the freed memory? All file handles(file system, char driver, block driver,
socket) has the same issue, as I said before it need handle in the general way.
Before that, it's user's responsibility not to call close if any FS API is
on going.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r821592496
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
In this case, opriv is already freed by ajoy_close, how to detect it
with the freed memory? All file handles(file system, char driver, block driver,
socket) has the same issue, as I said before it need handle in the general way.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820249165
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
Ok, I move enter_critical_section after call low->al_sample.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820243922
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
Oh, yes, but ajoy_sample call lower->al_buttons:
https://github.com/apache/incubator-nuttx/pull/5204/files#diff-5803ad0a8a93175597d14948ffe9a46f4e1ad28b59d28bcb568f56553b1fa8f7R241
I would expect that al_sample has the similar implementation requirement as
al_buttons. Do you think the assumption reasonable?
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820208640
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
ajoy_interrupt will be called inside interrupt handler which call
lower->al_sample through ajoy_sample indirectly. If ajoy_interrupt is called
only in thread context. All protection can be switch to the semaphore instead
in first place.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820208640
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
ajoy_interrupt will be called inside interrupt handler which call
lower->al_sample through ajoy_sample indirectly. If ajoy_interrupt is called
only in thread context. All protection can be switch to the semaphore instead.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r820062588
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
al_sample is impossible to pend since it may be called inside interrupt
handler. This is the root reason why we must protect it with critical section,
not semaphore.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r819810584
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> Probably you are right, but the usage of semaphore gives us exclusive
access with waiting for a task context while critical section resolves mutual
exclusion between task and interrupt context inside the driver. I mean that
only one of multiple callers of `ajoy_read` will call
`lower->al_sample()`+return from `al_sample` with semaphore and multiple
callers will call `lower->al_sample()` in case if critical section is used and
`al_sample()` call a pending primitive. That is conceptually difference that
I'm trying to clarify the behavior.
You mean al_sample may switch out inside critical section? Yes, if it wake
up a high priority thread.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815467176
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
The link list is just a demo, if you evaluate each field on by one, you
will find that all most fields touch in both the thread context and interrupt
context.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could
still keep a semaphore for `priv` protection and critical section to protect
`opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to
fix is that the original code protect the same resource some time with
semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access
ao_flink
so, the final result is that there is no protection at all. You can't
protect the same resource by two semaphores or mix of semaphore and critical
section. Since we need allow ajoy_interrupt is called from the interrupt
context directly, the critical section is the only choice here, and then
semaphore must be removed to avoid the potential misuse like the above in the
future. Actually, I always prefer to use semaphore to protect resource, but has
to keep the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is
pending on `file_read` call? I briefly looked thru the code and see that seems
like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while
other file api is executing. This problem isn't specific to joystick/button
drivers, it's a common issue to the whole file system, and can't handle inside
the driver level. The old code try to fix this problem with ao_closing, but
it's impossible to work after the simple analysis.
> Please add more reviewers to this change before merging 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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could
still keep a semaphore for `priv` protection and critical section to protect
`opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to
fix is that the original code protect the same resource some time with
semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access
ao_flink
so, the final result is that there is no protection at all. You can't
protect the same resource by two semaphores or mix of semaphore and critical
section. Since we need allow ajoy_interrupt is called from the interrupt
context directly, the critical section is the only choice here, and then
semaphore must be removed to avoid the potential misuse like the above in the
future. Actually, I always prefer to use semaphore to protect resource, but has
to keep the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is
pending on `file_read` call? I briefly looked thru the code and see that seems
like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while
other file api is executing. This problem isn't specific to joystick/button
drivers, it's a common issue to the whole file system, and can't handle inside
the driver level. The old code try to fix this problem with ao_closing, but it
impossible to work.
> Please add more reviewers to this change before merging 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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could
still keep a semaphore for `priv` protection and critical section to protect
`opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to
fix is that the original code protect the same resource some time with
semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access
ao_flink
so, the final result is that there is no protection at all. You can't
protect the same resource by two semaphores or mi of semaphore and critical
section. Since we need allow ajoy_interrupt is called from the interrupt
context directly, the critical section is the only choice, and then semaphore
must be removed to avoid the potential misuse like the above in the future.
Actually, I always prefer to use semaphore to protect resource, but has to keep
the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is
pending on `file_read` call? I briefly looked thru the code and see that seems
like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while
other file api is executing. This problem isn't specific to joystick/button
drivers, it's a common issue to the whole file system, and can't handle inside
the driver level. The old code try to fix this problem with ao_closing, but it
impossible to work.
> Please add more reviewers to this change before merging it.
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could
still keep a semaphore for `priv` protection and critical section to protect
`opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to
fix is that the original code protect the same resource some time with
semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access
ao_flink
so, the final result is that there is no protection at all. You can't
protect the same resource by two semaphores or mix of semaphore and critical
section. Since we need allow ajoy_interrupt is called from the interrupt
context directly, the critical section is the only choice, and then semaphore
must be removed to avoid the potential misuse like the above in the future.
Actually, I always prefer to use semaphore to protect resource, but has to keep
the critical section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is
pending on `file_read` call? I briefly looked thru the code and see that seems
like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while
other file api is executing. This problem isn't specific to joystick/button
drivers, it's a common issue to the whole file system, and can't handle inside
the driver level. The old code try to fix this problem with ao_closing, but it
impossible to work.
> Please add more reviewers to this change before merging 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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815464628
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
> Yes. The real use case would require reference counter. Or we could
still keep a semaphore for `priv` protection and critical section to protect
`opriv` manipulations.
So, which resource is protected by semaphore? The problem this PR want to
fix is that the original code protect the same resource some time with
semaphore, other time with the critical section, for example:
1. ajoy_open/ajoy_close hold au_exclsem before modifying ao_flink
2. ajoy_interrupt(ajoy_sample) hold the critical section before access
ao_flink
so, the final result is that no protection at all. You can't protect the
same resource by two semaphores or mi of semaphore and critical section. Since
we need allow ajoy_interrupt is called from the interrupt context directly, the
critical section is the only choice, and then semaphore must be removed to
avoid the potential misuse like the above in the future. Actually, I always
prefer to use semaphore to protect resource, but has to keep the critical
section here due to the requirement of ajoy_interrupt.
> I'm just thinking what if `ajoy_interrupt` fires while `al_sample` is
pending on `file_read` call? I briefly looked thru the code and see that seems
like nothing harmful, but I still have some doubts.
>
It's fine in this case. What can't handle is that someone call close while
other file api is executing. This problem isn't specific to joystick/button
drivers, it's a common issue to the whole file system, and can't handle inside
the driver level. The old code try to fix this problem with ao_closing, but it
impossible to work.
> Please add more reviewers to this change before merging 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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815457612
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
vfs layer can't handle this case correctly in the complex case. To avoid
the used after free, the better solution is:
1. Add a reference count to struct file
2. Initialize the reference count to 1 in file_open
3. Decrease the reference count in file_close
4. Increase count before call file_xxx and decrease after
5. Release the resource only when the reference count become zero
This way, the real release happen(delayed) after file_read return when
file_close is called during file_read is executing.
The similar approach is used in Linux. Before this infrastructure get
implemented, I would suggest that the caller should never call close if any
other file related api is executing.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815457612
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
vfs layer can't handle this case correctly in the complex case. To avoid
the used after free, the better solution is:
1. Add a reference count to struct file
2. Initialize the reference count to 1 in file_open
3. Decrease the reference count in file_close
4. Increase count before call file_xxx and decrease after
5. Release the resource only when the reference count become zero
This way, the real release happen(delayed) after file_read return when
file_close is called during file_read is executing.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5204: joystck/buttons: Always protect the open list by critical section
xiaoxiang781216 commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r815455016
##
File path: drivers/input/ajoystick.c
##
@@ -526,12 +461,7 @@ static ssize_t ajoy_read(FAR struct file *filep, FAR char
*buffer,
/* Get exclusive access to the driver structure */
- ret = ajoy_takesem(>au_exclsem);
- if (ret < 0)
-{
- ierr("ERROR: ajoy_takesem failed: %d\n", ret);
- return ret;
-}
+ flags = enter_critical_section();
Review comment:
It's fine to call file_read inside critical section, since the
implementation of critical section in nuttx allow the thread enter the wait
state while the critical section is hold.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
