[RFG] sfc: nvlog and devlink health

[RFG] sfc: nvlog and devlink health

[Edward Cree]

Solarflare NICs have a flash partition to which the MCPU logs various
 errors, warnings, and other diagnostic info.  We want to expose this
 'nvlog' data, and the best fit we've found so far is devlink health.
Reading it is simple enough — plan is to have a reporter whose diagnose
 method reads the partition and returns the contents (could potentially
 use dump instead but the extra layer of triggering and saving seems
 unnecessary).
The problem is how to clear it (since it fills up after comparatively
 few boots, so when debugging field issues you'll usually need to clear
 it first and then reproduce the issue).
 DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR is no use here, because it only
 clears the kernel-saved copy; it doesn't call any driver method.
The code we've developed internally, that I'm now preparing to submit
 upstream, handles this by having *two* reporters, 'nvlog' and
 'nvlog-clear'; both read the flash in their diagnose method but
 nvlog-clear additionally clears it afterwards.  It works, but it
 doesn't feel very clean.
Is this approach acceptable?  Is there a better way?

-ed

Re: [RFG] sfc: nvlog and devlink health

[Jiri Pirko]

Tue, Apr 15, 2025 at 04:51:39PM +0200, ecree.xilinx@gmail.com wrote:
>Solarflare NICs have a flash partition to which the MCPU logs various
> errors, warnings, and other diagnostic info.  We want to expose this
> 'nvlog' data, and the best fit we've found so far is devlink health.
>Reading it is simple enough — plan is to have a reporter whose diagnose
> method reads the partition and returns the contents (could potentially
> use dump instead but the extra layer of triggering and saving seems
> unnecessary).
>The problem is how to clear it (since it fills up after comparatively
> few boots, so when debugging field issues you'll usually need to clear
> it first and then reproduce the issue).
> DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR is no use here, because it only
> clears the kernel-saved copy; it doesn't call any driver method.

Can't it be extended to actually call an optional driver method?
That would sound fine to me and will solve your problem.


>The code we've developed internally, that I'm now preparing to submit
> upstream, handles this by having *two* reporters, 'nvlog' and
> 'nvlog-clear'; both read the flash in their diagnose method but
> nvlog-clear additionally clears it afterwards.  It works, but it
> doesn't feel very clean.
>Is this approach acceptable?  Is there a better way?
>
>-ed
>

Re: [RFG] sfc: nvlog and devlink health

[Edward Cree]

On 15/04/2025 17:41, Jiri Pirko wrote:
> Tue, Apr 15, 2025 at 04:51:39PM +0200, ecree.xilinx@gmail.com wrote:
>> DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR is no use here, because it only
>> clears the kernel-saved copy; it doesn't call any driver method.
> 
> Can't it be extended to actually call an optional driver method?
> That would sound fine to me and will solve your problem.

Would that be "diagnose"/"dump clear" or "dump"/"dump clear"?
The former is weird, are you sure it's not a misuse of the API to
 have "dump clear" clear something that's not a dump?  I feel like
 extending the devlink core to support a semantic mismatch /
 layering violation might raise a few eyebrows.
The latter just doesn't work as (afaict) calling dump twice
 without an intervening clear won't get updated output, and users
 might want to read again without erasing.

Re: [RFG] sfc: nvlog and devlink health

[Jiri Pirko]

Wed, Apr 16, 2025 at 12:24:07PM +0200, ecree.xilinx@gmail.com wrote:
>On 15/04/2025 17:41, Jiri Pirko wrote:
>> Tue, Apr 15, 2025 at 04:51:39PM +0200, ecree.xilinx@gmail.com wrote:
>>> DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR is no use here, because it only
>>> clears the kernel-saved copy; it doesn't call any driver method.
>> 
>> Can't it be extended to actually call an optional driver method?
>> That would sound fine to me and will solve your problem.
>
>Would that be "diagnose"/"dump clear" or "dump"/"dump clear"?
>The former is weird, are you sure it's not a misuse of the API to
> have "dump clear" clear something that's not a dump?  I feel like
> extending the devlink core to support a semantic mismatch /
> layering violation might raise a few eyebrows.

I probably misunderstood. I thought you wrote that it is the dump that
is actually cleared.


>The latter just doesn't work as (afaict) calling dump twice
> without an intervening clear won't get updated output, and users
> might want to read again without erasing.

Re: [RFG] sfc: nvlog and devlink health

[Przemek Kitszel]

On 4/16/25 12:24, Edward Cree wrote:
> On 15/04/2025 17:41, Jiri Pirko wrote:
>> Tue, Apr 15, 2025 at 04:51:39PM +0200, ecree.xilinx@gmail.com wrote:
>>> DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR is no use here, because it only
>>> clears the kernel-saved copy; it doesn't call any driver method.
>>
>> Can't it be extended to actually call an optional driver method?
>> That would sound fine to me and will solve your problem.
> 
> Would that be "diagnose"/"dump clear" or "dump"/"dump clear"?
> The former is weird, are you sure it's not a misuse of the API to
>   have "dump clear" clear something that's not a dump?  I feel like
>   extending the devlink core to support a semantic mismatch /
>   layering violation might raise a few eyebrows.
> The latter just doesn't work as (afaict) calling dump twice
>   without an intervening clear won't get updated output, and users
>   might want to read again without erasing.
> 

I guess it is common for HW/FW to have a buffer for errors/events/logs
that could either be cyclical or just stop data collection when full.

We have similar thing for fw health reporter in ice driver (E810),
we simply collect/display also the data from before the driver even
probed (seems valuable).

So, when to clean?
a) clearing the FW log at the point of user-triggered "clear" command,
will easily open up a window to loose events coming after the snapshot
was taken (likely minor issue, as it is typical to care the most about
(some of) the first events only);
b) let the driver to clean (send the "clean" command to FW) on the event
and optionally do the same for .probe
c) newer firmware could implement auto-clean-on-send,
making the b) above a no-op there (but it would be a good fallback for
old fw)

just requiring more actions from user seems too much for the problem,
that seems to be solve-able by the driver