Re: [PATCH 1/2] soundwire: add macro to selectively change error levels

[Greg KH <gregkh@linuxfoundation.org>]

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