| 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: <cd9bcde6-eb00-2576-438e-926a99e22d08@quicinc.com>
Date: Tue, 15 Aug 2023 00:30:33 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
Vikash Garodia <quic_vgarodia@...cinc.com>,
<stanimir.k.varbanov@...il.com>, <agross@...nel.org>,
<andersson@...nel.org>, <konrad.dybcio@...aro.org>,
<mchehab@...nel.org>, <hans.verkuil@...co.com>,
<linux-kernel@...r.kernel.org>, <linux-media@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH 03/33] iris: vidc: add v4l2 wrapper file
On 8/1/2023 2:53 AM, Krzysztof Kozlowski wrote:
> On 28/07/2023 15:23, Vikash Garodia wrote:
>> Here is the implementation of v4l2 wrapper functions for all
>> v4l2 IOCTLs.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@...cinc.com>
>> ---
>> .../platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h | 77 ++
>> .../platform/qcom/iris/vidc/src/msm_vidc_v4l2.c | 953 +++++++++++++++++++++
>> 2 files changed, 1030 insertions(+)
>> create mode 100644 drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h
>> create mode 100644 drivers/media/platform/qcom/iris/vidc/src/msm_vidc_v4l2.c
>>
>> diff --git a/drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h b/drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h
>> new file mode 100644
>> index 0000000..3766c9d
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/vidc/inc/msm_vidc_v4l2.h
>> @@ -0,0 +1,77 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef _MSM_VIDC_V4L2_H_
>> +#define _MSM_VIDC_V4L2_H_
>> +
>> +#include <linux/fs.h>
>> +#include <linux/poll.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-dev.h>
>> +#include <media/v4l2-ioctl.h>
>> +
>> +int msm_v4l2_open(struct file *filp);
>> +int msm_v4l2_close(struct file *filp);
>> +int msm_v4l2_querycap(struct file *filp, void *fh,
>> + struct v4l2_capability *cap);
>> +int msm_v4l2_enum_fmt(struct file *file, void *fh,
>> + struct v4l2_fmtdesc *f);
>> +int msm_v4l2_try_fmt(struct file *file, void *fh,
>> + struct v4l2_format *f);
>> +int msm_v4l2_s_fmt(struct file *file, void *fh,
>> + struct v4l2_format *f);
>> +int msm_v4l2_g_fmt(struct file *file, void *fh,
>> + struct v4l2_format *f);
>> +int msm_v4l2_s_selection(struct file *file, void *fh,
>> + struct v4l2_selection *s);
>> +int msm_v4l2_g_selection(struct file *file, void *fh,
>> + struct v4l2_selection *s);
>> +int msm_v4l2_s_parm(struct file *file, void *fh,
>> + struct v4l2_streamparm *a);
>> +int msm_v4l2_g_parm(struct file *file, void *fh,
>> + struct v4l2_streamparm *a);
>> +int msm_v4l2_reqbufs(struct file *file, void *fh,
>> + struct v4l2_requestbuffers *b);
>> +int msm_v4l2_querybuf(struct file *file, void *fh,
>> + struct v4l2_buffer *b);
>> +int msm_v4l2_create_bufs(struct file *filp, void *fh,
>> + struct v4l2_create_buffers *b);
>> +int msm_v4l2_prepare_buf(struct file *filp, void *fh,
>> + struct v4l2_buffer *b);
>> +int msm_v4l2_qbuf(struct file *file, void *fh,
>> + struct v4l2_buffer *b);
>> +int msm_v4l2_dqbuf(struct file *file, void *fh,
>> + struct v4l2_buffer *b);
>> +int msm_v4l2_streamon(struct file *file, void *fh,
>> + enum v4l2_buf_type i);
>> +int msm_v4l2_streamoff(struct file *file, void *fh,
>> + enum v4l2_buf_type i);
>> +int msm_v4l2_subscribe_event(struct v4l2_fh *fh,
>> + const struct v4l2_event_subscription *sub);
>> +int msm_v4l2_unsubscribe_event(struct v4l2_fh *fh,
>> + const struct v4l2_event_subscription *sub);
>> +int msm_v4l2_try_decoder_cmd(struct file *file, void *fh,
>> + struct v4l2_decoder_cmd *enc);
>> +int msm_v4l2_decoder_cmd(struct file *file, void *fh,
>> + struct v4l2_decoder_cmd *dec);
>> +int msm_v4l2_try_encoder_cmd(struct file *file, void *fh,
>> + struct v4l2_encoder_cmd *enc);
>> +int msm_v4l2_encoder_cmd(struct file *file, void *fh,
>> + struct v4l2_encoder_cmd *enc);
>> +int msm_v4l2_enum_framesizes(struct file *file, void *fh,
>> + struct v4l2_frmsizeenum *fsize);
>> +int msm_v4l2_enum_frameintervals(struct file *file, void *fh,
>> + struct v4l2_frmivalenum *fival);
>> +int msm_v4l2_queryctrl(struct file *file, void *fh,
>> + struct v4l2_queryctrl *ctrl);
>> +int msm_v4l2_querymenu(struct file *file, void *fh,
>> + struct v4l2_querymenu *qmenu);
>> +unsigned int msm_v4l2_poll(struct file *filp,
>> + struct poll_table_struct *pt);
>> +void msm_v4l2_m2m_device_run(void *priv);
>> +void msm_v4l2_m2m_job_abort(void *priv);
>> +
>> +#endif // _MSM_VIDC_V4L2_H_
>> diff --git a/drivers/media/platform/qcom/iris/vidc/src/msm_vidc_v4l2.c b/drivers/media/platform/qcom/iris/vidc/src/msm_vidc_v4l2.c
>> new file mode 100644
>> index 0000000..6dfb18b
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/vidc/src/msm_vidc_v4l2.c
>> @@ -0,0 +1,953 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include "msm_vidc.h"
>> +#include "msm_vidc_core.h"
>> +#include "msm_vidc_debug.h"
>> +#include "msm_vidc_driver.h"
>> +#include "msm_vidc_inst.h"
>> +#include "msm_vidc_internal.h"
>> +#include "msm_vidc_v4l2.h"
>> +
>> +static struct msm_vidc_inst *get_vidc_inst(struct file *filp, void *fh)
>> +{
>> + if (!filp || !filp->private_data)
>> + return NULL;
>> + return container_of(filp->private_data,
>> + struct msm_vidc_inst, fh);
>> +}
>> +
>> +unsigned int msm_v4l2_poll(struct file *filp, struct poll_table_struct *pt)
>> +{
>> + int poll = 0;
>> + struct msm_vidc_inst *inst = get_vidc_inst(filp, NULL);
>> +
>> + inst = get_inst_ref(g_core, inst);
>> + if (!inst) {
>> + d_vpr_e("%s: invalid instance\n", __func__);
>
> This does not look like Linux coding style. Don't create your own
> abstraction layer over Linux internal API. Use standard Linux functions
> which will behave better and scale along with kernel development.
>
I understand. these custom debug wrappers will be removed in next version.
>> + return POLLERR;
>> + }
>> + if (is_session_error(inst)) {
>> + i_vpr_e(inst, "%s: inst in error state\n", __func__);
>
> i_vpr_e is so obvious for every kernel developer... Please, no.
>
>> + poll = POLLERR;
>> + goto exit;
>> + }
>> +
>> + poll = msm_vidc_poll((void *)inst, filp, pt);
>> + if (poll)
>> + goto exit;
>> +
>> +exit:
>> + put_inst(inst);
>> + return poll;
>> +}
>> +
>> +int msm_v4l2_open(struct file *filp)
>> +{
>> + struct video_device *vdev = video_devdata(filp);
>> + struct msm_video_device *vid_dev =
>> + container_of(vdev, struct msm_video_device, vdev);
>> + struct msm_vidc_core *core = video_drvdata(filp);
>> + struct msm_vidc_inst *inst;
>> +
>> + inst = msm_vidc_open(core, vid_dev->type);
>> + if (!inst) {
>> + d_vpr_e("Failed to create instance, type = %d\n",
>> + vid_dev->type);
>> + return -ENOMEM;
>> + }
>> + filp->private_data = &inst->fh;
>> + return 0;
>> +}
>> +
>> +int msm_v4l2_close(struct file *filp)
>> +{
>> + int rc = 0;
>> + struct msm_vidc_inst *inst;
>> +
>> + inst = get_vidc_inst(filp, NULL);
>> + if (!inst) {
>> + d_vpr_e("%s: invalid instance\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + rc = msm_vidc_close(inst);
>> + filp->private_data = NULL;
>> + return rc;
>> +}
>> +
>> +int msm_v4l2_querycap(struct file *filp, void *fh,
>> + struct v4l2_capability *cap)
>> +{
>> + struct msm_vidc_inst *inst = get_vidc_inst(filp, fh);
>> + int rc = 0;
>> +
>> + inst = get_inst_ref(g_core, inst);
>> + if (!inst || !cap) {
>> + d_vpr_e("%s: invalid instance\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + client_lock(inst, __func__);
>
> ? So we don't know what's this? Mutex? Spinlock? Own reinvented lock?
>
>> + inst_lock(inst, __func__);
>
> Neither this?
>
> No, don't create your own abstractions over standard API.
Sure, will remove these custom wrappers and use standard API directly in
next version.
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists