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

[Eric Joyner <eric.joyner@amd.com>]

On 5/14/2026 6:27 PM, Jakub Kicinski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.

Man, it's been forever since I've been able to get back to this.

> ---
> [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.

Sure.

> 
> 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);
>>  }

I can use netdev_err_once() here, instead? There's no "netdev_err_ratelimited()"
like how there is for struct device and "dev_err_ratelimited()", interestingly.
Would that be more appropriate in the future?

There's also a similar issue in ionic_get_link_ksettings() below (and maybe
elsewhere?), but that can be fixed in a separate patch.

> 
> [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?

Is this a big enough issue to deal with? Is link really going to go down
sixty-five thousand times while the driver is loaded? I guess we could have a
running tally, but that seems like more effort than this is worth.

> 
> [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?

This count doesn't get reset in the firmware when the device the ionic driver
binds to gets reset; the freshly zeroed page will get the count from the
firmware again soon after the reset occurs.

> 
>> 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