| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dba2fa57-2101-b24a-a482-97878895fb92@collabora.com>
Date: Wed, 4 Dec 2019 11:03:52 +0100
From: Dafna Hirschfeld <dafna.hirschfeld@...labora.com>
To: Lucas A. M. Magalhães <lucmaga@...il.com>,
linux-media@...r.kernel.org
Cc: hverkuil@...all.nl, linux-kernel@...r.kernel.org,
helen.koike@...labora.com, edusbarretto@...il.com,
lkcamp@...ts.libreplanetbr.org
Subject: Re: [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice
Hi,
Thanks for the patch
On 12/4/19 12:01 AM, Lucas A. M. Magalhães wrote:
> From: Lucas A. M. Magalhaes <lucmaga@...il.com>
>
> Add a virtual subdevice to simulate the flash control API.
> Those are the supported controls:
> v4l2-ctl -d /dev/v4l-subdev6 -L
> Flash Controls
>
> led_mode 0x009c0901 (menu) : min=0 max=2 default=1 value=1
> 0: Off
> 1: Flash
> 2: Torch
> strobe_source 0x009c0902 (menu) : min=0 max=1 default=0 value=0
> 0: Software
> 1: External
> strobe 0x009c0903 (button) : flags=write-only, execute-on-write
> stop_strobe 0x009c0904 (button) : flags=write-only, execute-on-write
> strobe_status 0x009c0905 (bool) : default=0 value=0 flags=read-only
> strobe_timeout 0x009c0906 (int) : min=50 max=400 step=50 default=50 value=400
> intensity_flash_mode 0x009c0907 (int) : min=23040 max=1499600 step=11718 default=23040 value=23040
> intensity_torch_mode 0x009c0908 (int) : min=2530 max=187100 step=1460 default=2530 value=2530
> intensity_indicator 0x009c0909 (int) : min=0 max=255 step=1 default=0 value=0
> faults 0x009c090a (bitmask): max=0x00000002 default=0x00000000 value=0x00000000
>
> Co-authored-by: Eduardo Barretto <edusbarretto@...il.com>
> Signed-off-by: Eduardo Barretto <edusbarretto@...il.com>
> Signed-off-by: Lucas A. M. Magalhães <lucmaga@...il.com>
>
> ---
> Hi,
>
> I've copied some values from another driver (lm3646) to make it more
> realistic, as suggested by Hans. All values except for
> V4L2_CID_FLASH_INDICATOR_INTENSITY, which I couldn't find any
> implementation.
>
> The v4l-compliance is failing. From the documentation
> V4L2_CID_FLASH_STROBE should just work if the
> V4L2_CID_FLASH_STROBE_SOURCE is "Software" and the
> V4L2_CID_FLASH_LED_MODE is "Flash", otherwise it should fail. With the
> standard values configured for the V4L2_CID_FLASH_STROBE will not fail.
> But during the tests v4l-compliance sets V4L2_CID_FLASH_LED_MODE to
> "Torch" and V4L2_CID_FLASH_STROBE_SOURCE to "External" which makes
> V4L2_CID_FLASH_STROBE to fail. How do I proceed? Should the
> v4l-compliance be changed?
>
> Changes in v3:
> - Fix style errors
> - Use more realistic numbers for the controllers
> - Change from kthread to workqueue
> - Change commit message for the new controllers values
>
> Changes in v2:
> - Fix v4l2-complience errors
> - Add V4L2_CID_FLASH_STROBE_STATUS behavior
> - Add V4L2_CID_FLASH_STROBE restrictions
> - Remove vimc_fla_g_volatile_ctrl
> - Remove unnecessarie V4L2_CID_FLASH_CLASS
> - Change varables names
>
> drivers/media/platform/vimc/Makefile | 2 +-
> drivers/media/platform/vimc/vimc-common.c | 2 +
> drivers/media/platform/vimc/vimc-common.h | 4 +
> drivers/media/platform/vimc/vimc-core.c | 5 +
> drivers/media/platform/vimc/vimc-flash.c | 248 ++++++++++++++++++++++
> 5 files changed, 260 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/platform/vimc/vimc-flash.c
>
> diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
> index a53b2b532e9f..e759bbb04b14 100644
> --- a/drivers/media/platform/vimc/Makefile
> +++ b/drivers/media/platform/vimc/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
> - vimc-debayer.o vimc-scaler.o vimc-sensor.o
> + vimc-debayer.o vimc-scaler.o vimc-sensor.o vimc-flash.o
>
> obj-$(CONFIG_VIDEO_VIMC) += vimc.o
>
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index a3120f4f7a90..cb786de75573 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -203,6 +203,8 @@ struct media_pad *vimc_pads_init(u16 num_pads, const unsigned long *pads_flag)
> struct media_pad *pads;
> unsigned int i;
>
> + if (!num_pads)
> + return NULL;
Please rebase on top of latest master,
this function was removed in patch 'media: vimc: embed the pads of entities in the entities' structs'
> /* Allocate memory for the pads */
> pads = kcalloc(num_pads, sizeof(*pads), GFP_KERNEL);
> if (!pads)
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 698db7c07645..19815f0f4d40 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -169,6 +169,10 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> const char *vcfg_name);
> void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>
> +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
> + const char *vcfg_name);
This should be lined with the opening '('
> +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +
> /**
> * vimc_pads_init - initialize pads
> *
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 6e3e5c91ae39..5f6c750d3d8d 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -91,6 +91,11 @@ static struct vimc_ent_config ent_config[] = {
> .add = vimc_cap_add,
> .rm = vimc_cap_rm,
> },
> + {
> + .name = "Flash Controller",
> + .add = vimc_fla_add,
> + .rm = vimc_fla_rm,
> + }
> };
>
> static const struct vimc_ent_link ent_links[] = {
> diff --git a/drivers/media/platform/vimc/vimc-flash.c b/drivers/media/platform/vimc/vimc-flash.c
> new file mode 100644
> index 000000000000..3918beecec57
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc-flash.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * vimc-flash.c Virtual Media Controller Driver
> + *
> + * Copyright (C) 2019
> + * Contributors: Lucas A. M. Magalhães <lamm@...maga.dev>
> + * Eduardo Barretto <edusbarretto@...il.com>
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/workqueue.h>
> +#include <linux/sched.h>
> +#include <linux/vmalloc.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include "vimc-common.h"
> +
> +/*
> + * Flash timeout in ms
> + */
> +#define VIMC_FLASH_TIMEOUT_MS_MIN 50
> +#define VIMC_FLASH_TIMEOUT_MS_MAX 400
> +#define VIMC_FLASH_TIMEOUT_MS_STEP 50
> +
> +/*
> + * Torch intencity in uA
> + */
> +#define VIMC_FLASH_TORCH_UA_MIN 2530
> +#define VIMC_FLASH_TORCH_UA_MAX 187100
> +#define VIMC_FLASH_TORCH_UA_STEP 1460
> +
> +/*
> + * Flash intencity in uA
> + */
> +#define VIMC_FLASH_FLASH_UA_MIN 23040
> +#define VIMC_FLASH_FLASH_UA_MAX 1499600
> +#define VIMC_FLASH_FLASH_UA_STEP 11718
> +
> +struct vimc_fla_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct v4l2_ctrl_handler hdl;
> + int strobe_source;
> + bool is_strobe;
> + int led_mode;
> + int indicator_intensity;
> + int torch_intensity;
> + int flash_intensity;
> + u64 timeout;
> + u64 last_strobe;
> + struct workqueue_struct *wq;
> + struct work_struct work;
> + struct v4l2_ctrl *strobe_status_ctl;
> +};
> +
> +static void vimc_fla_strobe_work(struct work_struct *work)
> +{
> + struct vimc_fla_device *vfla =
> + container_of(work, struct vimc_fla_device, work);
> + v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, true);
> + vfla->last_strobe = ktime_get_ns();
> + while (vfla->is_strobe &&
> + vfla->last_strobe + vfla->timeout > ktime_get_ns()) {
> + msleep_interruptible(VIMC_FLASH_TIMEOUT_MS_STEP);
> + }
> + v4l2_ctrl_s_ctrl(vfla->strobe_status_ctl, false);
> +}
> +
> +static int vimc_fla_s_ctrl(struct v4l2_ctrl *c)
> +{
> + struct vimc_fla_device *vfla =
> + container_of(c->handler, struct vimc_fla_device, hdl);
> +
> + switch (c->id) {
> + case V4L2_CID_FLASH_LED_MODE:
> + vfla->led_mode = c->val;
> + return 0;
> + case V4L2_CID_FLASH_STROBE_SOURCE:
> + vfla->strobe_source = c->val;
> + return 0;
> + case V4L2_CID_FLASH_STROBE:
> + if (vfla->led_mode != V4L2_FLASH_LED_MODE_FLASH ||
> + vfla->strobe_source != V4L2_FLASH_STROBE_SOURCE_SOFTWARE){
you can add error/warning debug here,
if you choose not to, then the parentheses are redundant.
Additionally, the opening '{' should come after a space
You can run srctipts/checkpatch.pl before submitting to catch such issues
> + return -EINVAL;
> + }
> + queue_work(vfla->wq, &vfla->work);
> + return 0;
> + case V4L2_CID_FLASH_STROBE_STATUS:
> + vfla->is_strobe = c->val;
> + return 0;
> + case V4L2_CID_FLASH_STROBE_STOP:
> + vfla->is_strobe = false;
> + return 0;
> + case V4L2_CID_FLASH_TIMEOUT:
> + vfla->timeout = c->val * 1000000; /* MS to NS */
> + return 0;
> + case V4L2_CID_FLASH_INTENSITY:
> + vfla->flash_intensity = c->val;
> + return 0;
> + case V4L2_CID_FLASH_TORCH_INTENSITY:
> + vfla->torch_intensity = c->val;
> + return 0;
> + case V4L2_CID_FLASH_INDICATOR_INTENSITY:
> + vfla->indicator_intensity = c->val;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops vimc_fla_ctrl_ops = {
> + .s_ctrl = vimc_fla_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_core_ops vimc_fla_core_ops = {
> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_ops vimc_fla_ops = {
> + .core = &vimc_fla_core_ops,
> +};
> +
> +static void vimc_fla_release(struct v4l2_subdev *sd)
> +{
> + struct vimc_fla_device *vfla =
> + container_of(sd, struct vimc_fla_device, sd);
one tab is enough
> +
> + v4l2_ctrl_handler_free(&vfla->hdl);
> + kfree(vfla);
> +}
> +
> +static const struct v4l2_subdev_internal_ops vimc_fla_int_ops = {
> + .release = vimc_fla_release,
> +};
> +
> +/* initialize device */
> +struct vimc_ent_device *vimc_fla_add(struct vimc_device *vimc,
> + const char *vcfg_name)
> +{
> + struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
> + struct vimc_fla_device *vfla;
> + int ret;
> +
> + /* Allocate the vfla struct */
> + vfla = kzalloc(sizeof(*vfla), GFP_KERNEL);
> + if (!vfla)
> + return NULL;
I think it is better to change the return values of the .add calbbacks
to return ERR_PTR upon error and not just NULL so that different types of
errors can be distinguished.
(This is not related specifically to this patch),
If you want you can send a patch fixing that.
> +
> + v4l2_ctrl_handler_init(&vfla->hdl, 4);
> + v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_LED_MODE,
> + V4L2_FLASH_LED_MODE_TORCH, ~0x7,
> + V4L2_FLASH_LED_MODE_FLASH);
> + v4l2_ctrl_new_std_menu(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_STROBE_SOURCE, 0x1, ~0x3,
> + V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_STROBE, 0, 0, 0, 0);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_TIMEOUT, VIMC_FLASH_TIMEOUT_MS_MIN,
> + VIMC_FLASH_TIMEOUT_MS_MAX,
> + VIMC_FLASH_TIMEOUT_MS_STEP,
> + VIMC_FLASH_TIMEOUT_MS_MIN);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_TORCH_INTENSITY,
> + VIMC_FLASH_TORCH_UA_MIN,
> + VIMC_FLASH_TORCH_UA_MAX,
> + VIMC_FLASH_TORCH_UA_STEP,
> + VIMC_FLASH_TORCH_UA_MIN);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INTENSITY,
> + VIMC_FLASH_FLASH_UA_MIN,
> + VIMC_FLASH_FLASH_UA_MAX,
> + VIMC_FLASH_FLASH_UA_STEP,
> + VIMC_FLASH_FLASH_UA_MIN);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_INDICATOR_INTENSITY,
> + 0,
> + 255,
> + 1,
> + 0);
why not having one line with "0,255,1,0);" ?
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_STROBE_STATUS, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&vfla->hdl, &vimc_fla_ctrl_ops,
> + V4L2_CID_FLASH_FAULT, 0,
> + V4L2_FLASH_FAULT_TIMEOUT, 0, 0);
> + vfla->sd.ctrl_handler = &vfla->hdl;
> + if (vfla->hdl.error) {
> + ret = vfla->hdl.error;
> + goto err_free_vfla;
> + }
> + vfla->strobe_status_ctl = v4l2_ctrl_find(&vfla->hdl,
> + V4L2_CID_FLASH_STROBE_STATUS);
> +
> + /* Initialize ved and sd */
> + ret = vimc_ent_sd_register(&vfla->ved, &vfla->sd, v4l2_dev,
> + vcfg_name,
> + MEDIA_ENT_F_FLASH, 0, NULL,
> + &vimc_fla_int_ops, &vimc_fla_ops);
> + if (ret)
> + goto err_free_hdl;
> +
> + /* Create processing workqueue */
> + vfla->wq = alloc_workqueue("%s", 0, 0, "vimc-flash thread");
> + if (!vfla->wq)
> + goto err_unregister;
> +
> + INIT_WORK(&vfla->work, vimc_fla_strobe_work);
> + /* Initialize standard values */
> + vfla->indicator_intensity = 0;
> + vfla->torch_intensity = 0;
> + vfla->flash_intensity = 0;
> + vfla->is_strobe = false;
> + vfla->timeout = 0;
> + vfla->last_strobe = 0;
> + vfla->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
> + vfla->led_mode = V4L2_FLASH_LED_MODE_FLASH;
> +
> + return &vfla->ved;
> +
> +err_unregister:
> + vimc_ent_sd_unregister(&vfla->ved, &vfla->sd);
> +err_free_hdl:
> + v4l2_ctrl_handler_free(&vfla->hdl);
> +err_free_vfla:
> + kfree(vfla);
> +
> + return NULL;
> +}
> +
> +void vimc_fla_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
> +{
> + struct vimc_fla_device *vfla;
> +
> + if (!ved)
> + return;
> +
> + vfla = container_of(ved, struct vimc_fla_device, ved);
> + destroy_workqueue(vfla->wq);
> + vimc_ent_sd_unregister(ved, &vfla->sd);
> +}
>
Not sure but I think there are indentation issues in this patch, a tab should be 8 spaces
Thanks,
Dafna
Powered by blists - more mailing lists