Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2409399216
##
boards/boardctl.c:
##
@@ -133,16 +133,14 @@ static inline int
case BOARDIOC_USBDEV_CONNECT:/* Connect the CDC/ACM device */
#ifndef CONFIG_CDCACM_COMPOSITE
{
-DEBUGASSERT(ctrl->handle != NULL);
ret = cdcacm_initialize(ctrl->instance, ctrl->handle);
}
#endif
break;
case BOARDIOC_USBDEV_DISCONNECT: /* Disconnect the CDC/ACM device
*/
{
-DEBUGASSERT(ctrl->handle != NULL && *ctrl->handle != NULL);
-cdcacm_uninitialize(*ctrl->handle);
+ret = cdcacm_uninitialize_instance(ctrl->instance, NULL);
Review Comment:
For the memory protected builds, the client can't always store the handle.
The point here is that the caller doesn't need to store the handle, since it is
already stored by the kernel in the driver inode. If the client passes NULL
handle here, it means that the kernel looks up the handle from the filesystem
(see "cdcacm_uninitialize_instance" below). This PR essentially adds support
for unregistering the CDCACM by just knowing the instance number.
In the previous block the DEBUGASSERT has always been redundant, NULL
pointer was a valid argument for "cdcacm_initialize" aready before this PR. If
/ when the client doesn't need the handle, it can just pass in NULL pointer,
this is not an error.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
jlaitine commented on PR #17010: URL: https://github.com/apache/nuttx/pull/17010#issuecomment-3375287614 > Please add the test logs to your PR Sure, here is the log (this PR picked on top of the latest release 12.11.0), and using the corresponding apps change in https://github.com/apache/nuttx-apps/pull/3179. This shows how sercon and serdis work 1) in normal case, first connect and then disconnect and 2) in error cases, disconnecting an already disconnected dev, and connecting an already connected dev. [sercon_serdis.log](https://github.com/user-attachments/files/22735538/sercon_serdis.log) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
acassis commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2410261765
##
boards/boardctl.c:
##
@@ -133,16 +133,14 @@ static inline int
case BOARDIOC_USBDEV_CONNECT:/* Connect the CDC/ACM device */
#ifndef CONFIG_CDCACM_COMPOSITE
{
-DEBUGASSERT(ctrl->handle != NULL);
ret = cdcacm_initialize(ctrl->instance, ctrl->handle);
}
#endif
break;
case BOARDIOC_USBDEV_DISCONNECT: /* Disconnect the CDC/ACM device
*/
{
-DEBUGASSERT(ctrl->handle != NULL && *ctrl->handle != NULL);
-cdcacm_uninitialize(*ctrl->handle);
+ret = cdcacm_uninitialize_instance(ctrl->instance, NULL);
Review Comment:
Understood, thank you very much!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
xiaoxiang781216 merged PR #17010: URL: https://github.com/apache/nuttx/pull/17010 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
acassis commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2406711078
##
boards/boardctl.c:
##
@@ -133,16 +133,14 @@ static inline int
case BOARDIOC_USBDEV_CONNECT:/* Connect the CDC/ACM device */
#ifndef CONFIG_CDCACM_COMPOSITE
{
-DEBUGASSERT(ctrl->handle != NULL);
ret = cdcacm_initialize(ctrl->instance, ctrl->handle);
}
#endif
break;
case BOARDIOC_USBDEV_DISCONNECT: /* Disconnect the CDC/ACM device
*/
{
-DEBUGASSERT(ctrl->handle != NULL && *ctrl->handle != NULL);
-cdcacm_uninitialize(*ctrl->handle);
+ret = cdcacm_uninitialize_instance(ctrl->instance, NULL);
Review Comment:
@jlaitine why did you remove the DEBUGASSERT() here and in the previous
block? What is already done in the higher layer? I didn't see you including it,
so I assume it was already there
##
drivers/usbdev/cdcacm.c:
##
@@ -3430,8 +3443,60 @@ void cdcacm_uninitialize(FAR struct usbdevclass_driver_s
*classdev)
{
FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
FAR struct cdcacm_dev_s*priv = drvr->dev;
- char devname[CDCACM_DEVNAME_SIZE];
+
+ cdcacm_uninitialize_instance(priv->minor, classdev);
+}
+
+/
+ * Name: cdcacm_uninitialize_instance
+ *
+ * Description:
+ * Function to uninitialize specific cdcacm instance
+ *
+ * Input Parameters:
+ * minor - CDCACM node minor number
+ *
+ * Returned Value:
+ * OK when successful, -ENODEV if cdcacm is not initialized
+ *
+ /
+
+int cdcacm_uninitialize_instance(int minor,
+ FAR struct usbdevclass_driver_s *classdev)
+{
int ret;
+ FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
+ FAR struct cdcacm_dev_s *priv;
+ char devname[CDCACM_DEVNAME_SIZE];
+
+ /* Create device node path from minor number */
+
+ snprintf(devname, sizeof(devname), CDCACM_DEVNAME_FORMAT, minor);
+
+ ret = nxmutex_lock(&g_init_lock);
+ if (ret < 0)
+{
+ return ret;
+}
+
+ /* If classdev is not provided, find it from the file system */
+
+ if (!classdev)
+{
+ FAR struct cdcacm_alloc_s *cdcacm_alloc = find_driver(devname);
+ if (cdcacm_alloc)
Review Comment:
```suggestion
if (cdcacm_alloc != NULL)
##
drivers/usbdev/cdcacm.c:
##
@@ -3430,8 +3443,60 @@ void cdcacm_uninitialize(FAR struct usbdevclass_driver_s
*classdev)
{
FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
FAR struct cdcacm_dev_s*priv = drvr->dev;
- char devname[CDCACM_DEVNAME_SIZE];
+
+ cdcacm_uninitialize_instance(priv->minor, classdev);
+}
+
+/
+ * Name: cdcacm_uninitialize_instance
+ *
+ * Description:
+ * Function to uninitialize specific cdcacm instance
+ *
+ * Input Parameters:
+ * minor - CDCACM node minor number
+ *
+ * Returned Value:
+ * OK when successful, -ENODEV if cdcacm is not initialized
+ *
+ /
+
+int cdcacm_uninitialize_instance(int minor,
+ FAR struct usbdevclass_driver_s *classdev)
+{
int ret;
+ FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
+ FAR struct cdcacm_dev_s *priv;
+ char devname[CDCACM_DEVNAME_SIZE];
+
+ /* Create device node path from minor number */
+
+ snprintf(devname, sizeof(devname), CDCACM_DEVNAME_FORMAT, minor);
+
+ ret = nxmutex_lock(&g_init_lock);
+ if (ret < 0)
+{
+ return ret;
+}
+
+ /* If classdev is not provided, find it from the file system */
+
+ if (!classdev)
Review Comment:
```suggestion
if (classdev == NULL)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
linguini1 commented on PR #17010: URL: https://github.com/apache/nuttx/pull/17010#issuecomment-3354067399 Please add the test logs to your PR -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
jlaitine commented on code in PR #17010: URL: https://github.com/apache/nuttx/pull/17010#discussion_r2349832488 ## drivers/usbdev/cdcacm.c: ## @@ -313,6 +313,10 @@ static const struct uart_ops_s g_uartops = cdcuart_sendbuf/* sendbuf */ }; +/* Mutex to protect device initialization / uninitialization*/ + +static mutex_t g_init_lock = NXMUTEX_INITIALIZER; Review Comment: Then what do you suggest? Locking driver using FS inode lock? Whether a driver is registered in file system or not is completely irrelevant to the initialization or uninitialization of the driver. These things must not be tightly coupled together. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2349900425
##
drivers/usbdev/cdcacm.c:
##
@@ -3430,8 +3430,53 @@ void cdcacm_uninitialize(FAR struct usbdevclass_driver_s
*classdev)
{
FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
FAR struct cdcacm_dev_s*priv = drvr->dev;
- char devname[CDCACM_DEVNAME_SIZE];
+
+ cdcacm_uninitialize_instance(priv->minor, classdev);
+}
+
+/
+ * Name: cdcacm_uninitialize_instance
+ *
+ * Description:
+ * Function to uninitialize specific cdcacm instance
+ *
+ * Input Parameters:
+ * minor - CDCACM node minor number
+ *
+ * Returned Value:
+ * OK when successful, -ENODEV if cdcacm is not initialized
+ *
+ /
+
+int cdcacm_uninitialize_instance(int minor,
+ FAR struct usbdevclass_driver_s *classdev)
+{
int ret;
+ FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
+ FAR struct cdcacm_dev_s *priv;
+ char devname[CDCACM_DEVNAME_SIZE];
+
+ /* Create device node path from minor number */
+
+ snprintf(devname, sizeof(devname), CDCACM_DEVNAME_FORMAT, minor);
+
+ /* If classdev is not provided, find it from the file system */
+
+ if (!classdev)
+{
+ FAR struct cdcacm_alloc_s *cdcacm_alloc = find_driver(devname);
+ if (cdcacm_alloc)
+{
+ drvr = &cdcacm_alloc->drvr;
Review Comment:
No, it can't return an inode to a caller, if the caller is not a filesystem
driver.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
jlaitine commented on code in PR #17010: URL: https://github.com/apache/nuttx/pull/17010#discussion_r2349829225 ## include/nuttx/fs/fs.h: ## @@ -1369,6 +1369,22 @@ int fdlist_close(FAR struct fdlist *list, int fd); int nx_close(int fd); +/ + * Name: find_driver + * + * Description: + * Returns the pointer of a registered driver specified by 'pathname' + * + * Input Parameters: + * pathname - the full path to the driver's device node in file system + * + * Returned Value: + * Pointer to driver's registered private pointer or NULL if not found. + * + / + +FAR void *find_driver(FAR const char *pathname); Review Comment: No, this is wrong. find_blockdriver is used only inside fs, so that is fine. You just can't give inodes outside of fs (to drivers/usbdev) and expect to release the inodes there. I won't do that. You can do it yourself if you want it that way. Search for "release_inode" and see where the inodes are released now. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
jlaitine commented on PR #17010: URL: https://github.com/apache/nuttx/pull/17010#issuecomment-3293502178 find_driver function in fs can be used to find a *pointer to a driver*. Not to find and reserve an inode. inodes are file system internal things and should only be handled by filesystem drivers. Not by drivers outside fs. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] cdcacm: Enable using the device pointer stored in filesystem for cdcam_uninitialize [nuttx]
xiaoxiang781216 commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2349729104
##
drivers/usbdev/cdcacm.c:
##
@@ -313,6 +313,10 @@ static const struct uart_ops_s g_uartops =
cdcuart_sendbuf/* sendbuf */
};
+/* Mutex to protect device initialization / uninitialization*/
+
+static mutex_t g_init_lock = NXMUTEX_INITIALIZER;
Review Comment:
but I don't suggest to add lock like this.
##
include/nuttx/fs/fs.h:
##
@@ -1369,6 +1369,22 @@ int fdlist_close(FAR struct fdlist *list, int fd);
int nx_close(int fd);
+/
+ * Name: find_driver
+ *
+ * Description:
+ * Returns the pointer of a registered driver specified by 'pathname'
+ *
+ * Input Parameters:
+ * pathname - the full path to the driver's device node in file system
+ *
+ * Returned Value:
+ * Pointer to driver's registered private pointer or NULL if not found.
+ *
+ /
+
+FAR void *find_driver(FAR const char *pathname);
Review Comment:
it's better to return inode like find_blockdriver
```
int find_driver(FAR const char *pathname, FAR struct inode **ppinode);
```
##
drivers/usbdev/cdcacm.c:
##
@@ -3430,8 +3430,53 @@ void cdcacm_uninitialize(FAR struct usbdevclass_driver_s
*classdev)
{
FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
FAR struct cdcacm_dev_s*priv = drvr->dev;
- char devname[CDCACM_DEVNAME_SIZE];
+
+ cdcacm_uninitialize_instance(priv->minor, classdev);
+}
+
+/
+ * Name: cdcacm_uninitialize_instance
+ *
+ * Description:
+ * Function to uninitialize specific cdcacm instance
+ *
+ * Input Parameters:
+ * minor - CDCACM node minor number
+ *
+ * Returned Value:
+ * OK when successful, -ENODEV if cdcacm is not initialized
+ *
+ /
+
+int cdcacm_uninitialize_instance(int minor,
+ FAR struct usbdevclass_driver_s *classdev)
+{
int ret;
+ FAR struct cdcacm_driver_s *drvr = (FAR struct cdcacm_driver_s *)classdev;
+ FAR struct cdcacm_dev_s *priv;
+ char devname[CDCACM_DEVNAME_SIZE];
+
+ /* Create device node path from minor number */
+
+ snprintf(devname, sizeof(devname), CDCACM_DEVNAME_FORMAT, minor);
+
+ /* If classdev is not provided, find it from the file system */
+
+ if (!classdev)
+{
+ FAR struct cdcacm_alloc_s *cdcacm_alloc = find_driver(devname);
+ if (cdcacm_alloc)
+{
+ drvr = &cdcacm_alloc->drvr;
Review Comment:
but find_driver should have the similar prototype as find_blockdriver anyway.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
