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(&priv->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
