Re: [PATCH net-next v3 2/5] ionic: Get "link_down_count" ext link stat from firmware

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: Get "link_down_count" ext link stat from firmware

This patch replaces the driver-side link_down_count counter on
struct ionic_lif with a value read directly from the firmware's
DMA-updated struct ionic_port_status.link_down_count, exposed via
ethtool_link_ext_stats.link_down_events in ionic_get_link_ext_stats().

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> index 78a802eb159f..cc2b294910f6 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> @@ -115,9 +115,16 @@ static void ionic_get_link_ext_stats(struct net_device *netdev,
>  				     struct ethtool_link_ext_stats *stats)
>  {
>  	struct ionic_lif *lif = netdev_priv(netdev);
> +	struct ionic *ionic = lif->ionic;
> +
> +	if (!ionic->idev.port_info) {
> +		netdev_err(netdev, "port_info not initialized\n");
> +		return;
> +	}

[Low]
Should this NULL check sit inside the is_physfn branch below? The
port_info pointer is only dereferenced on the PF path, so a VF calling
ethtool on link ext stats will now log a netdev_err even though the VF
path never reads port_info.

The message is also unrate-limited and reachable from userspace via
ethtool. port_info is freed and set to NULL in ionic_port_reset() and
reallocated in ionic_port_init() on FW reset / FLR / PCI error recovery
paths (ionic_reset_prepare()/ionic_reset_done() in ionic_bus_pci.c, and
ionic_restart_lif() in ionic_lif.c), so any ethtool query during that
transient window will emit a kernel log line where the previous code
silently did nothing.

>  
> -	if (lif->ionic->pdev->is_physfn)
> -		stats->link_down_events = lif->link_down_count;
> +	if (ionic->pdev->is_physfn)
> +		stats->link_down_events =
> +		    le16_to_cpu(ionic->idev.port_info->status.link_down_count);
>  }

[Medium]
The firmware-supplied link_down_count is a __le16 in struct
ionic_port_status (ionic_if.h), so the value saturates at 65,535. The
previous lif->link_down_count was an unsigned int (32-bit), and the
ethtool field stats->link_down_events is u64 and treated as monotonic
by userspace consumers (per the comment on struct
ethtool_link_ext_stats in include/linux/ethtool.h).

Without driver-side rollover detection that accumulates the carry into
a wider counter, after 65,536 link bounces the reported value will
appear to go backward to a small number. Should the driver detect a
firmware value smaller than the previously-read value and fold the
delta into a 32/64-bit running tally before assigning to
stats->link_down_events?

[Medium]
Does this change make link_down_events non-persistent across firmware
resets? port_info lives in DMA-coherent memory that
ionic_port_reset() frees and clears:

drivers/net/ethernet/pensando/ionic/ionic_main.c:ionic_port_reset() {
    ...
    dma_free_coherent(...);
    idev->port_info = NULL;
    ...
}

and ionic_port_init() reallocates a fresh zeroed page. These are
called from the FLR / PCI error recovery path
(ionic_reset_prepare()/ionic_reset_done() in ionic_bus_pci.c) and from
ionic_restart_lif() in ionic_lif.c, so FW-reset events that previously
left the driver-resident counter untouched will now zero this stat.

The commit message does not mention this behavioral change. Should the
driver snapshot the firmware value before each reset and accumulate it
across reset boundaries to keep the ethtool counter monotonic?

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 637e635bbf03..eb7e552bc12e 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -179,7 +179,6 @@ static void ionic_link_status_check(struct ionic_lif *lif)
>  		}
>  	} else {
>  		if (netif_carrier_ok(netdev)) {
> -			lif->link_down_count++;
>  			netdev_info(netdev, "Link down\n");
>  			netif_carrier_off(netdev);
>  		}
-- 
pw-bot: cr