Re: [PATCH v9 2/8] media: Add registration helpers for V4L2 flash sub-devices

[Jacek Anaszewski <j.anaszewski@samsung.com>]

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@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>>>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> Thanks for adding indicator support!
>>>
>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>
>>
>> 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.

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

>>
>>
>>                  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)) {
                 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;
}





-- 
Best Regards,
Jacek Anaszewski