linux-kernel - Re: [PATCH v9 7/7] media: nuvoton: Add driver for NPCM video capture and encode engine

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADnNmFpjrRy2J5mrFnp6JRiY5W=Xot83PV_JMuBWMP-uf63Rig@mail.gmail.com>
Date:   Thu, 22 Dec 2022 10:23:32 +0800
From:   Kun-Fa Lin <milkfafa@...il.com>
To:     Andrzej Pietrasiewicz <andrzej.p@...labora.com>
Cc:     mchehab@...nel.org, hverkuil-cisco@...all.nl,
        avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com,
        venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        devicetree@...r.kernel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org,
        kwliu@...oton.com, kflin@...oton.com
Subject: Re: [PATCH v9 7/7] media: nuvoton: Add driver for NPCM video capture
 and encode engine

Hi Andrzej,

Thanks for the review.

> > +static void npcm_video_ece_set_fb_addr(struct npcm_video *video, u32 buffer)
>
> static inline void?
>

> > +static void npcm_video_ece_set_enc_dba(struct npcm_video *video, u32 addr)
>
> ditto

> > +static void npcm_video_ece_clear_rect_offset(struct npcm_video *video)
>
> ditto

> > +static int npcm_video_ece_init(struct npcm_video *video)
>
> static inline int? But...
>
> > +{
> > +     npcm_video_ece_ip_reset(video);
> > +     npcm_video_ece_ctrl_reset(video);
> > +
> > +     return 0;
>
> ...the return value is not inspected by the only caller anyway, so why not
>
> static inline void?

OK, I'll declare these functions as static inline void.

> > +static int npcm_video_ece_stop(struct npcm_video *video)
> > +{
> > +     struct regmap *ece = video->ece.regmap;
> > +
> > +     regmap_update_bits(ece, ECE_DDA_CTRL, ECE_DDA_CTRL_ECEEN, 0);
> > +     regmap_update_bits(ece, ECE_DDA_CTRL, ECE_DDA_CTRL_INTEN, 0);
> > +     regmap_update_bits(ece, ECE_HEX_CTRL, ECE_HEX_CTRL_ENCDIS,
> > +                        ECE_HEX_CTRL_ENCDIS);
> > +     npcm_video_ece_clear_rect_offset(video);
> > +
> > +     return 0;
>
> Nobody inspects this return value. Either void, or check the return value
> at call site and handle a non-zero failure.

OK, will change to void.

> > +static unsigned int npcm_video_get_rect_count(struct npcm_video *video)
> > +{
> > +     struct list_head *head, *pos, *nx;
> > +     struct rect_list *tmp;
> > +     unsigned int count;
>
>         unsigned int count = 0;
>
> otherwise if the below condition is not met, ...
> > +
> > +     if (video->list && video->rect) {
> > +             count = video->rect[video->vb_index];
> > +             head = &video->list[video->vb_index];
> > +
> > +             list_for_each_safe(pos, nx, head) {
> > +                     tmp = list_entry(pos, struct rect_list, list);
> > +                     list_del(&tmp->list);
> > +                     kfree(tmp);
> > +             }
>
> why does a function whose name implies merely getting a number actually
> remove all elements from some list? count equals video->rect[video->vb_index];
> and everthing else looks like a(n indented?) side effect. This should be
> somehow commented in the code.
>
> > +     }
> > +
> > +     return count;
>
> ... an undefined number is returned
>
> Which makes me wonder if the compiler is not warning about using a possibly
> uninitialized value.

You are right, this is not the right place to remove the rect_list.
It makes more sense to remove the list right after the associated
video buffer gets dequeued.
I'll do that change.

> > +static int npcm_video_capres(struct npcm_video *video, u32 hor_res,
> > +                          u32 vert_res)
> > +{
> > +     struct regmap *vcd = video->vcd_regmap;
> > +     u32 res, cap_res;
> > +
> > +     if (hor_res > MAX_WIDTH || vert_res > MAX_HEIGHT)
> > +             return -EINVAL;
> > +
> > +     res = FIELD_PREP(VCD_CAP_RES_VERT_RES, vert_res) |
> > +           FIELD_PREP(VCD_CAP_RES_HOR_RES, hor_res);
> > +
> > +     regmap_write(vcd, VCD_CAP_RES, res);
> > +     regmap_read(vcd, VCD_CAP_RES, &cap_res);
> > +
> > +     if (cap_res != res)
> > +             return -EINVAL;
> > +
> > +     return 0;
>
> The return value is not handled by the caller

> > +static int npcm_video_gfx_reset(struct npcm_video *video)
> > +{
> > +     struct regmap *gcr = video->gcr_regmap;
> > +
> > +     regmap_update_bits(gcr, INTCR2, INTCR2_GIRST2, INTCR2_GIRST2);
> > +
> > +     npcm_video_vcd_state_machine_reset(video);
> > +
> > +     regmap_update_bits(gcr, INTCR2, INTCR2_GIRST2, 0);
> > +
> > +     return 0;
>
> Never inspected by callers

> > +static int npcm_video_command(struct npcm_video *video, u32 value)
> > +{
> > +     struct regmap *vcd = video->vcd_regmap;
> > +     u32 cmd;
> > +
> > +     regmap_write(vcd, VCD_STAT, VCD_STAT_CLEAR);
> > +
> > +     regmap_read(vcd, VCD_CMD, &cmd);
> > +     cmd |= FIELD_PREP(VCD_CMD_OPERATION, value);
> > +
> > +     regmap_write(vcd, VCD_CMD, cmd);
> > +     regmap_update_bits(vcd, VCD_CMD, VCD_CMD_GO, VCD_CMD_GO);
> > +     video->op_cmd = value;
> > +
> > +     return 0;
>
> Never inspected by caller

> > +static int npcm_video_start_frame(struct npcm_video *video)
> > +{
>
> One of the callers ignores the return value, but not the other. Why?

These problems will be addressed in the next patch. Thank you.

Regards,
Marvin