Message-ID: <20230214171643.10f1590f@kernel.org>
Date:   Tue, 14 Feb 2023 17:16:43 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
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 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.

> >> 3) for ice, once the health reporter is enabled we request the firmware
> >> to send us logging, then we get our admin queue message and simply copy
> >> this into the health reporter as a new event
> >>
> >> 4) user space is in charge of monitoring health reports and can decide
> >> how to copy events out to disk and when to delete the health reports
> >> from the kernel.  
> > 
> > That's also out of what's expected with health reporters. User should
> > not have to run vendor tools with devlink health. Decoding of the dump
> > may require vendor tools but checking if system is healthy or something
> > crashed should happen without any user space involvement.
> 
> So this wasn't about using a specific "vendor" tool, but more that
> devlink health can decide when to delete a given dump?
> 
> Ultimately we have to take the binary data and give it to a vendor
> specific tool to decode (whether I like that or not...). The information
> required to decode the messages is not something we have permission to
> share and code into the driver.
> 
> > I bet all vendors at this point have separate modules in the FW.
> > It's been the case for a while, that's why we have multiple versions
> > supported in devlink dev info.  
> 
> So one key here is that module for us refers to various sub-components
> of our main firmware, and does not tie into the devlink info modules at
> all, nor would that even make sense to us.
> 
> Its more like sections i.e.
> 
> DCB,
> MDIO,
> NVM,
> Scheduler,
> Tx queue management,
> SyncE,
> LLDP,
> Link Management,
> ...
> 
> 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.

Powered by blists - more mailing lists