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@xxxxxxxxxxxx> wrote: > > Hi, > > On Thu, Sep 16, 2021 at 11:12 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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
