You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by pk...@apache.org on 2022/03/09 08:10:51 UTC
[incubator-nuttx] 02/03: input/djoystck: Always protect the resource by critical section
This is an automated email from the ASF dual-hosted git repository.
pkarashchenko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit f884e647ec78930042f9d4d7df27ee1066078a9e
Author: Xiang Xiao <xi...@xiaomi.com>
AuthorDate: Sun Feb 27 04:36:30 2022 +0800
input/djoystck: Always protect the resource by critical section
it's wrong to protect the resource by du_exclsem
in some case and critical section in others
Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
---
drivers/input/djoystick.c | 146 +++++++++-------------------------------------
1 file changed, 29 insertions(+), 117 deletions(-)
diff --git a/drivers/input/djoystick.c b/drivers/input/djoystick.c
index 5e614d6..ce114fe 100644
--- a/drivers/input/djoystick.c
+++ b/drivers/input/djoystick.c
@@ -64,7 +64,6 @@ struct djoy_upperhalf_s
FAR const struct djoy_lowerhalf_s *du_lower;
djoy_buttonset_t du_sample; /* Last sampled button states */
- sem_t du_exclsem; /* Supports exclusive access to the device */
/* The following is a singly linked list of open references to the
* joystick device.
@@ -81,10 +80,6 @@ struct djoy_open_s
FAR struct djoy_open_s *do_flink;
- /* The following will be true if we are closing */
-
- volatile bool do_closing;
-
/* Joystick event notification information */
pid_t do_pid;
@@ -107,11 +102,6 @@ struct djoy_open_s
* Private Function Prototypes
****************************************************************************/
-/* Semaphore helpers */
-
-static inline int djoy_takesem(sem_t *sem);
-#define djoy_givesem(s) nxsem_post(s);
-
/* Sampling and Interrupt handling */
static void djoy_enable(FAR struct djoy_upperhalf_s *priv);
@@ -156,15 +146,6 @@ static const struct file_operations djoy_fops =
****************************************************************************/
/****************************************************************************
- * Name: djoy_takesem
- ****************************************************************************/
-
-static inline int djoy_takesem(sem_t *sem)
-{
- return nxsem_wait(sem);
-}
-
-/****************************************************************************
* Name: djoy_enable
****************************************************************************/
@@ -174,18 +155,11 @@ static void djoy_enable(FAR struct djoy_upperhalf_s *priv)
FAR struct djoy_open_s *opriv;
djoy_buttonset_t press;
djoy_buttonset_t release;
- irqstate_t flags;
DEBUGASSERT(priv);
lower = priv->du_lower;
DEBUGASSERT(lower);
- /* This routine is called both task level and interrupt level, so
- * interrupts must be disabled.
- */
-
- flags = enter_critical_section();
-
/* Visit each opened reference to the device */
press = 0;
@@ -218,8 +192,6 @@ static void djoy_enable(FAR struct djoy_upperhalf_s *priv)
lower->dl_enable(lower, 0, 0, NULL, NULL);
}
-
- leave_critical_section(flags);
}
/****************************************************************************
@@ -336,38 +308,30 @@ static int djoy_open(FAR struct file *filep)
FAR struct djoy_open_s *opriv;
FAR const struct djoy_lowerhalf_s *lower;
djoy_buttonset_t supported;
- int ret;
+ irqstate_t flags;
DEBUGASSERT(filep && filep->f_inode);
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
priv = (FAR struct djoy_upperhalf_s *)inode->i_private;
- /* Get exclusive access to the driver structure */
-
- ret = djoy_takesem(&priv->du_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: djoy_takesem failed: %d\n", ret);
- return ret;
- }
-
/* Allocate a new open structure */
opriv = (FAR struct djoy_open_s *)kmm_zalloc(sizeof(struct djoy_open_s));
if (!opriv)
{
ierr("ERROR: Failed to allocate open structure\n");
- ret = -ENOMEM;
- goto errout_with_sem;
+ return -ENOMEM;
}
/* Initialize the open structure */
lower = priv->du_lower;
DEBUGASSERT(lower && lower->dl_supported);
- supported = lower->dl_supported(lower);
+ flags = enter_critical_section();
+
+ supported = lower->dl_supported(lower);
opriv->do_pollevents.dp_press = supported;
opriv->do_pollevents.dp_release = supported;
@@ -383,11 +347,9 @@ static int djoy_open(FAR struct file *filep)
/* Attach the open structure to the file structure */
filep->f_priv = (FAR void *)opriv;
- ret = OK;
-errout_with_sem:
- djoy_givesem(&priv->du_exclsem);
- return ret;
+ leave_critical_section(flags);
+ return OK;
}
/****************************************************************************
@@ -402,8 +364,6 @@ static int djoy_close(FAR struct file *filep)
FAR struct djoy_open_s *curr;
FAR struct djoy_open_s *prev;
irqstate_t flags;
- bool closing;
- int ret;
DEBUGASSERT(filep && filep->f_priv && filep->f_inode);
opriv = filep->f_priv;
@@ -411,36 +371,7 @@ static int djoy_close(FAR struct file *filep)
DEBUGASSERT(inode->i_private);
priv = (FAR struct djoy_upperhalf_s *)inode->i_private;
- /* Handle an improbable race conditions with the following atomic test
- * and set.
- *
- * This is actually a pretty feeble attempt to handle this. The
- * improbable race condition occurs if two different threads try to
- * close the joystick driver at the same time. The rule: don't do
- * that! It is feeble because we do not really enforce stale pointer
- * detection anyway.
- */
-
flags = enter_critical_section();
- closing = opriv->do_closing;
- opriv->do_closing = true;
- leave_critical_section(flags);
-
- if (closing)
- {
- /* Another thread is doing the close */
-
- return OK;
- }
-
- /* Get exclusive access to the driver structure */
-
- ret = djoy_takesem(&priv->du_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: djoy_takesem failed: %d\n", ret);
- return ret;
- }
/* Find the open structure in the list of open structures for the device */
@@ -452,8 +383,8 @@ static int djoy_close(FAR struct file *filep)
if (!curr)
{
ierr("ERROR: Failed to find open entry\n");
- ret = -ENOENT;
- goto errout_with_exclsem;
+ leave_critical_section(flags);
+ return -ENOENT;
}
/* Remove the structure from the device */
@@ -467,6 +398,12 @@ static int djoy_close(FAR struct file *filep)
priv->du_open = opriv->do_flink;
}
+ /* Enable/disable interrupt handling */
+
+ djoy_enable(priv);
+
+ leave_critical_section(flags);
+
/* Cancel any pending notification */
nxsig_cancel_notification(&opriv->do_work);
@@ -474,15 +411,7 @@ static int djoy_close(FAR struct file *filep)
/* And free the open structure */
kmm_free(opriv);
-
- /* Enable/disable interrupt handling */
-
- djoy_enable(priv);
- ret = OK;
-
-errout_with_exclsem:
- djoy_givesem(&priv->du_exclsem);
- return ret;
+ return OK;
}
/****************************************************************************
@@ -496,6 +425,7 @@ static ssize_t djoy_read(FAR struct file *filep, FAR char *buffer,
FAR struct djoy_open_s *opriv;
FAR struct djoy_upperhalf_s *priv;
FAR const struct djoy_lowerhalf_s *lower;
+ irqstate_t flags;
int ret;
DEBUGASSERT(filep && filep->f_inode);
@@ -516,12 +446,7 @@ static ssize_t djoy_read(FAR struct file *filep, FAR char *buffer,
/* Get exclusive access to the driver structure */
- ret = djoy_takesem(&priv->du_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: djoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
/* Read and return the current state of the joystick buttons */
@@ -532,7 +457,7 @@ static ssize_t djoy_read(FAR struct file *filep, FAR char *buffer,
opriv->do_pollpending = false;
ret = sizeof(djoy_buttonset_t);
- djoy_givesem(&priv->du_exclsem);
+ leave_critical_section(flags);
return (ssize_t)ret;
}
@@ -546,6 +471,7 @@ static int djoy_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
FAR struct djoy_upperhalf_s *priv;
FAR struct djoy_open_s *opriv;
FAR const struct djoy_lowerhalf_s *lower;
+ irqstate_t flags;
int ret;
DEBUGASSERT(filep && filep->f_priv && filep->f_inode);
@@ -556,12 +482,7 @@ static int djoy_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
/* Get exclusive access to the driver structure */
- ret = djoy_takesem(&priv->du_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: djoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
/* Handle the ioctl command */
@@ -661,7 +582,7 @@ static int djoy_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
break;
}
- djoy_givesem(&priv->du_exclsem);
+ leave_critical_section(flags);
return ret;
}
@@ -673,25 +594,19 @@ static int djoy_poll(FAR struct file *filep, FAR struct pollfd *fds,
bool setup)
{
FAR struct inode *inode;
- FAR struct djoy_upperhalf_s *priv;
FAR struct djoy_open_s *opriv;
- int ret;
+ irqstate_t flags;
+ int ret = OK;
int i;
DEBUGASSERT(filep && filep->f_priv && filep->f_inode);
opriv = filep->f_priv;
inode = filep->f_inode;
DEBUGASSERT(inode->i_private);
- priv = (FAR struct djoy_upperhalf_s *)inode->i_private;
/* Get exclusive access to the driver structure */
- ret = djoy_takesem(&priv->du_exclsem);
- if (ret < 0)
- {
- ierr("ERROR: djoy_takesem failed: %d\n", ret);
- return ret;
- }
+ flags = enter_critical_section();
/* Are we setting up the poll? Or tearing it down? */
@@ -731,7 +646,7 @@ static int djoy_poll(FAR struct file *filep, FAR struct pollfd *fds,
ierr("ERROR: Too man poll waiters\n");
fds->priv = NULL;
ret = -EBUSY;
- goto errout_with_dusem;
+ goto errout;
}
}
else if (fds->priv)
@@ -745,7 +660,7 @@ static int djoy_poll(FAR struct file *filep, FAR struct pollfd *fds,
{
ierr("ERROR: Poll slot not found\n");
ret = -EIO;
- goto errout_with_dusem;
+ goto errout;
}
#endif
@@ -755,8 +670,8 @@ static int djoy_poll(FAR struct file *filep, FAR struct pollfd *fds,
fds->priv = NULL;
}
-errout_with_dusem:
- djoy_givesem(&priv->du_exclsem);
+errout:
+ leave_critical_section(flags);
return ret;
}
@@ -797,7 +712,6 @@ int djoy_register(FAR const char *devname,
priv = (FAR struct djoy_upperhalf_s *)
kmm_zalloc(sizeof(struct djoy_upperhalf_s));
-
if (!priv)
{
ierr("ERROR: Failed to allocate device structure\n");
@@ -812,7 +726,6 @@ int djoy_register(FAR const char *devname,
/* Initialize the new djoystick driver instance */
priv->du_lower = lower;
- nxsem_init(&priv->du_exclsem, 0, 1);
DEBUGASSERT(lower->dl_sample);
priv->du_sample = lower->dl_sample(lower);
@@ -823,7 +736,6 @@ int djoy_register(FAR const char *devname,
if (ret < 0)
{
ierr("ERROR: register_driver failed: %d\n", ret);
- nxsem_destroy(&priv->du_exclsem);
kmm_free(priv);
}