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
>>
>> Add new helper functions that report back if this was the first open
>> or last close.
>>
>> Signed-off-by: Hans Verkuil
>> ---
>
> ...
>
>> @@ -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
>
> Add new helper functions that report back if this was the first open
> or last close.
>
> Signed-off-by: Hans Verkuil
> ---
...
> @@ -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
Add new helper functions that report back if this was the first open
or last close.
Signed-off-by: Hans Verkuil
---
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
