[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5204: input/ajoystck: Always protect the au_open list by critical section

[GitBox <gi...@apache.org>]

pkarashchenko commented on a change in pull request #5204:
URL: https://github.com/apache/incubator-nuttx/pull/5204#discussion_r782415335



##########
File path: drivers/input/ajoystick.c
##########
@@ -331,10 +327,6 @@ static void ajoy_sample(FAR struct ajoy_upperhalf_s *priv)
         }
     }
 
-  /* Enable/disable interrupt handling */
-
-  ajoy_enable(priv);

Review comment:
       Why `ajoy_enable` is not needed here?

##########
File path: drivers/input/ajoystick.c
##########
@@ -412,45 +407,14 @@ static int ajoy_close(FAR struct file *filep)
   FAR struct ajoy_open_s *curr;
   FAR struct ajoy_open_s *prev;
   irqstate_t flags;
-  bool closing;
-  int ret;
 
   DEBUGASSERT(filep && filep->f_priv && filep->f_inode);
   opriv = filep->f_priv;
   inode = filep->f_inode;
   DEBUGASSERT(inode->i_private);
   priv  = (FAR struct ajoy_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->ao_closing;
-  opriv->ao_closing = true;
-  leave_critical_section(flags);
-
-  if (closing)
-    {
-      /* Another thread is doing the close */
-
-      return OK;
-    }
-
-  /* Get exclusive access to the driver structure */

Review comment:
       I'm not sure in this fix. I would expect still to wait for `au_exclsem` in `close`. I'm expecting flow like:
   ```
     priv = (FAR struct ajoy_upperhalf_s *)inode->i_private;
   
     ret = ajoy_takesem(&priv->au_exclsem);
     if (ret < 0)
       {
         ierr("ERROR: ajoy_takesem failed: %d\n", ret);
         return ret;
       }
   
     flags = enter_critical_section();
   
     /* Find the open structure in the list of open structures for the device */
   
     for (prev = NULL, curr = priv->au_open;
          curr && curr != opriv;
          prev = curr, curr = curr->ao_flink);
   
     DEBUGASSERT(curr);
     if (curr == NULL)
       {
         leave_critical_section(flags);
         ierr("ERROR: Failed to find open entry\n");
         ret = -ENOENT;
         goto errout_with_exclsem;
       }
   
     /* Remove the structure from the device */
   
     if (prev)
       {
         prev->ao_flink = opriv->ao_flink;
       }
     else
       {
         priv->au_open = opriv->ao_flink;
       }
   
     leave_critical_section(flags);
   
     /* Cancel any pending notification */
   
     nxsig_cancel_notification(&opriv->ao_work);
   
     /* And free the open structure */
   
     kmm_free(opriv);
   
     /* Enable/disable interrupt handling */
   
     ajoy_enable(priv);
     return OK;
   
   errout_with_exclsem:
     ajoy_givesem(&priv->au_exclsem);
     return ret;
   ```




-- 
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-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org