linux-kernel - Re: [PATCH v10 3/5] drivers/soc/litex: add LiteX SoC Controller 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]
Message-ID: <CAPk366QvUdK1EVpUEVBkgb4me5aMfx6GBWSVNy8OKb8reT0Xvw@mail.gmail.com>
Date:   Tue, 15 Sep 2020 14:58:08 +0200
From:   Mateusz Holenko <mholenko@...micro.com>
To:     Stafford Horne <shorne@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Karol Gugala <kgugala@...micro.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        Filip Kokosinski <fkokosinski@...micro.com>,
        Pawel Czarnecki <pczarnecki@...ernships.antmicro.com>,
        Joel Stanley <joel@....id.au>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Maxime Ripard <mripard@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Sam Ravnborg <sam@...nborg.org>,
        Icenowy Zheng <icenowy@...c.io>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Gabriel L. Somlo" <gsomlo@...il.com>
Subject: Re: [PATCH v10 3/5] drivers/soc/litex: add LiteX SoC Controller driver

On Mon, Sep 14, 2020 at 3:24 PM Stafford Horne <shorne@...il.com> wrote:
>
> On Mon, Sep 14, 2020 at 12:33:11PM +0200, Mateusz Holenko wrote:
> > On Fri, Sep 11, 2020 at 2:57 AM Stafford Horne <shorne@...il.com> wrote:
> > >
> > > On Wed, Aug 12, 2020 at 02:34:34PM +0200, Mateusz Holenko wrote:
> > > > From: Pawel Czarnecki <pczarnecki@...ernships.antmicro.com>
> > > >
> > > > This commit adds driver for the FPGA-based LiteX SoC
> > > > Controller from LiteX SoC builder.
> > > >
> > > > Co-developed-by: Mateusz Holenko <mholenko@...micro.com>
> > > > Signed-off-by: Mateusz Holenko <mholenko@...micro.com>
> > > > Signed-off-by: Pawel Czarnecki <pczarnecki@...ernships.antmicro.com>
> > > > ---
> > > > +     node = dev->of_node;
> > > > +     if (!node)
> > > > +             return -ENODEV;
>
> We return here without BUG() if the setup fails.
>
> > > > +
> > > > +     soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL);
> > > > +     if (!soc_ctrl_dev)
> > > > +             return -ENOMEM;
>
> We return here without BUG() if we are out of memory.
>
> > > > +
> > > > +     soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
> > > > +     if (IS_ERR(soc_ctrl_dev->base))
> > > > +             return PTR_ERR(soc_ctrl_dev->base);
>
> Etc.

You are totally right - this is not consistent.
We should probably either trigger BUG() in each case or don't bother at all.

>
> > > > +
> > > > +     result = litex_check_csr_access(soc_ctrl_dev->base);
> > > > +     if (result) {
> > > > +             // LiteX CSRs access is broken which means that
> > > > +             // none of LiteX drivers will most probably
> > > > +             // operate correctly
> > > The comment format here with // is not usually used in the kernel, but its not
> > > forbidded.  Could you use the /* */ multiline style?
> >
> > Sure, I'll change the commenting style here.
> >
> > >
> > > > +             BUG();
> > > Instead of stopping the system with BUG, could we just do:
> > >
> > >         return litex_check_csr_access(soc_ctrl_dev->base);
> > >
> > > We already have failure for NODEV/NOMEM so might as well not call BUG() here
> > > too.
> >
> > It's true that litex_check_csr_accessors() already generates error
> > codes that could be
> > returned directly.
> > The point of using BUG() macro here, however, is to stop booting the
> > system so that it's visible
> > (and impossible to miss for the user) that an unresolvable HW issue
> > was encountered.
> >
> > CSR-accessors - the litex_{g,s}et_reg() functions - are intended to be
> > used by other LiteX drivers
> > and it's very unlikely that those drivers would work properly after
> > the fail of litex_check_csr_accessors().
> > Since in such case the UART driver will be affected too (no boot logs
> > and error messages visible to the user),
> > I thought it'll be easier to spot and debug the problem if the system
> > stopped in the BUG loop.
> > Perhaps there are other, more linux-friendly, ways of achieving a
> > similar goal - I'm open for suggestions.
>
> I see your point, but I thought if failed with an exit status above, we could do
> the same here.  But I guess failing here means that something is really wrong as
> validation failed.
>
> Some points:
>  - If we return here, the system will still boot but there will be no UART
>  - If we bail with BUG(), here the system stops, and there is no UART
>  - Both cases the user can connect with a debugger and read "dmesg", to see what
>    is wrong, but BUG() does not print an error message on all architectures.
>
> We could also use:
>
>  - WARN(1, "Failed to validate CSR registers, the system is probably broken.");
>
> If you want to keep BUG() it may be fine.
>
> I am not an expert on handling these type of bailout's so other input is
> appreciated.

I don't have a strong opinion about using BUG() here - I just thought
it would be easier for the user.
If this is, however, not how linux typically works, I'm ok with
reworking this part.

> -Stafford

Best,
Mateusz

--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland