Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 12 Apr 2021 16:35:35 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>
Cc:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "luca@coelho.fi" <luca@coelho.fi>,
        "Beker, Ayala" <ayala.beker@intel.com>,
        "Coelho, Luciano" <luciano.coelho@intel.com>
Subject: Re: [PATCH RESEND 2/3] iwlwifi: mei: add the driver to allow
 cooperation with CSME
Message-ID: <YHRat4egd/LM8DNJ@kroah.com>
References: <20210412124328.24472-1-emmanuel.grumbach@intel.com>
 <20210412124328.24472-2-emmanuel.grumbach@intel.com>
 <YHRFy3aq/gB7Vde6@kroah.com>
 <SA0PR11MB475215678ADCCE657C6574D6F2709@SA0PR11MB4752.namprd11.prod.outlook.com>
 <YHRWtJqbuFGmR2Sw@kroah.com>
 <SA0PR11MB475228709B9B0B6D74CE07A2F2709@SA0PR11MB4752.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SA0PR11MB475228709B9B0B6D74CE07A2F2709@SA0PR11MB4752.namprd11.prod.outlook.com>
Precedence: bulk

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'll print an error then. I still didn't understand what's the difference between BUG_ON and WARN_ON in your eyes.

Neither should be used in new code unless the kernel is so messed up
that the only way out is to reboot the machine, as both of them cause
that to happen.

thanks,

greg k-h