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 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.

> > > +	if (WARN_ON(rd > q_sz || wr > q_sz))
> > > +		return -EINVAL;
> > 
> > If any of the WARN_ON() things in this driver can ever trigger, just handle
> > them normally and do NOT call WARN_ON() as you just rebooted the box for
> > a simple thing that you could have recovered from.
> 
> My understanding is that WARN_ON does not reboot the box which is why it was invented when we already had BUG_ON. This can't be triggered by the user space, but can be triggered by the CSME firmware that runs on the chipset.

when panic_on_warn is enabled, the box will reboot.

If firmware can cause this, then properly handle the issue and continue
on, don't reboot machines.

> > Some of these WARN_ON() in the code feel very lazy as you control the
> > caller so you "know" that this will never happen.  So don't leave them in, it's
> > debugging code that you can now remove.
> 
> I can transform them in error prints, but again, my understanding is / was that WARN_ON is ok to use and won't reboot the box since it differs from BUG_ON.

Again, panic_on_warn.

> > > +
> > > +	IWL_MEI_DEBUG(cldev, "Got CSME filters\n");
> > 
> > ftrace is your friend, remove these pointless "the code got here!"
> > lines.  You have loads of them...
> 
> This is debug. Won't appear unless you want it.
> I have extensive tracing support.

Again, use ftrace for tracing, don't use debug print messages.

> > > +static void iwl_mei_handle_rx_host_own_req(struct mei_cl_device
> > *cldev,
> > > +					   const struct iwl_sap_msg_dw *dw)
> > {
> > > +	struct iwl_mei *mei = mei_cldev_get_drvdata(cldev);
> > > +
> > > +	IWL_MEI_DEBUG(cldev, "Got ownership req response: %d\n", dw-
> > >val);
> > 
> > Code got here!  :(
> 
> Same, this is protected by a dynamic debug knob.
> I can see tons of those in mei bus driver .
> $ git grep _dbg\( -- drivers/misc/mei | wc -l
> 228

mei should be fixed as well.

> > > +
> > > +	if (!dw->val) {
> > > +		IWL_MEI_INFO(cldev, "Ownership req denied\n");
> > 
> > why?????
> > 
> > > +		return;
> > 
> > No error returned?
> 
> This is not an error. This means that the CSME firmware is not allowing the host to use the WLAN device.

Ok, and what can a user do about this?

> > > +	if (!netdev) {
> > > +		IWL_MEI_INFO(cldev, "No netdevice, dropping the Tx
> > packet\n");
> > 
> > quiet!!!
> 
> This is actually not a usual path. A race is happening here...

"race" implies "error" which is not "dev_info()" material.

> > Or at the least, make this an error so that a user can handle it properly.  They
> > can do something about this, right?  If not, why did you just tell them this
> > happened?
> 
> There isn't much I can do here. The CSME firmware is sending a packet
> to iwlmei to send on the WLAN link, but the link isn't valid anymore.

Then report an error.

> > > +		if (le16_to_cpu(hdr.type) != SAP_MSG_DATA_PACKET) {
> > > +			IWL_MEI_INFO(cldev, "Unsupported message: type
> > %d, len %d\n",
> > > +				     le16_to_cpu(hdr.type), len);
> > 
> > So userspace can spam the kernel log?
> 
> This is not userspace, this is the CSME firmware.

Then make it an error, not an informational message.  Someone needs to
see it to fix the firmware, right?

> > Please revisit _ALL_ of your messages you are printing out here, it feels way
> > way way too noisy, like you got the code up and working with the debug lines
> > in it and forgot to remove it.
> 
> [   12.665966] iwlmei 0000:00:16.0-13280904-7792-4fcb-a1aa-5e70cbb1e865: Got a valid SAP_ME_MSG_START_OK from CSME firmware
> 
> That's the only line I am printing by default. I can remove it. I can't do better than 0 printing.

0 printing would be good.

> > > +}
> > > +
> > > +#else
> > > +
> > > +static void iwl_mei_dbgfs_register(struct iwl_mei *mei) {} static
> > > +void iwl_mei_dbgfs_unregister(struct iwl_mei *mei) {}
> > 
> > This type of thing goes in a .h file, you know better.
> 
> A header file only for less a handful of functions?

You already have .h files for this code, why not put it in there?

thanks,

greg k-h