| 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: <d6459b89-1bb2-4906-bba7-d0ac549ccfff@linaro.org>
Date: Wed, 13 Nov 2024 02:33:47 +0200
From: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
To: Vikram Sharma <quic_vikramsa@...cinc.com>, rfoss@...nel.org,
todor.too@...il.com, bryan.odonoghue@...aro.org, krzk+dt@...nel.org
Cc: linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH v2 1/1] media: qcom: camss: Restructure
camss_link_entities
Hello Vikram.
On 11/12/24 15:38, Vikram Sharma wrote:
> Refactor the camss_link_entities function by breaking it down into
> three distinct functions. Each function will handle the linking of
> a specific entity separately, enhancing readability.
>
> Signed-off-by: Suresh Vankadara <quic_svankada@...cinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@...cinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@...cinc.com>
> ---
> drivers/media/platform/qcom/camss/camss-vfe.c | 6 +-
> drivers/media/platform/qcom/camss/camss.c | 196 ++++++++++++------
> drivers/media/platform/qcom/camss/camss.h | 4 +
> 3 files changed, 138 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 83c5a36d071f..446604cc7ef6 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -1794,9 +1794,9 @@ int msm_vfe_register_entities(struct vfe_device *vfe,
> &video_out->vdev.entity, 0,
> MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> if (ret < 0) {
> - dev_err(dev, "Failed to link %s->%s entities: %d\n",
> - sd->entity.name, video_out->vdev.entity.name,
> - ret);
> + camss_link_err(vfe->camss, sd->entity.name,
> + video_out->vdev.entity.name,
> + ret);
As Bryan said, the change above is unrelated to the restructuring.
See also my next comment.
> goto error_link;
> }
> }
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index fabe034081ed..980cb1e337be 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1840,14 +1840,83 @@ static int camss_init_subdevices(struct camss *camss)
> }
>
> /*
> - * camss_link_entities - Register subdev nodes and create links
> + * camss_link_err - print error in case link creation fails
> + * @src_name: name for source of the link
> + * @sink_name: name for sink of the link
> + */
> +inline void camss_link_err(struct camss *camss,
> + const char *src_name,
> + const char *sink_name,
> + int ret)
> +{
> + if (!camss || !src_name || !sink_name)
> + return;
> + dev_err(camss->dev,
> + "Failed to link %s->%s entities: %d\n",
> + src_name,
> + sink_name,
> + ret);
> +}
I believe this new function is simply not needed. It is not helpful.
> +/*
> + * camss_link_entities_csid - Register subdev nodes and create links
> * @camss: CAMSS device
> *
> * Return 0 on success or a negative error code on failure
> */
> -static int camss_link_entities(struct camss *camss)
> +static int camss_link_entities_csid(struct camss *camss)
> {
> - int i, j, k;
> + struct media_entity *src_entity;
> + struct media_entity *sink_entity;
> + int ret, line_num;
> + u16 sink_pad;
> + u16 src_pad;
> + int i, j;
> +
> + for (i = 0; i < camss->res->csid_num; i++) {
> + if (camss->ispif)
> + line_num = camss->ispif->line_num;
> + else
> + line_num = camss->vfe[i].res->line_num;
> +
> + src_entity = &camss->csid[i].subdev.entity;
> + for (j = 0; j < line_num; j++) {
> + if (camss->ispif) {
> + sink_entity = &camss->ispif->line[j].subdev.entity;
> + src_pad = MSM_CSID_PAD_SRC;
> + sink_pad = MSM_ISPIF_PAD_SINK;
> + } else {
> + sink_entity = &camss->vfe[i].line[j].subdev.entity;
> + src_pad = MSM_CSID_PAD_FIRST_SRC + j;
> + sink_pad = MSM_VFE_PAD_SINK;
> + }
So, you split one solid function, which covers csid->ispif and ispif->vfe
into two separate functions, the logic of "if (camss->ispif)" is applied
twice in two different functions, while before the change it was done just
once, then why does it enhance readability? I think it's just the opposite...
> +
> + ret = media_create_pad_link(src_entity,
> + src_pad,
> + sink_entity,
> + sink_pad,
> + 0);
> + if (ret < 0) {
> + camss_link_err(camss, src_entity->name,
> + sink_entity->name,
> + ret);
> + return ret;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * camss_link_entities_csiphy - Register subdev nodes and create links
> + * @camss: CAMSS device
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int camss_link_entities_csiphy(struct camss *camss)
> +{
> + int i, j;
> int ret;
>
> for (i = 0; i < camss->res->csiphy_num; i++) {
> @@ -1858,81 +1927,77 @@ static int camss_link_entities(struct camss *camss)
> MSM_CSID_PAD_SINK,
> 0);
> if (ret < 0) {
> - dev_err(camss->dev,
> - "Failed to link %s->%s entities: %d\n",
> - camss->csiphy[i].subdev.entity.name,
> - camss->csid[j].subdev.entity.name,
> - ret);
> + camss_link_err(camss,
> + camss->csiphy[i].subdev.entity.name,
> + camss->csid[j].subdev.entity.name,
> + ret);
> return ret;
> }
> }
> }
>
> - if (camss->ispif) {
> - for (i = 0; i < camss->res->csid_num; i++) {
> - for (j = 0; j < camss->ispif->line_num; j++) {
> - ret = media_create_pad_link(&camss->csid[i].subdev.entity,
> - MSM_CSID_PAD_SRC,
> - &camss->ispif->line[j].subdev.entity,
> - MSM_ISPIF_PAD_SINK,
> + return 0;
> +}
> +
> +/*
> + * camss_link_entities_ispif - Register subdev nodes and create links
> + * @camss: CAMSS device
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int camss_link_entities_ispif(struct camss *camss)
> +{
> + int i, j, k;
> + int ret;
> +
> + for (i = 0; i < camss->ispif->line_num; i++) {
> + for (k = 0; k < camss->res->vfe_num; k++) {
> + for (j = 0; j < camss->vfe[k].res->line_num; j++) {
> + struct v4l2_subdev *ispif = &camss->ispif->line[i].subdev;
> + struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
> +
> + ret = media_create_pad_link(&ispif->entity,
> + MSM_ISPIF_PAD_SRC,
> + &vfe->entity,
> + MSM_VFE_PAD_SINK,
> 0);
> if (ret < 0) {
> - dev_err(camss->dev,
> - "Failed to link %s->%s entities: %d\n",
> - camss->csid[i].subdev.entity.name,
> - camss->ispif->line[j].subdev.entity.name,
> - ret);
> + camss_link_err(camss, ispif->entity.name,
> + vfe->entity.name,
> + ret);
> return ret;
> }
> }
> }
> -
> - for (i = 0; i < camss->ispif->line_num; i++)
> - for (k = 0; k < camss->res->vfe_num; k++)
> - for (j = 0; j < camss->vfe[k].res->line_num; j++) {
> - struct v4l2_subdev *ispif = &camss->ispif->line[i].subdev;
> - struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
> -
> - ret = media_create_pad_link(&ispif->entity,
> - MSM_ISPIF_PAD_SRC,
> - &vfe->entity,
> - MSM_VFE_PAD_SINK,
> - 0);
> - if (ret < 0) {
> - dev_err(camss->dev,
> - "Failed to link %s->%s entities: %d\n",
> - ispif->entity.name,
> - vfe->entity.name,
> - ret);
> - return ret;
> - }
> - }
> - } else {
> - for (i = 0; i < camss->res->csid_num; i++)
> - for (k = 0; k < camss->res->vfe_num; k++)
> - for (j = 0; j < camss->vfe[k].res->line_num; j++) {
> - struct v4l2_subdev *csid = &camss->csid[i].subdev;
> - struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
> -
> - ret = media_create_pad_link(&csid->entity,
> - MSM_CSID_PAD_FIRST_SRC + j,
> - &vfe->entity,
> - MSM_VFE_PAD_SINK,
> - 0);
> - if (ret < 0) {
> - dev_err(camss->dev,
> - "Failed to link %s->%s entities: %d\n",
> - csid->entity.name,
> - vfe->entity.name,
> - ret);
> - return ret;
> - }
> - }
> }
>
> return 0;
> }
>
> +/*
> + * camss_link_entities - Register subdev nodes and create links
> + * @camss: CAMSS device
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int camss_link_entities(struct camss *camss)
> +{
> + int ret;
> +
> + ret = camss_link_entities_csiphy(camss);
> + if (ret < 0)
> + return ret;
> +
> + ret = camss_link_entities_csid(camss);
> + if (ret < 0)
> + return ret;
> +
> + if (camss->ispif)
> + ret = camss_link_entities_ispif(camss);
Since there is an expectation that the change is non-functional, please
keep the logic unmodified.
if (camss->ispif) {
ret = camss_link_entities_ispif(camss);
} else {
}
> + return ret;
> +}
> +
> /*
> * camss_register_entities - Register subdev nodes and create links
> * @camss: CAMSS device
> @@ -2073,9 +2138,10 @@ static int camss_subdev_notifier_complete(struct v4l2_async_notifier *async)
> input, MSM_CSIPHY_PAD_SINK,
> MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> if (ret < 0) {
> - dev_err(camss->dev,
> - "Failed to link %s->%s entities: %d\n",
> - sensor->name, input->name, ret);
> + camss_link_err(camss,
> + sensor->name,
> + input->name,
> + ret);
> return ret;
> }
> }
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 0ce84fcbbd25..2086000ad5c1 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -160,5 +160,9 @@ void camss_pm_domain_off(struct camss *camss, int id);
> int camss_vfe_get(struct camss *camss, int id);
> void camss_vfe_put(struct camss *camss, int id);
> void camss_delete(struct camss *camss);
> +void camss_link_err(struct camss *camss,
> + const char *src_name,
> + const char *sink_name,
> + int ret);
Please don't add this into the header file. Drop it.
> #endif /* QC_MSM_CAMSS_H */
In fact I didn't find the change as a simplification, because now there are
more if-branches apparently. Is there a reason why the change is needed?
--
Best wishes,
Vladimir
Powered by blists - more mailing lists