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
