Message-ID: <ac41759b-29d1-acfd-7165-96bbac1840c7@intel.com>
Date:   Tue, 14 Feb 2023 17:33:25 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
        <netdev@...r.kernel.org>, <jiri@...dia.com>, <idosch@...sch.org>
Subject: Re: [PATCH net-next 0/5][pull request] add v2 FW logging for ice
 driver



On 2/14/2023 5:16 PM, Jakub Kicinski wrote:
> On Tue, 14 Feb 2023 16:07:04 -0800 Jacob Keller wrote:
>>>> 2b) add some firmware logging specific knobs as a "build on top of
>>>> health reporters" or by creating a separate firmware logging bit that
>>>> ties into a reporter. These knows would be how to set level, etc.  
>>>
>>> Right, the level setting is the part that I'm the least sure of.
>>> That sounds like something more fitting to ethtool dumps.
>>
>> I don't feel like this fits into ethtool at all as its not network
>> specific and tying it to a netdev feels weird.
> 
> Yes, I know, all NICs are generic IO devices now. While the only
> example of what can go wrong we heard so far is a link flap...
> 
> Reimplementing a similar API in devlink with a backward compat
> is definitely an option.
> 

Sure. Well the interface is more of a way for firmware team to get
debugging information out of the firmware. Its sort of like a "print"
debugging, where information about the state of firmware during
different activities can be recorded.

The idea is that when a problem is detected by a user, they can enable
firmware logging to capture this data and then that can aid us in
determining what really went wrong.

It isn't a "we detected a problem" interface. It's a "here's a bunch of
debugging logging you asked for!" interface.

>> I believe when a firmware dev adds a log message they choose an
>> appropriate section and log level for when it should be reported.
>>
>> This makes me think the right approach is to add a new "devlink fwlog"
>> section entirely where we can define its semantics. It doesn't quite
>> line up with the current intention of health reporters.
>>
>> We also considered some sort of extension to devlink regions, where each
>> new batch of messages from firmware would be a new snapshot.
>>
>> Again this still requires some form of controls for whether to enable
>> logging, how many snapshots to store, how to discard old snapshots if we
>> run out of space, and what modules and log levels to enable.
> 
> Yeah, it doesn't fit into health or regions if there's no signal on 
> when things go wrong. If ethtool set_dump / get_dump doesn't fit a new
> command may be better.

I am not sure how ETHTOOL_SET_DUMP works for configuring the settings of
what modules to enable or what log level. I guess it has "@flag" which
would be the settings. This is u32 which might not be enough information
to specify the log level and the module config. It also doesn't expose
those values to allow users to actually understand what they're
configuring on or off.

It does look like it would work for this patch set via the following flow:

ETHTOOL_SET_DUMP -> enable dumping with the appropriate flags (driver
defined...)

Once activated, the driver would then capture and store data as we do
now, and user space could retrieve it via ETHTOOL_GET_DUMP_FLAG and
ETHTOOL_GET_DUMP_DATA

But this doesn't really help solve the problem of how to expose the
module and log levels. Are we accepting just a binary flags that are
driver specific?

Powered by blists - more mailing lists