Message-ID: <CAD=FV=WPOvL5xK+nFtgcyH=99=pMFs2eogO0XTg=32Q4QOU+4g@mail.gmail.com>
Date:   Fri, 17 Sep 2021 08:02:36 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Philip Chen <philipchen@...omium.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Jonas Karlman <jonas@...boo.se>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

Hi,

On Thu, Sep 16, 2021 at 11:12 PM Stephen Boyd <swboyd@...omium.org> wrote:
>
> > > > In the case of devm_regmap_init_i2c(), the driver could be fine but
> > > > you might be trying to instantiate it on a system whose i2c bus lacks
> > > > the needed functionality. That's not a bug in the bridge driver but an
> > > > error in system integration. Yeah, after bringup of the new system you
> > > > probably don't need the error, but it will be useful during people's
> > > > bringups year after year.
> > > >
> > >
> > > The point I'm trying to make is that these error messages in probe
> > > almost never get printed after the driver is brought up on the hardware
> > > that starts shipping out to non-kernel developers. Of course they happen
> > > when kernel devs are enabling new hardware year after year on the same
> > > tried and tested driver. They're worthwhile messages to have to make our
> > > lives easier at figuring out some misconfiguration, etc. The problem is
> > > they lead to bloat once the bringup/configuration phase is over.
> > >
> > > At one point we directed driver authors at dev_dbg() for these prints so
> > > that the strings would be removed from the kernel image if debugging
> > > wasn't enabled. It looks like dev_err_probe() goes in the opposite
> > > direction by printing an error message and passing the string to an
> > > exported function, so dev_dbg() won't reduce the image size. Ugh!
> >
> > So maybe the key here is that "CONFIG_PRINTK=n" is not the same as
> > "CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT" and it's not just that one has
> > a more flippant name than the other. I think your argument about the
> > fact that these errors almost never come up in practice is actually
> > true for pretty much _all_ probe errors, isn't it? So if you wanted to
> > keep non-probe errors in your system (keep PRINTK=y) and just do away
> > with these bloat-y probe errors then dev_err_probe() could really be
> > the key and there'd be a big benefit for using for all these errors
> > during probe, not just ones that have a chance of deferring. ...and
> > yes, you could make this config do something fancy like do a stack
> > dump or print the return address if you actually hit one of these
> > errors once you've thrown away the string.
>
> Yes, but it's also just as important to push similar messages, i.e. "I
> failed to get some resource", into the API that hands resources out so
> that bloat is minimized further and drivers are kept simple.

Sure, but this is a slippery slope. If there's any chance that a
caller might want to know about the error but _not_ want the error
message printed then you can't push the error message into the API.
It's really hard to find error cases (even with "get resource" type
functions) where the caller _always_ wants the error reported. Even
kmalloc() has a nod to this with __GFP_NOWARN, though I'm not
advocating adding a "no warn" flag to all APIs. It's always possible
that the caller is expecting some types of errors and handles the case
elegantly.

Let's pop all the way back up to the original point here, which was
about devm_regmap_init_i2c(). What should happen with errors? Let's
look specifically at the errors that could be returned by
regmap_get_i2c_bus() which is the first thing devm_regmap_init_i2c()
tries to do. Those errors have to do with the i2c bus not supporting
the features needed by our regmap.


a) We could return the error without printing anything like the code does today.

No bloat, but during bringup of this bridge chip on a new i2c bus we'd
have to manually add printouts to the probe function to figure out
this error.


b) We could push error reporting into regmap_get_i2c_bus().

No per-driver bloat, but some drivers might have a legitimate reason
not to have an error print here. Perhaps they have a fallback `regmap`
config that they want to be able to use that works with different bus
capabilities. I don't think we can do this.


c) We could use dev_dbg() to print the error

Only bloat if dynamic debug or DEBUG is defined


d) We could use dev_err_probe() to print the error

Extra bloat, though it could be minimized (without sacrificing all
"printk") with a future patch to drop the string from dev_err_probe()
and perhaps replace it with a WARN_ON(). Also handles the fact that
perhaps someday someone might find a reason that regmap_get_i2c_bus()
and/or devm_regmap_init_i2c() should suddenly start returning
-EPROBE_DEFER.


I'm still advocating for "d)" above and I believe you originally
advocated for "a)" or "c)". It's really not such a huge deal, so if
you're adamant about "a)" then I'll shut up. I'm curious if I've
managed to convince you all about "d)" though.

-Doug

Powered by blists - more mailing lists