Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on PR #11008: URL: https://github.com/apache/nuttx/pull/11008#issuecomment-1991467138 Ok, let me close this 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 closed pull request #11008: drivers/video: Update unlink() and video_uninitialize() operation URL: https://github.com/apache/nuttx/pull/11008 -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
SPRESENSE commented on PR #11008: URL: https://github.com/apache/nuttx/pull/11008#issuecomment-1989855041 @xiaoxiang781216 @Donny9 Thank you for information. I verified that work well with #11892 without my change. I withdraw this 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
Donny9 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1519621769
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
@SPRESENSE please refs to https://github.com/apache/nuttx/pull/11892
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1519352890
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
Ok, after compare the mainline with our local git, I found @Donny9 hit the
similar issue when dealing with USB hotplug, and change unreigster_driver to
fix the problem.
unreigster_driver is better place to fix this issue. @Donny9 could you
upstream the patch? so @SPRESENSE could verify whether your change work in this
case too.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1519352890
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
Ok, after compare the mainline with our local git, I found @Donny9 hit the
similar issue when dealing with USB hotplug, and change unreigster_driver to
fix the problem.
unreigster_driver is better place to fix this issue. @Donny9 could you
upstream the patch? so @SPRESENSE could veriy whether your change work in this
case too.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1519352890
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
Ok, after compare the mainline with our local git, I found the similar issue
when dealing with USB hotplug, and change unreigster_driver to fix the problem.
unreigster_driver is better place to fix this issue. @Donny9 could you
upstream the patch? so @SPRESENSE could veriy whether your change work in this
case too.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1519352890
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
Ok, after compare the mainline with our local git, we found the similar
issue when dealing with USB hotplug, and change unreigster_driver to fix the
problem.
unreigster_driver is better place to fix this issue. @Donny9 could you
upstream the patch? so @SPRESENSE could veriy whether your change work in this
case too.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
SPRESENSE commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1519342452
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
@xiaoxiang781216 When unregister_driver is executed, the device file is
removed, but video_unlink() does not seem to be called.
(This was confirmed with my Spresense environment.)
So, this change is necessary to avoid memory leak.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1518727527
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
unregister_driver will call nx_unlink internally, why do we need make this
change?
##
drivers/video/video.c:
##
@@ -3488,7 +3494,16 @@ int video_register(FAR const char *devpath,
int video_unregister(FAR const char *devpath)
{
- return unregister_driver(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
Review Comment:
unregister_driver will call nx_unlink internally, why do we need make this
change? @SPRESENSE
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
SPRESENSE commented on PR #11008: URL: https://github.com/apache/nuttx/pull/11008#issuecomment-1984986467 @xiaoxiang781216 I moved resource release from video_uninitialize() to video_unregister(). -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
hiragane-mitty commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1517123606
##
drivers/video/video.c:
##
@@ -1015,6 +1017,18 @@ static void cleanup_resources(FAR video_mng_t *vmng)
cleanup_scenes_parameter(vmng);
}
+static void cleanup_private_data(FAR struct inode *inode)
+{
+ FAR video_mng_t *priv = inode->i_private;
Review Comment:
@xiaoxiang781216 I fixed it.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on PR #11008: URL: https://github.com/apache/nuttx/pull/11008#issuecomment-1846582319 > @xiaoxiang781216 Yes, it's as you say. video_unregister() should free resource, and video_uninitialize()/video_unlink() might be necessary to call video_unregister. so we should move the change to video_unregister. -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
SPRESENSE commented on PR #11008: URL: https://github.com/apache/nuttx/pull/11008#issuecomment-1846479059 @xiaoxiang781216 Yes, it's as you say. video_unregister() should free resource, and video_uninitialize()/video_unlink() might be necessary to call video_unregister. -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1408123417
##
drivers/video/video.c:
##
@@ -1015,6 +1017,18 @@ static void cleanup_resources(FAR video_mng_t *vmng)
cleanup_scenes_parameter(vmng);
}
+static void cleanup_private_data(FAR struct inode *inode)
+{
+ FAR video_mng_t *priv = inode->i_private;
Review Comment:
```suggestion
FAR video_mng_t *priv = inode->i_private;
```
##
drivers/video/video.c:
##
@@ -3410,7 +3416,16 @@ int video_initialize(FAR const char *devpath)
int video_uninitialize(FAR const char *devpath)
{
- return video_unregister(devpath);
Review Comment:
but if user call video_unregister instead video_uninitialize, the leak still
happen too
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
SPRESENSE commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1406944670
##
drivers/video/video.c:
##
@@ -3410,7 +3416,16 @@ int video_initialize(FAR const char *devpath)
int video_uninitialize(FAR const char *devpath)
{
- return video_unregister(devpath);
Review Comment:
@xiaoxiang781216
Sorry for the late reply.
I think "No".
For examples, drivers/video/goldfish.c use the video_register().
some drivers may use the video_register() and video_unregister().
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
SPRESENSE commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1406944349
##
drivers/video/video.c:
##
@@ -3410,7 +3416,16 @@ int video_initialize(FAR const char *devpath)
int video_uninitialize(FAR const char *devpath)
{
- return video_unregister(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
+ if (ret == 0)
Review Comment:
@xiaoxiang78126 Sorry for the late reply.
If unlink() has already been executed, /dev/video file is deleted.
this validates that unlink() is not called.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] drivers/video: Update unlink() and video_uninitialize() operation [nuttx]
xiaoxiang781216 commented on code in PR #11008:
URL: https://github.com/apache/nuttx/pull/11008#discussion_r1369660623
##
drivers/video/video.c:
##
@@ -3410,7 +3416,16 @@ int video_initialize(FAR const char *devpath)
int video_uninitialize(FAR const char *devpath)
{
- return video_unregister(devpath);
Review Comment:
should we fix video_unregister?
##
drivers/video/video.c:
##
@@ -3410,7 +3416,16 @@ int video_initialize(FAR const char *devpath)
int video_uninitialize(FAR const char *devpath)
{
- return video_unregister(devpath);
+ int ret;
+ struct stat buf;
+
+ ret = nx_stat(devpath, , 1);
+ if (ret == 0)
Review Comment:
why not call nx_unlink directly
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
