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 <xiaoxiang@xiaomi.com>

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;
+      return -ENOMEM;
-      goto errout_with_sem;
     }
 
   /* 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:
+  leave_critical_section(flags);
-  btn_givesem(&priv->bu_exclsem);
+  return OK;
-  return ret;
 }
 
 /****************************************************************************
@@ -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);
+  flags = enter_critical_section();
-  if (ret < 0)
-    {
-      ierr("ERROR: btn_takesem failed: %d\n", ret);
-      return ret;
-    }
 
   /* 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;
+      leave_critical_section(flags);
-      goto errout_with_exclsem;
+      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);
-
+  return OK;
-  /* Enable/disable interrupt handling */
-
-  btn_enable(priv);
-  ret = OK;
-
-errout_with_exclsem:
-  btn_givesem(&priv->bu_exclsem);
-  return ret;
 }
 
 /****************************************************************************
@@ -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);
+  flags = enter_critical_section();
-  if (ret < 0)
-    {
-      ierr("ERROR: btn_takesem failed: %d\n", ret);
-      return ret;
-    }
 
   /* 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);
+  flags = enter_critical_section();
-  if (ret < 0)
-    {
-      ierr("ERROR: btn_takesem failed: %d\n", ret);
-      return ret;
-    }
 
   /* 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);
+  flags = enter_critical_section();
-  if (ret < 0)
-    {
-      ierr("ERROR: btn_takesem failed: %d\n", ret);
-      return ret;
-    }
 
   /* 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);
+  flags = enter_critical_section();
-  if (ret < 0)
-    {
-      ierr("ERROR: btn_takesem failed: %d\n", ret);
-      return ret;
-    }
 
   /* 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;
 }