Re: [PR] cdcacm: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348117515
##
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:
Returning an inode in here, leaks the ownership of the inode from drivers/fs
into drivers/usbdev, which is not nice. It is better that inode refcounts are
kept within drivers/fs . In other words, I find it messy to bump the refcount
in drivers/fs but release it elsewhere.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348078274
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
I now added a new function (find_driver) under fs/driver, and use that in
cdcacm driver to get the driver instance. This works the same now for all the
cdcacm instances (the boardctrl ioctl can connect/disconnect by the cdcacm
number).
Also the same mechanism is usable for any driver which registers itself into
the filesystem, if such needed in the future.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on PR #17010: URL: https://github.com/apache/nuttx/pull/17010#issuecomment-3291136151 @xiaoxiang781216 I added the inode_lock around the search as well as a mutex (as a separate patch) into cdcacm driver. The mutex now fixes the existing race condition which you pointed out, but of course only in the case when the pointer is managed in kernel/fs only. Many board files manage the pointer directly, but in these cases it is up to the boards to fix any races if ever needed. -- 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: When using system_cdcacm, store the handle in kernel [nuttx]
xiaoxiang781216 commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344553312
##
drivers/usbdev/cdcacm.c:
##
@@ -254,6 +254,10 @@ static voidcdcuart_dmareceive(FAR struct uart_dev_s
*dev);
static FAR struct cdcacm_dev_s *g_syslog_cdcacm;
#endif
+#ifdef CONFIG_SYSTEM_CDCACM
+FAR struct usbdevclass_driver_s *g_system_cdcacm;
Review Comment:
where we really use g_system_cdcacm?
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
why cdcacm_uninitialize doesn't work
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010: URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344647494 ## drivers/usbdev/cdcacm.c: ## @@ -254,6 +254,10 @@ static voidcdcuart_dmareceive(FAR struct uart_dev_s *dev); static FAR struct cdcacm_dev_s *g_syslog_cdcacm; #endif +#ifdef CONFIG_SYSTEM_CDCACM +FAR struct usbdevclass_driver_s *g_system_cdcacm; Review Comment: This PR is just to support "sercon" and "serdis" apps also in "CONFIG_BUILD_KERNEL". If sercon is bult as a standalone process, storing the driver pointer in here: https://github.com/apache/nuttx-apps/blob/df711238fee7423ead0f44e1ca039d1d003175bf/system/cdcacm/cdcacm_main.c#L48 https://github.com/apache/nuttx-apps/blob/df711238fee7423ead0f44e1ca039d1d003175bf/system/cdcacm/cdcacm_main.c#L90 just doesn't work. It is gone after the process exits. Also, when initializing the system_cdcacm from board files, the user needs to store the pointer and later pass it to the cdcacm_uninitialize. This is just extra work, there can be only one "system cdcacm" so it can be simply stored in cdcacm driver. This change doesn't break the functionality of existing board files though. -- 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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348113061
##
fs/driver/fs_finddriver.c:
##
@@ -0,0 +1,78 @@
+/
+ * fs/driver/fs_finddriver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+
+#include "inode/inode.h"
+
+/
+ * Public Functions
+ /
+
+/
+ * 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)
+{
+ struct inode_search_s desc;
+ FAR void *drvr = NULL;
+
+ DEBUGASSERT(pathname != NULL);
+
+ /* Find the inode registered with this pathname */
+
+ SETUP_SEARCH(&desc, pathname, false);
Review Comment:
I was thinking that it is not necessary here, because only the driver code
itself is finding it's own pointers. And it is also the driver code itself who
registers and unregisters itself to the filesystem.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
xiaoxiang781216 commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348096371
##
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:
should we return inode with refcount like find_blockdriver to avoid other
threads destroy the driver to make drv pointer stale.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348110800
##
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 it is the driver itself who destroys it, wouldn't it be the driver's
responsibility to lock if needed?
I don't think that we should use the filesystem's inode lock for that
purpose; the driver existence is not hard-bound to whether it is registered in
the filesystem or not.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on PR #17010: URL: https://github.com/apache/nuttx/pull/17010#issuecomment-3291240388 Still made a small cleanup for the inode_lock, it is not necessary to keep it around the search setup, just around inode_find. -- 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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348218148
##
fs/driver/fs_finddriver.c:
##
@@ -0,0 +1,78 @@
+/
+ * fs/driver/fs_finddriver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+
+#include "inode/inode.h"
+
+/
+ * Public Functions
+ /
+
+/
+ * 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)
+{
+ struct inode_search_s desc;
+ FAR void *drvr = NULL;
+
+ DEBUGASSERT(pathname != NULL);
+
+ /* Find the inode registered with this pathname */
+
+ SETUP_SEARCH(&desc, pathname, false);
Review Comment:
Ok, I'll add inode_lock around the search
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348213651
##
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:
If this is an issue, it is an issue also now, right? This PR doesn't change
that behaviour. There is no locking with the pointer atm. if it is stored in
"cdcacm" in CONFIG_BUILD_FLAT or CONFIG_BUILD_PROTECTED.
We can add a mutex around cdcacm_intialize, and
cdcacm_uninitialize_instance, if you like. But that is not relally related to
this PR.
In any case, device protecting itself should not be done with inode lock or
inode refcount. Those are only for the filesystem.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
xiaoxiang781216 commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348142201
##
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:
user may unplug cable randomly which may make the pointer invaid.
##
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:
user may unplug cable randomly which may make the pointer invalid.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
xiaoxiang781216 commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348139563
##
fs/driver/fs_finddriver.c:
##
@@ -0,0 +1,78 @@
+/
+ * fs/driver/fs_finddriver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+
+#include "inode/inode.h"
+
+/
+ * Public Functions
+ /
+
+/
+ * 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)
+{
+ struct inode_search_s desc;
+ FAR void *drvr = NULL;
+
+ DEBUGASSERT(pathname != NULL);
+
+ /* Find the inode registered with this pathname */
+
+ SETUP_SEARCH(&desc, pathname, false);
Review Comment:
other part may register/unregister driver concurrently which may make the
list change in the middle of search
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348113061
##
fs/driver/fs_finddriver.c:
##
@@ -0,0 +1,78 @@
+/
+ * fs/driver/fs_finddriver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+
+#include "inode/inode.h"
+
+/
+ * Public Functions
+ /
+
+/
+ * 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)
+{
+ struct inode_search_s desc;
+ FAR void *drvr = NULL;
+
+ DEBUGASSERT(pathname != NULL);
+
+ /* Find the inode registered with this pathname */
+
+ SETUP_SEARCH(&desc, pathname, false);
Review Comment:
I was thinking that it is not necessary here, because only the driver code
itself is finding it's own pointers. And it is also the driver code itself who
registers and unregisters itself.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348117515
##
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:
Returning an inode to here leaks the ownership of the inode from drivers/fs
into drivers/usbdev, which is not nice. It is better that inode refcounts are
kept within drivers/fs . In other words, I find it messy to bump the refcount
in drivers/fs but release it elsewhere.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348113061
##
fs/driver/fs_finddriver.c:
##
@@ -0,0 +1,78 @@
+/
+ * fs/driver/fs_finddriver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+
+#include "inode/inode.h"
+
+/
+ * Public Functions
+ /
+
+/
+ * 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)
+{
+ struct inode_search_s desc;
+ FAR void *drvr = NULL;
+
+ DEBUGASSERT(pathname != NULL);
+
+ /* Find the inode registered with this pathname */
+
+ SETUP_SEARCH(&desc, pathname, false);
Review Comment:
I was thinking that it is not necessary here, because only the driver code
itself is finding it's own pointers
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
xiaoxiang781216 commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2348086591
##
fs/driver/fs_finddriver.c:
##
@@ -0,0 +1,78 @@
+/
+ * fs/driver/fs_finddriver.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+/
+ * Included Files
+ /
+
+#include
+#include
+
+#include "inode/inode.h"
+
+/
+ * Public Functions
+ /
+
+/
+ * 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)
+{
+ struct inode_search_s desc;
+ FAR void *drvr = NULL;
+
+ DEBUGASSERT(pathname != NULL);
+
+ /* Find the inode registered with this pathname */
+
+ SETUP_SEARCH(&desc, pathname, false);
Review Comment:
do we need hold inode lock?
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2347160399
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
Sure, it would be more generic of course. Although, instead of making
another list, I'd rather just search the drivers by device node name from
filesystem in that case.
I actually have a draft ready, I'll clean it up, test and push it tomorrow
for review, perhaps it is better :)
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
xiaoxiang781216 commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2347088299
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
Another approach is making system/cdcacm become a daemon, so it can retrieve
the handle in disconnect.
But, even with the current approach, it's better to hold the handle in
boards/boardctl.c instead each individual usb driver since it's a common
problem happen in other similar drivers(e.g. USB mass storage), we need a
general solution. In the board file we can save the handle in an array or link
list to support the multiple handle easily.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344650211
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
There is no place to store the ctrl->handle in "sercon" or any other user
side app when it is a standalone process. So the handle can't be passed in by
"serdis".
So this doesn't work in CONFIG_BUILD_KERNEL:
https://github.com/apache/nuttx-apps/blob/df711238fee7423ead0f44e1ca039d1d003175bf/system/cdcacm/cdcacm_main.c#L137
An NSH command "serdis" would be creating a new process with a NULL handle.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344659956
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
Of course; it would be possible to dig out the handle from the filesystem,
by using the "instance" number ("x"), generating the filename "/dev/ttyACMx",
finding the inode and finally using inode->priv. I just feel that it is
unnecessarily complex for this purpose. Again, if someone uses
"CONFIG_SYSTEM_CDCACM", there is only just one of those, and it is simple to
store the pointer.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344659956
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
Of course; it would be possible to dig out the handle from the filesystem,
by using the "instance" number ("x"), generating the filename "/dev/ttyACMx",
finding the inode and finally using inode->priv. I just feel that it is
unnecessarily complex for this purpose. Again, if someone uses
"CONFIG_SYSTEM_CDCACM", there is only one of those, and it is simple to store
the pointer.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344650211
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
There is no place to store the ctrl->handle in "sercon" or any other user
side app when it is a standalone process. So the handle can't be passed in by
"serdis".
So this doesn't work in CONFIG_BUILD_KERNEL:
https://github.com/apache/nuttx-apps/blob/df711238fee7423ead0f44e1ca039d1d003175bf/system/cdcacm/cdcacm_main.c#L137
Call to serdis would be creating a new process with a NULL handle.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344659956
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
Of course; it would be possible to dig out the handle from the filesystem,
by using the "instance" number ("x"), generating the filename "/dev/ttyACMx",
finding the inode and finally using inode->priv. I just feel that it is
unnecessary complex for this purpose, again, if someone uses
"CONFIG_SYSTEM_CDCACM", there is only just one of those, and it is simple to
store the pointer.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344659956
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
Of course; it would be possible to dig out the handle from the filesystem,
by using the "instance" number ("x"), generating the filename "/dev/ttyACMx",
finding the inode and finally using inode->priv. I just feel that it is
unnecessary complex for this purpose. Again, if someone uses
"CONFIG_SYSTEM_CDCACM", there is only just one of those, and it is simple to
store the pointer.
--
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: When using system_cdcacm, store the handle in kernel [nuttx]
jlaitine commented on code in PR #17010:
URL: https://github.com/apache/nuttx/pull/17010#discussion_r2344650211
##
boards/boardctl.c:
##
@@ -133,16 +133,25 @@ 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);
+#ifdef CONFIG_SYSTEM_CDCACM
+if (ctrl->instance == CONFIG_SYSTEM_CDCACM_DEVMINOR)
+ {
+ret = cdcacm_uninitialize_system_cdcacm();
+ }
+else
+#endif
+ {
+DEBUGASSERT(ctrl->handle != NULL &&
+*ctrl->handle != NULL);
+cdcacm_uninitialize(*ctrl->handle);
Review Comment:
There is no place to store the ctrl->handle in "sercon" or any other user
side app when it is a standalone process. So the handle can't be passed in by
"serdis".
--
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]
