Message-ID: <CA+cxXhmOPUxe=GH7f-BMUVD=FF-0+18ThQc4EthwTY0+epr3Ug@mail.gmail.com>
Date:   Fri, 17 Sep 2021 15:49:04 -0700
From:   Philip Chen <philipchen@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Stephen Boyd <swboyd@...omium.org>,
        LKML <linux-kernel@...r.kernel.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 Doug and Stephen,

Thanks for the review.
Before we reach a consensus on the best logging option, I'll just
remove the printouts from this patch and just return PTR_ERR.
Once we reach a consensus, we can probably improve logging in a separate patch.

On Fri, Sep 17, 2021 at 8:02 AM Doug Anderson <dianders@...omium.org> wrote:
>
> 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