linux-kernel - Re: [PATCH v3 4/4] media: platform: Add Renesas RZ/G2L CRU driver

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] [day] [month] [year] [list]

Date:   Wed, 26 Oct 2022 18:29:10 +0100
From:   "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Niklas Söderlund <niklas.soderlund@...natech.se>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        Biju Das <biju.das.jz@...renesas.com>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3 4/4] media: platform: Add Renesas RZ/G2L CRU driver

Hi Sakari,

Thank you for the review.

On Tue, Oct 25, 2022 at 11:33 PM Sakari Ailus
<sakari.ailus@...ux.intel.com> wrote:
>
> Hi Prabhakar,
>
> A few comments below... apologies for not reviewing this earlier. Looks
> good in general but there are a few points that need some attention.
>
> On Wed, Oct 05, 2022 at 12:43:43AM +0100, Prabhakar wrote:
> ...
> > +static int rzg2l_cru_ip_pre_streamon(struct v4l2_subdev *sd, u32 flags)
> > +{
> > +     struct rzg2l_cru_dev *cru;
> > +     int ret;
> > +
> > +     cru = v4l2_get_subdevdata(sd);
> > +
> > +     if (!cru->is_csi)
> > +             return -EINVAL;
> > +
> > +     ret = v4l2_subdev_call(cru->ip.remote, video, pre_streamon, 0);
>
> If you're calling pre_streamon successfully, you'll have to have an
> equivalent number of post_streamoff calls.
>
Agreed, I will implement the post_streamoff()

> ...
>
> > +static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
> > +{
> > +     struct media_pipeline *pipe;
> > +     struct v4l2_subdev *sd;
> > +     struct media_pad *pad;
> > +     unsigned long flags;
> > +     int ret;
> > +
> > +     pad = media_pad_remote_pad_first(&cru->pad);
> > +     if (!pad)
> > +             return -EPIPE;
> > +
> > +     sd = media_entity_to_v4l2_subdev(pad->entity);
> > +
> > +     if (!on) {
> > +             media_pipeline_stop(&cru->vdev.entity);
> > +             return v4l2_subdev_call(sd, video, s_stream, 0);
>
> Ditto.
>
OK.

> > +     }
> > +
> > +     ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = v4l2_subdev_call(sd, video, pre_streamon, 0);
> > +     if (ret == -ENOIOCTLCMD)
> > +             ret = 0;
> > +     if (ret)
> > +             return ret;
>
> For all cases below where streaming on doesn't succeed, you'll have to have
> a call of post_streamoff.
>
Agreed, I missed that I will call post_streamoff in the error paths.

> > +
> > +     spin_lock_irqsave(&cru->qlock, flags);
> > +
> > +     /* Select a video input */
> > +     if (cru->is_csi)
> > +             rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
> > +
> > +     /* Cancel the software reset for image processing block */
> > +     rzg2l_cru_write(cru, CRUnRST, CRUnRST_VRESETN);
> > +
> > +     /* Disable and clear the interrupt before using */
> > +     rzg2l_cru_write(cru, CRUnIE, 0);
> > +     rzg2l_cru_write(cru, CRUnINTS, 0x001f000f);
> > +
> > +     /* Initialize the AXI master */
> > +     rzg2l_cru_initialize_axi(cru);
> > +
> > +     /* Initialize image convert */
> > +     ret = rzg2l_cru_initialize_image_conv(cru);
> > +     if (ret) {
> > +             spin_unlock_irqrestore(&cru->qlock, flags);
> > +             return ret;
> > +     }
> > +
> > +     /* Enable interrupt */
> > +     rzg2l_cru_write(cru, CRUnIE, CRUnIE_EFE);
> > +
> > +     /* Enable image processing reception */
> > +     rzg2l_cru_write(cru, ICnEN, ICnEN_ICEN);
> > +
> > +     spin_unlock_irqrestore(&cru->qlock, flags);
> > +
> > +     pipe = sd->entity.pipe ? sd->entity.pipe : &cru->vdev.pipe;
> > +     ret = media_pipeline_start(&cru->vdev.entity, pipe);
> > +     if (ret)
> > +             return ret;
> > +
> > +     clk_disable_unprepare(cru->vclk);
> > +
> > +     ret = v4l2_subdev_call(sd, video, s_stream, 1);
> > +     if (ret == -ENOIOCTLCMD)
> > +             ret = 0;
> > +     if (ret) {
> > +             /* enable back vclk so that release() disables it */
> > +             clk_prepare_enable(cru->vclk);
> > +             media_pipeline_stop(&cru->vdev.entity);
> > +             return ret;
> > +     }
> > +
> > +     ret = clk_prepare_enable(cru->vclk);
>
> What will happen if enabling vclk will fail here? (Or above?)
>
Hmm, I will have to undo the stuff here. I will fix that in the next
version and for the above failure I'll add a warning message as
s_stream(1) itself has failed.

Cheers
Prabhakar