Hi Jacek,
On Wed, Jun 03, 2015 at 09:56:39AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 06/02/2015 05:32 PM, Sakari Ailus wrote:
> >Hi, Jacek!
> >
> >On Tue, Jun 02, 2015 at 11:13:54AM +0200, Jacek Anaszewski wrote:
> >>Hi Sakari,
> >>
> >>On 06/01/2015 10:59 PM, Sakari Ailus wrote:
> >>>Hi Jacek,
> >>>
> >>>On Mon, May 25, 2015 at 05:13:57PM +0200, Jacek Anaszewski wrote:
> >>>>This patch adds helper functions for registering/unregistering
> >>>>LED Flash class devices as V4L2 sub-devices. The functions should
> >>>>be called from the LED subsystem device driver. In case the
> >>>>support for V4L2 Flash sub-devices is disabled in the kernel
> >>>>config the functions' empty versions will be used.
> >>>>
> >>>>Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
> >>>>Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> >>>>Cc: Sakari Ailus <sakari.ailus@xxxxxx>
> >>>>Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>>
> >>>Thanks for adding indicator support!
> >>>
> >>>Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >>>
> >>
> >>I missed one thing - sysfs interface of the indicator LED class
> >>also has to be disabled/enabled of v4l2_flash_open/close.
> >
> >Good catch.
> >
> >>
> >>I am planning to reimplement the functions as follows,
> >>please let me know if you see any issues here:
> >>
> >>static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> >>*fh)
> >>{
> >> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
> >>
> >> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> >>
> >> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> >> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> >>
> >> struct led_classdev *led_cdev_ind;
> >>
> >> int ret = 0;
> >
> >I think you could spare some newlines above (and below as well).
>
> There must have been some issue on thunderbird side,
> originally there were no newlines above.
Ok. I've used Seamonkey, I believe it uses the same editor. Some bugs were
unintroduced a few years ago, and since that I've mostly used a different
editor (and MUA) when replying to patches.
> >>
> >>
> >> mutex_lock(&led_cdev->led_access);
> >>
> >>
> >> if (!v4l2_fh_is_singular(&fh->vfh))
> >>
> >> goto unlock;
> >>
> >>
> >> led_sysfs_disable(led_cdev);
> >> led_trigger_remove(led_cdev);
> >>
> >>
> >> if (iled_cdev) {
> >> led_cdev_ind = &iled_cdev->led_cdev;
> >
> >You can also declare led_cdev_ind here as you don't need it outside the
> >basic block.
>
> With new approach it will be required also in error path.
True.
> >>
> >>
> >> mutex_lock(&led_cdev_ind->led_access);
> >>
> >>
> >> led_sysfs_disable(led_cdev_ind);
> >> led_trigger_remove(led_cdev_ind);
> >>
> >>
> >> mutex_unlock(&led_cdev_ind->led_access);
> >
> >Please don't acquire the indicator mutex while holding the flash mutex. I
> >don't think there's a need to do so, thus creating a dependency between the
> >two.Just remember to check for v4l2_fh_is_singular() in both cases.
>
> I thought that the code would be simpler this way. Nevertheless I
> produced a new version by following your advise.
>
> >
> >>
> >> }
> >>
> >>
> >> ret = __sync_device_with_v4l2_controls(v4l2_flash);
> >
> >If ret is < 0 here, you end up disabling the sysfs interface while open()
> >fails (and v4l2_flash_close() will not be run). Shouldn't you handle that?
>
> Good point.
>
> Please find the new version of v4l2_flash{open|close} functions below:
>
> static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> *fh)
> {
> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> struct led_classdev *led_cdev_ind = NULL;
> int ret = 0;
>
> mutex_lock(&led_cdev->led_access);
>
> if (!v4l2_fh_is_singular(&fh->vfh)) {
Hmm. I just realised that this might be a bit broken ---
v4l2_fh_is_singular() should return the same value independently of where in
subdev's open() handler it is called.
But adding file handles to the list and checking how many there are of them
is not properly serialised, i.e. another process could change the list in
between. I'll try to submit a patch for that. No need to wait for that
though.
So please ignore my earlier request to check for v4l2_fh_is_singular() for
multiple times, it won't help. Once is enough, albeit more than that won't
hurt.
> mutex_unlock(&led_cdev->led_access);
> return 0;
> }
>
> led_sysfs_disable(led_cdev);
> led_trigger_remove(led_cdev);
>
> mutex_unlock(&led_cdev->led_access);
>
> if (iled_cdev) {
> led_cdev_ind = &iled_cdev->led_cdev;
>
> mutex_lock(&led_cdev_ind->led_access);
>
> if (!v4l2_fh_is_singular(&fh->vfh)) {
> mutex_unlock(&led_cdev_ind->led_access);
> return 0;
> }
>
> led_sysfs_disable(led_cdev_ind);
> led_trigger_remove(led_cdev_ind);
>
> mutex_unlock(&led_cdev_ind->led_access);
> }
>
> ret = __sync_device_with_v4l2_controls(v4l2_flash);
> if (ret < 0)
> goto out_sync_device;
>
> return 0;
> out_sync_device:
> mutex_lock(&led_cdev->led_access);
> if (v4l2_fh_is_singular(&fh->vfh))
> led_sysfs_enable(led_cdev);
> mutex_unlock(&led_cdev->led_access);
>
> if (led_cdev_ind) {
> mutex_lock(&led_cdev_ind->led_access);
> if (v4l2_fh_is_singular(&fh->vfh))
> led_sysfs_enable(led_cdev_ind);
> mutex_unlock(&led_cdev_ind->led_access);
> }
>
> return ret;
> }
>
> static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> *fh)
> {
> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> int ret = 0;
>
> mutex_lock(&led_cdev->led_access);
>
> if (v4l2_fh_is_singular(&fh->vfh)) {
> if (v4l2_flash->ctrls[STROBE_SOURCE])
> ret =
> v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[STROBE_SOURCE],
> V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
> led_sysfs_enable(led_cdev);
> }
>
> mutex_unlock(&led_cdev->led_access);
>
> if (iled_cdev) {
> struct led_classdev *led_cdev_ind = &iled_cdev->led_cdev;
>
> mutex_lock(&led_cdev_ind->led_access);
>
> if (v4l2_fh_is_singular(&fh->vfh))
> led_sysfs_enable(led_cdev_ind);
>
> mutex_unlock(&led_cdev_ind->led_access);
> }
>
> return ret;
> }
>
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
