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 <xiaoxiang@xiaomi.com>
This commit is contained in:
Xiang Xiao
Petro Karashchenko
parent
a924d7a17e
commit
f884e647ec
@ -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);
|
||||
@ -155,15 +145,6 @@ static const struct file_operations djoy_fops =
|
||||
* Private Functions
|
||||
****************************************************************************/
|
||||
|
||||
/****************************************************************************
|
||||
* 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);
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user