Re: [PATCH net-next v2 8/9] eth: fbnic: report FW uptime in health diagnose

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 15 Sep 2025 18:35:40 -0700 Lee Trager wrote:
> >   Statistics
> >   ----------
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> > index 0e8920685da6..f3f3585c0aac 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> > @@ -487,6 +487,18 @@ static int fbnic_fw_reporter_dump(struct devlink_health_reporter *reporter,
> >   	return err;
> >   }
> >   
> > +static int
> > +fbnic_fw_reporter_diagnose(struct devlink_health_reporter *reporter,
> > +			   struct devlink_fmsg *fmsg,
> > +			   struct netlink_ext_ack *extack)
> > +{
> > +	struct fbnic_dev *fbd = devlink_health_reporter_priv(reporter);
> > +
> > +	devlink_fmsg_u32_pair_put(fmsg, "FW uptime", fbd->firmware_time);  
> 
> I originally added fbd->firmware_time as part of the implementation for 
> logging support in D51521853. The original idea was to correlate 
> firmware logs to host time. This proved to be difficult. Instead I used 
> firmware time to detect firmware crashes in D52065019. Time is to set 0 
> when fbnic_fw_log_write() is called in fbnic_devlink_fw_report() because 
> we don't know the actual time firmware crashed.

I don't see this in my series.

> fbd->firmware_time is 
> only updated with the heartbeat is received. When a crash occurs 
> fbd->firmware_time is reset once firmware comes back up. Ignoring the 
> crash case this should be something like fbd->firmware_time + (jiffies - 
> fbd->last_heartbeat_req) * 1000.

I don't understand what you're getting at, TBH.

> Another issue is your using a u32 for fbd->firmware_time which is u64. 
> Firmware returns its time by calling k_uptime_get()[1] which returns an 
> s64 as its the firmware uptime in milliseconds.

Ah, good point.

> We also don't use firmware time in its raw integer form anywhere in the 
> driver or firmware. Its very hard to read FBNIC_FW_LOG_FMT has the 
> format used in the driver which is based on what Zephyr uses[2].
> 
> IMO this doesn't really have a use case and I would just drop it.

Knowing FW uptime is very useful in multi-host systems.
NIC get into bad state and digging up when the last NIC power cycle
happened from orchestration datasets is a PITA.