Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: Stafford Horne <shorne@gmail.com>
To: Mateusz Holenko <mholenko@antmicro.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	devicetree <devicetree@vger.kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Karol Gugala <kgugala@antmicro.com>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Paul E. McKenney" <paulmck@linux.ibm.com>,
	Filip Kokosinski <fkokosinski@antmicro.com>,
	Pawel Czarnecki <pczarnecki@internships.antmicro.com>,
	Joel Stanley <joel@jms.id.au>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Maxime Ripard <mripard@kernel.org>,
	Shawn Guo <shawnguo@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
	Sam Ravnborg <sam@ravnborg.org>, Icenowy Zheng <icenowy@aosc.io>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"Gabriel L. Somlo" <gsomlo@gmail.com>
Subject: Re: [PATCH v10 3/5] drivers/soc/litex: add LiteX SoC Controller driver
Date: Mon, 14 Sep 2020 22:24:33 +0900
Message-ID: <20200914132433.GB2512402@lianli.shorne-pla.net> (raw)
In-Reply-To: <CAPk366Tvb9g960e3ZLv3+_H8FZJRRe0Jqa4q7tejE+svMcQvLA@mail.gmail.com>

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@gmail.com> wrote:
> >
> > On Wed, Aug 12, 2020 at 02:34:34PM +0200, Mateusz Holenko wrote:
> > > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > >
> > > This commit adds driver for the FPGA-based LiteX SoC
> > > Controller from LiteX SoC builder.
> > >
> > > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.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.

> > > +
> > > +     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.

-Stafford

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-12 12:33 [PATCH v10 0/5] LiteX SoC controller and LiteUART serial driver Mateusz Holenko
2020-08-12 12:34 ` [PATCH v10 1/5] dt-bindings: vendor: add vendor prefix for LiteX Mateusz Holenko
2020-08-12 12:34 ` [PATCH v10 2/5] dt-bindings: soc: document LiteX SoC Controller bindings Mateusz Holenko
2020-08-12 12:34 ` [PATCH v10 3/5] drivers/soc/litex: add LiteX SoC Controller driver Mateusz Holenko
2020-09-11  0:57   ` Stafford Horne
2020-09-14 10:33     ` Mateusz Holenko
2020-09-14 13:24       ` Stafford Horne [this message]
2020-09-15 12:58         ` Mateusz Holenko
2020-08-12 12:34 ` [PATCH v10 4/5] dt-bindings: serial: document LiteUART bindings Mateusz Holenko
2020-08-12 12:35 ` [PATCH v10 5/5] drivers/tty/serial: add LiteUART driver Mateusz Holenko
2020-08-28  8:18   ` Greg Kroah-Hartman
2020-09-11  1:00 ` [PATCH v10 0/5] LiteX SoC controller and LiteUART serial driver Stafford Horne

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200914132433.GB2512402@lianli.shorne-pla.net \
    --to=shorne@gmail.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=fkokosinski@antmicro.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=gsomlo@gmail.com \
    --cc=heiko@sntech.de \
    --cc=icenowy@aosc.io \
    --cc=joel@jms.id.au \
    --cc=jslaby@suse.com \
    --cc=kgugala@antmicro.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab+samsung@kernel.org \
    --cc=mholenko@antmicro.com \
    --cc=mripard@kernel.org \
    --cc=paulmck@linux.ibm.com \
    --cc=pczarnecki@internships.antmicro.com \
    --cc=robh+dt@kernel.org \
    --cc=sam@ravnborg.org \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git