Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/video: Update unlink() and video_uninitialize() operation #11008

Closed
wants to merge 1 commit into from

Conversation

SPRESENSE
Copy link
Contributor

Summary

Add unregister_driver() execution to unlink() operation to delete device file.
Also, add resource cleanup to video_uninitialize() operation to avoid memory leak without unlink() execution for compatibility.

Impact

drivers/video

Testing

Tested with spresense

drivers/video/video.c Outdated Show resolved Hide resolved
drivers/video/video.c Outdated Show resolved Hide resolved
drivers/video/video.c Outdated Show resolved Hide resolved
drivers/video/video.c Outdated Show resolved Hide resolved
@SPRESENSE
Copy link
Contributor Author

@xiaoxiang781216 Yes, it's as you say.
video_unregister() should free resource,
and video_uninitialize()/video_unlink() might be necessary to call video_unregister.

@xiaoxiang781216
Copy link
Contributor

@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.

Add unregister_driver() execution to unlink() operation
to delete device file.
Also, add resource cleanup to video_uninitialize() operation
to avoid memory leak without unlink() execution for compatibility.
@SPRESENSE
Copy link
Contributor Author

@xiaoxiang781216 I moved resource release from video_uninitialize() to video_unregister().

int ret;
struct stat buf;

ret = nx_stat(devpath, &buf, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unregister_driver will call nx_unlink internally, why do we need make this change? @SPRESENSE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SPRESENSE please refs to #11892

@SPRESENSE
Copy link
Contributor Author

@xiaoxiang781216 @Donny9 Thank you for information. I verified that work well with #11892 without my change. I withdraw this PR.

@xiaoxiang781216
Copy link
Contributor

Ok, let me close this pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants