Re: [RFC PATCH 3/7] v4l2-fh: add v4l2_fh_open_is_first and v4l2_fh_release_is_last
Hi Sakari,
On 07/26/2015 12:42 AM, Sakari Ailus wrote:
Hi Hans,
On Fri, Jul 24, 2015 at 12:21:32PM +0200, Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
Add new helper functions that report back if this was the first open
or last close.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
...
@@ -65,11 +65,23 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct
video_device *vdev);
*/
bool v4l2_fh_add(struct v4l2_fh *fh);
/*
+ * It allocates a v4l2_fh and inits and adds it to the video_device
associated
+ * with the file pointer. In addition it returns true if this was the first
+ * open and false otherwise. The error code is returned in *err.
+ */
+bool v4l2_fh_open_is_first(struct file *filp, int *err);
The new interface functions look a tad clumsy to me.
What would you think of returning the singularity value from v4l2_fh_open()
straight away? Negative integers are errors, so zero and positive values are
free.
A few drivers just check if the value is non-zero and then return that
value, but there are just a handful of those.
I don't like messing with that. It can be done because all driver opens go
through
either v4l2-dev.c or v4l2-subdev.c and we can convert a return value of 0 to
0. We
have to do that since fs/open.c doesn't check for 0, just != 0.
But that makes our 'open' implementation non-standard, and I feel that that's
both confusing and increases the chances of future bugs (precisely because it is
unexpected).
In pretty much all open() implementations of drivers you will have a 'int err;'
variable or something similar, so being able to do:
if (v4l2_fh_open_is_first(file, err)) {
}
is actually quite efficient (see patch 7/7).
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/7] v4l2-fh: add v4l2_fh_open_is_first and v4l2_fh_release_is_last
Hi Hans,
On Fri, Jul 24, 2015 at 12:21:32PM +0200, Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
Add new helper functions that report back if this was the first open
or last close.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
...
@@ -65,11 +65,23 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
*vdev);
*/
bool v4l2_fh_add(struct v4l2_fh *fh);
/*
+ * It allocates a v4l2_fh and inits and adds it to the video_device
associated
+ * with the file pointer. In addition it returns true if this was the first
+ * open and false otherwise. The error code is returned in *err.
+ */
+bool v4l2_fh_open_is_first(struct file *filp, int *err);
The new interface functions look a tad clumsy to me.
What would you think of returning the singularity value from v4l2_fh_open()
straight away? Negative integers are errors, so zero and positive values are
free.
A few drivers just check if the value is non-zero and then return that
value, but there are just a handful of those.
+/*
* Can be used as the open() op of v4l2_file_operations.
* It allocates a v4l2_fh and inits and adds it to the video_device
associated
* with the file pointer.
*/
-int v4l2_fh_open(struct file *filp);
+static inline int v4l2_fh_open(struct file *filp)
+{
+ int err;
+
+ v4l2_fh_open_is_first(filp, err);
+ return err;
+}
/*
* Remove file handle from the list of file handles. Must be called in
* v4l2_file_operations-release() handler if the driver uses v4l2_fh.
@@ -86,12 +98,23 @@ bool v4l2_fh_del(struct v4l2_fh *fh);
*/
void v4l2_fh_exit(struct v4l2_fh *fh);
/*
+ * It deletes and exits the v4l2_fh associated with the file pointer and
+ * frees it. It will do nothing if filp-private_data (the pointer to the
+ * v4l2_fh struct) is NULL. This function returns true if this was the
+ * last open file handle and false otherwise.
+ */
+bool v4l2_fh_release_is_last(struct file *filp);
+/*
* Can be used as the release() op of v4l2_file_operations.
* It deletes and exits the v4l2_fh associated with the file pointer and
* frees it. It will do nothing if filp-private_data (the pointer to the
* v4l2_fh struct) is NULL. This function always returns 0.
*/
-int v4l2_fh_release(struct file *filp);
+static inline int v4l2_fh_release(struct file *filp)
+{
+ v4l2_fh_release_is_last(filp);
+ return 0;
+}
/*
* Returns true if this filehandle is the only filehandle opened for the
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 3/7] v4l2-fh: add v4l2_fh_open_is_first and v4l2_fh_release_is_last
From: Hans Verkuil hans.verk...@cisco.com
Add new helper functions that report back if this was the first open
or last close.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/v4l2-core/v4l2-fh.c | 25 ++---
include/media/v4l2-fh.h | 27 +--
2 files changed, 39 insertions(+), 13 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-fh.c
b/drivers/media/v4l2-core/v4l2-fh.c
index 325e378..ac9c028 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -66,19 +66,21 @@ bool v4l2_fh_add(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_fh_add);
-int v4l2_fh_open(struct file *filp)
+bool v4l2_fh_open_is_first(struct file *filp, int *err)
{
struct video_device *vdev = video_devdata(filp);
struct v4l2_fh *fh = kzalloc(sizeof(*fh), GFP_KERNEL);
+ *err = fh ? -ENOMEM : 0;
+
filp-private_data = fh;
- if (fh == NULL)
- return -ENOMEM;
- v4l2_fh_init(fh, vdev);
- v4l2_fh_add(fh);
- return 0;
+ if (fh) {
+ v4l2_fh_init(fh, vdev);
+ return v4l2_fh_add(fh);
+ }
+ return false;
}
-EXPORT_SYMBOL_GPL(v4l2_fh_open);
+EXPORT_SYMBOL_GPL(v4l2_fh_open_is_first);
bool v4l2_fh_del(struct v4l2_fh *fh)
{
@@ -103,18 +105,19 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_fh_exit);
-int v4l2_fh_release(struct file *filp)
+bool v4l2_fh_release_is_last(struct file *filp)
{
struct v4l2_fh *fh = filp-private_data;
+ bool is_last = false;
if (fh) {
- v4l2_fh_del(fh);
+ is_last = v4l2_fh_del(fh);
v4l2_fh_exit(fh);
kfree(fh);
}
- return 0;
+ return is_last;
}
-EXPORT_SYMBOL_GPL(v4l2_fh_release);
+EXPORT_SYMBOL_GPL(v4l2_fh_release_is_last);
bool v4l2_fh_is_singular(struct v4l2_fh *fh)
{
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 17215fc..e937866 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -65,11 +65,23 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
*vdev);
*/
bool v4l2_fh_add(struct v4l2_fh *fh);
/*
+ * It allocates a v4l2_fh and inits and adds it to the video_device associated
+ * with the file pointer. In addition it returns true if this was the first
+ * open and false otherwise. The error code is returned in *err.
+ */
+bool v4l2_fh_open_is_first(struct file *filp, int *err);
+/*
* Can be used as the open() op of v4l2_file_operations.
* It allocates a v4l2_fh and inits and adds it to the video_device associated
* with the file pointer.
*/
-int v4l2_fh_open(struct file *filp);
+static inline int v4l2_fh_open(struct file *filp)
+{
+ int err;
+
+ v4l2_fh_open_is_first(filp, err);
+ return err;
+}
/*
* Remove file handle from the list of file handles. Must be called in
* v4l2_file_operations-release() handler if the driver uses v4l2_fh.
@@ -86,12 +98,23 @@ bool v4l2_fh_del(struct v4l2_fh *fh);
*/
void v4l2_fh_exit(struct v4l2_fh *fh);
/*
+ * It deletes and exits the v4l2_fh associated with the file pointer and
+ * frees it. It will do nothing if filp-private_data (the pointer to the
+ * v4l2_fh struct) is NULL. This function returns true if this was the
+ * last open file handle and false otherwise.
+ */
+bool v4l2_fh_release_is_last(struct file *filp);
+/*
* Can be used as the release() op of v4l2_file_operations.
* It deletes and exits the v4l2_fh associated with the file pointer and
* frees it. It will do nothing if filp-private_data (the pointer to the
* v4l2_fh struct) is NULL. This function always returns 0.
*/
-int v4l2_fh_release(struct file *filp);
+static inline int v4l2_fh_release(struct file *filp)
+{
+ v4l2_fh_release_is_last(filp);
+ return 0;
+}
/*
* Returns true if this filehandle is the only filehandle opened for the
* associated video_device. If fh is NULL, then it returns false.
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
