Re: [PATCH RESEND 2/3] iwlwifi: mei: add the driver to allow cooperation with CSME

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Apr 12, 2021 at 02:46:53PM +0000, Grumbach, Emmanuel wrote:
> > On Mon, Apr 12, 2021 at 02:29:45PM +0000, Grumbach, Emmanuel wrote:
> > > >
> > > > On Mon, Apr 12, 2021 at 01:44:58PM +0000, Grumbach, Emmanuel wrote:
> > > > > > > +#define IWL_MEI_DEBUG(c, f, a...)		\
> > > > > > > +	do {					\
> > > > > > > +		CHECK_FOR_NEWLINE(f);		\
> > > > > >
> > > > > > Huh?
> > > > > >
> > > > > > > +		dev_dbg(&(c)->dev, f, ## a);	\
> > > > > >
> > > > > > Just use dev_dbg(), don't be special for a single driver, it
> > > > > > hurts when trying to read different drivers.
> > > > >
> > > > > I took this from iwlwifi. I can change if needed, not a big deal.
> > > >
> > > > Please do.
> > > >
> > > > > > > +module_param_named(defer_start_message,
> > defer_start_message,
> > > > > > bool,
> > > > > > > +0644); MODULE_PARM_DESC(defer_start_message,
> > > > > > > +		 "Defer the start message Tx to CSME (default
> > false)");
> > > > > >
> > > > > > Why do you need this?  Who is going to set it to anything else,
> > > > > > and why would they?  This isn't the 1990's anymore, please do
> > > > > > not add new module parameters.
> > > > >
> > > > > For testing. I need this to be able to force a certain order of
> > > > > initialization
> > > > which is possible (and hence must be tested) but not likely to happen.
> > > > > Another point is tracing. This allows me to load the module but
> > > > > prevent any
> > > > real operation. Then, start tracing. This way, I can see the whole
> > > > flow in tracing, even the very beginning.
> > > >
> > > > Then call this something obvious,
> > > >
> > "kernel_hacker_debuging_testing_only_use_if_you_know_what_you_are_
> > > > doing".
> > > >
> > > > Or better yet, just put it in debugfs to turn it on/off and no
> > > > module parameter is needed at all.
> > > >
> > >
> > > Debugfs is not a replacement for module parameters. Debugfs can be
> > > used only after the driver already ran quite a bit of its
> > > initialization code path. Here I want to be able to catch the very
> > > first messages with tracing.
> > 
> > Then use the proper trace functionality of the kernel, which is not module
> > parameters :(
> 
> I am sorry if I drive you nuts but I don't know any "proper trace
> functionality" in the kernel that the user could enable / disable and
> that would be available immediately at init.

The in-kernel trace facilities do not work for boot code?
Documentation/tracing/boottime-trace.rst seems to disagree :)

> The user needs to
> "activate" the trace points using trace-cmd or whatever other tool. By
> the time the user does so, the driver has already run the code path I
> wish to debug. I can use debug prints, but you didn't seem happy about
> it. So I am happy to use tracing, but then we need to make sure it
> cover all the cases. The way I make it cover all the cases is with
> this module parameter. If you know a better way, I'll be happy to use
> it.

See the above document, there's nothing "special" about a single kernel
driver that should warrant a one-off user/kernel api like this.

thanks,

greg k-h