>
> 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.
> > > > + 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.
I'll print an error then. I still didn't understand what's the difference between BUG_ON and WARN_ON in your eyes.
>
> > > 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?
Not much, but a kernel developer would like to know that it happened.
>
> > > > + 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?
Ok.
>
> > > 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
