On Thu, Apr 01, 2021 at 01:07:49PM -0500, Pierre-Louis Bossart wrote:
>
> > > > > +#define sdw_dev_dbg_or_err(dev, is_err, fmt, ...) \
> > > > > + do { \
> > > > > + if (is_err) \
> > > > > + dev_err(dev, fmt, __VA_ARGS__); \
> > > > > + else \
> > > > > + dev_dbg(dev, fmt, __VA_ARGS__); \
> > > > > + } while (0)
> > > >
> > > > I see a variant in sof code and now here, why not add in a
> > > > dev_dbg_or_err() and use everywhere?
> > >
> > > Good point, I hesitated back and forth on specific v. generic macro.
> > >
> > > The main reason why I added this macro for SoundWire is that quite a few
> > > subsystems have their own debug functions (DRM, ACPI, etc), and I wasn't
> > > sure if there was any appetite to add more options in
> > > include/linux/dev_printk.h. SOF also uses a different format due to history.
> >
> > It is better if those other subsystems move to using the common kernel
> > debug functions. Historically they were all separate, there is no good
> > reason for them to be that way today.
> >
> > So please do not create custom subsystem debug macros like this just for
> > this tiny set of drivers.
> >
> > My bigger issue with this is that this macro is crazy. Why do you need
> > debugging here at all for this type of thing? That's what ftrace is
> > for, do not sprinkle code with "we got this return value from here!" all
> > over the place like what this does.
>
> We are not sprinkling the code all over the place with any new logs, they
> exist already in the SoundWire code and this patch helps filter them out.
> See e.g. patch 2/2
>
> - dev_err(&slave->dev,
> - "Clk Stop type =%d failed: %d\n", type, ret);
> + sdw_dev_dbg_or_err(&slave->dev, ret != -ENODATA,
> + "Clk Stop mode %d type =%d failed: %d\n",
> + mode, type, ret);
You just added a debug log for no reason.
That's what I was referring to :)
> If you see all my recent patches they were precisely trying to avoid
> polluting the console logs with too much information that is irrelevant from
> most users, and making sure that when a log is provided it's uniquely
> identifiable.
>
> There are similar macros where -EPROBE_DEFER is ignored.
deffered probe is a totally different beast and one that I constantly am
ashamed I accepted into the kernel as the added complexity it has caused
is crazy.
> This addresses a very SoundWire-specific case where if we see a -ENODATA
> error code (Command Ignored), we ignore it and don't report it by default.
> We still have a rare set of cases where this -ENODATA code shows up
> unexpectedly, possibly due to problematic reset sequences, and we want
> developers to help track them down what causes this sequence using dynamic
> debug.
>
> I am not arguing about ftrace v. dynamic debug, and that's also partly why I
> didn't feel comfortable expanding the generic set of debug functions.
Great, then don't add unneeded dev_dbg() lines :)
thanks,
greg k-h
