| Message ID | 1437733296-38198-4-git-send-email-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, On Fri, Jul 24, 2015 at 12:21:32PM +0200, Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > Add new helper functions that report back if this was the first open > or last close. > > Signed-off-by: Hans Verkuil <hans.verkuil@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
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.verkuil@cisco.com> >> >> Add new helper functions that report back if this was the first open >> or last close. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.