From b690262aac1b459d3e380531ff11c0c3432e862a Mon Sep 17 00:00:00 2001 From: Xiang Xiao Date: Sun, 27 Feb 2022 15:52:59 +0800 Subject: [PATCH] input/buttons: Always protect the resource by critical section it's wrong to protect the resource by bu_exclsem in some case and critical section in others Signed-off-by: Xiang Xiao --- drivers/input/button_upper.c | 163 ++++++++--------------------------- 1 file changed, 36 insertions(+), 127 deletions(-) diff --git a/drivers/input/button_upper.c b/drivers/input/button_upper.c index 3d84c0abd1..6fefdb306b 100644 --- a/drivers/input/button_upper.c +++ b/drivers/input/button_upper.c @@ -60,7 +60,6 @@ struct btn_upperhalf_s FAR const struct btn_lowerhalf_s *bu_lower; btn_buttonset_t bu_sample; /* Last sampled button states */ - sem_t bu_exclsem; /* Supports exclusive access to the device */ /* The following is a singly linked list of open references to the * button device. @@ -77,10 +76,6 @@ struct btn_open_s FAR struct btn_open_s *bo_flink; - /* The following will be true if we are closing */ - - volatile bool bo_closing; - /* Button event notification information */ pid_t bo_pid; @@ -103,11 +98,6 @@ struct btn_open_s * Private Function Prototypes ****************************************************************************/ -/* Semaphore helpers */ - -static inline int btn_takesem(sem_t *sem); -#define btn_givesem(s) nxsem_post(s); - /* Sampling and Interrupt handling */ static void btn_enable(FAR struct btn_upperhalf_s *priv); @@ -153,15 +143,6 @@ static const struct file_operations btn_fops = * Private Functions ****************************************************************************/ -/**************************************************************************** - * Name: btn_takesem - ****************************************************************************/ - -static inline int btn_takesem(sem_t *sem) -{ - return nxsem_wait(sem); -} - /**************************************************************************** * Name: btn_enable ****************************************************************************/ @@ -249,18 +230,11 @@ static void btn_sample(FAR struct btn_upperhalf_s *priv) btn_buttonset_t change; btn_buttonset_t press; btn_buttonset_t release; - irqstate_t flags; int i; DEBUGASSERT(priv && priv->bu_lower); lower = priv->bu_lower; - /* This routine is called both task level and interrupt level, so - * interrupts must be disabled. - */ - - flags = enter_critical_section(); - /* Sample the new button state */ DEBUGASSERT(lower->bl_buttons); @@ -322,7 +296,6 @@ static void btn_sample(FAR struct btn_upperhalf_s *priv) } priv->bu_sample = sample; - leave_critical_section(flags); } /**************************************************************************** @@ -336,38 +309,30 @@ static int btn_open(FAR struct file *filep) FAR struct btn_open_s *opriv; FAR const struct btn_lowerhalf_s *lower; btn_buttonset_t supported; - int ret; + irqstate_t flags; DEBUGASSERT(filep && filep->f_inode); inode = filep->f_inode; DEBUGASSERT(inode->i_private); priv = (FAR struct btn_upperhalf_s *)inode->i_private; - /* Get exclusive access to the driver structure */ - - ret = btn_takesem(&priv->bu_exclsem); - if (ret < 0) - { - ierr("ERROR: btn_takesem failed: %d\n", ret); - return ret; - } - /* Allocate a new open structure */ opriv = (FAR struct btn_open_s *)kmm_zalloc(sizeof(struct btn_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->bu_lower; DEBUGASSERT(lower && lower->bl_supported); - supported = lower->bl_supported(lower); + flags = enter_critical_section(); + + supported = lower->bl_supported(lower); opriv->bo_pollevents.bp_press = supported; opriv->bo_pollevents.bp_release = supported; @@ -379,15 +344,13 @@ static int btn_open(FAR struct file *filep) /* Attach the open structure to the file structure */ filep->f_priv = (FAR void *)opriv; - ret = OK; /* Enable/disable interrupt handling */ btn_enable(priv); -errout_with_sem: - btn_givesem(&priv->bu_exclsem); - return ret; + leave_critical_section(flags); + return OK; } /**************************************************************************** @@ -402,8 +365,6 @@ static int btn_close(FAR struct file *filep) FAR struct btn_open_s *curr; FAR struct btn_open_s *prev; irqstate_t flags; - bool closing; - int ret; DEBUGASSERT(filep && filep->f_priv && filep->f_inode); opriv = filep->f_priv; @@ -411,36 +372,9 @@ static int btn_close(FAR struct file *filep) DEBUGASSERT(inode->i_private); priv = (FAR struct btn_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 button 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->bo_closing; - opriv->bo_closing = true; - leave_critical_section(flags); - - if (closing) - { - /* Another thread is doing the close */ - - return OK; - } - /* Get exclusive access to the driver structure */ - ret = btn_takesem(&priv->bu_exclsem); - if (ret < 0) - { - ierr("ERROR: btn_takesem failed: %d\n", ret); - return ret; - } + flags = enter_critical_section(); /* Find the open structure in the list of open structures for the device */ @@ -452,8 +386,8 @@ static int btn_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 +401,12 @@ static int btn_close(FAR struct file *filep) priv->bu_open = opriv->bo_flink; } + /* Enable/disable interrupt handling */ + + btn_enable(priv); + + leave_critical_section(flags); + /* Cancel any pending notification */ nxsig_cancel_notification(&opriv->bo_work); @@ -474,15 +414,7 @@ static int btn_close(FAR struct file *filep) /* And free the open structure */ kmm_free(opriv); - - /* Enable/disable interrupt handling */ - - btn_enable(priv); - ret = OK; - -errout_with_exclsem: - btn_givesem(&priv->bu_exclsem); - return ret; + return OK; } /**************************************************************************** @@ -496,7 +428,7 @@ static ssize_t btn_read(FAR struct file *filep, FAR char *buffer, FAR struct btn_open_s *opriv; FAR struct btn_upperhalf_s *priv; FAR const struct btn_lowerhalf_s *lower; - int ret; + irqstate_t flags; DEBUGASSERT(filep && filep->f_inode); opriv = filep->f_priv; @@ -518,12 +450,7 @@ static ssize_t btn_read(FAR struct file *filep, FAR char *buffer, /* Get exclusive access to the driver structure */ - ret = btn_takesem(&priv->bu_exclsem); - if (ret < 0) - { - ierr("ERROR: btn_takesem failed: %d\n", ret); - return ret; - } + flags = enter_critical_section(); /* Read and return the current state of the buttons */ @@ -532,7 +459,7 @@ static ssize_t btn_read(FAR struct file *filep, FAR char *buffer, *(FAR btn_buttonset_t *)buffer = lower->bl_buttons(lower); opriv->bo_pending = false; - btn_givesem(&priv->bu_exclsem); + leave_critical_section(flags); return (ssize_t)sizeof(btn_buttonset_t); } @@ -546,6 +473,7 @@ static ssize_t btn_write(FAR struct file *filep, FAR const char *buffer, FAR struct inode *inode; FAR struct btn_upperhalf_s *priv; FAR const struct btn_lowerhalf_s *lower; + irqstate_t flags; int ret; DEBUGASSERT(filep && filep->f_inode); @@ -567,12 +495,7 @@ static ssize_t btn_write(FAR struct file *filep, FAR const char *buffer, /* Get exclusive access to the driver structure */ - ret = btn_takesem(&priv->bu_exclsem); - if (ret < 0) - { - ierr("ERROR: btn_takesem failed: %d\n", ret); - return ret; - } + flags = enter_critical_section(); /* Write the current state of the buttons */ @@ -587,7 +510,7 @@ static ssize_t btn_write(FAR struct file *filep, FAR const char *buffer, ret = -ENOSYS; } - btn_givesem(&priv->bu_exclsem); + leave_critical_section(flags); return (ssize_t)ret; } @@ -601,7 +524,8 @@ static int btn_ioctl(FAR struct file *filep, int cmd, unsigned long arg) FAR struct btn_upperhalf_s *priv; FAR struct btn_open_s *opriv; FAR const struct btn_lowerhalf_s *lower; - int ret; + irqstate_t flags; + int ret = OK; DEBUGASSERT(filep && filep->f_priv && filep->f_inode); opriv = filep->f_priv; @@ -611,12 +535,7 @@ static int btn_ioctl(FAR struct file *filep, int cmd, unsigned long arg) /* Get exclusive access to the driver structure */ - ret = btn_takesem(&priv->bu_exclsem); - if (ret < 0) - { - ierr("ERROR: btn_takesem failed: %d\n", ret); - return ret; - } + flags = enter_critical_section(); /* Handle the ioctl command */ @@ -717,7 +636,7 @@ static int btn_ioctl(FAR struct file *filep, int cmd, unsigned long arg) break; } - btn_givesem(&priv->bu_exclsem); + leave_critical_section(flags); return ret; } @@ -731,7 +650,8 @@ static int btn_poll(FAR struct file *filep, FAR struct pollfd *fds, FAR struct inode *inode; FAR struct btn_upperhalf_s *priv; FAR struct btn_open_s *opriv; - int ret; + irqstate_t flags; + int ret = OK; int i; DEBUGASSERT(filep && filep->f_priv && filep->f_inode); @@ -742,12 +662,7 @@ static int btn_poll(FAR struct file *filep, FAR struct pollfd *fds, /* Get exclusive access to the driver structure */ - ret = btn_takesem(&priv->bu_exclsem); - if (ret < 0) - { - ierr("ERROR: btn_takesem failed: %d\n", ret); - return ret; - } + flags = enter_critical_section(); /* Are we setting up the poll? Or tearing it down? */ @@ -789,7 +704,7 @@ static int btn_poll(FAR struct file *filep, FAR struct pollfd *fds, ierr("ERROR: Too many poll waiters\n"); fds->priv = NULL; ret = -EBUSY; - goto errout_with_dusem; + goto errout; } } else if (fds->priv) @@ -803,7 +718,7 @@ static int btn_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 @@ -813,9 +728,10 @@ static int btn_poll(FAR struct file *filep, FAR struct pollfd *fds, fds->priv = NULL; } -errout_with_dusem: btn_enable(priv); - btn_givesem(&priv->bu_exclsem); + +errout: + leave_critical_section(flags); return ret; } @@ -855,7 +771,6 @@ int btn_register(FAR const char *devname, priv = (FAR struct btn_upperhalf_s *) kmm_zalloc(sizeof(struct btn_upperhalf_s)); - if (!priv) { ierr("ERROR: Failed to allocate device structure\n"); @@ -870,7 +785,6 @@ int btn_register(FAR const char *devname, /* Initialize the new button driver instance */ priv->bu_lower = lower; - nxsem_init(&priv->bu_exclsem, 0, 1); DEBUGASSERT(lower->bl_buttons); priv->bu_sample = lower->bl_buttons(lower); @@ -881,13 +795,8 @@ int btn_register(FAR const char *devname, if (ret < 0) { ierr("ERROR: register_driver failed: %d\n", ret); - goto errout_with_priv; + kmm_free(priv); } - return OK; - -errout_with_priv: - nxsem_destroy(&priv->bu_exclsem); - kmm_free(priv); return ret; }