netdev - Re: [Intel-wired-lan] [PATCH iwl-next 1/2] ice: add link_down

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4012b88a-091d-4f81-92ab-ad32727914ff@linux.intel.com>
Date: Mon, 14 Apr 2025 15:12:32 +0200
From: "Szapar-Mudlaw, Martyna" <martyna.szapar-mudlaw@...ux.intel.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next 1/2] ice: add link_down_events
 statistic



On 4/9/2025 2:08 PM, Paul Menzel wrote:
> Dear Martyna,
> 
> 
> Thank you for your patch.
> 
> Am 09.04.25 um 13:36 schrieb Martyna Szapar-Mudlaw:
>> Introduce a new ethtool statistic to ice driver, `link_down_events`,
>> to track the number of times the link transitions from up to down.
>> This counter can help diagnose issues related to link stability,
>> such as port flapping or unexpected link drops.
>>
>> The counter increments when a link-down event occurs and is exposed
>> via ethtool stats as `link_down_events.nic`.
> 
> It’d be great if you pasted an example output.

In v2 (which I just submitted) the generic ethtool statistic is used for 
this, instead of driver specific, so I guess no need to paste the 
example output now.

> 
>> Reviewed-by: Michal Kubiak <michal.kubiak@...el.com>
>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar- 
>> mudlaw@...ux.intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice.h         | 1 +
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 1 +
>>   drivers/net/ethernet/intel/ice/ice_main.c    | 3 +++
>>   3 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ 
>> ethernet/intel/ice/ice.h
>> index 7200d6042590..6304104d1900 100644
>> --- a/drivers/net/ethernet/intel/ice/ice.h
>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>> @@ -621,6 +621,7 @@ struct ice_pf {
>>       u16 globr_count;    /* Global reset count */
>>       u16 empr_count;        /* EMP reset count */
>>       u16 pfr_count;        /* PF reset count */
>> +    u32 link_down_events;
> 
> Why not u16?

So now using u32 instead of u16 is more justified, as the v2 uses the 
generic ethtool stat, where this value is also u32 :)

> 
>>       u8 wol_ena : 1;        /* software state of WoL */
>>       u32 wakeup_reason;    /* last wakeup reason */
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/ 
>> net/ethernet/intel/ice/ice_ethtool.c
>> index b0805704834d..7bad0113aa88 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> @@ -137,6 +137,7 @@ static const struct ice_stats 
>> ice_gstrings_pf_stats[] = {
>>       ICE_PF_STAT("mac_remote_faults.nic", stats.mac_remote_faults),
>>       ICE_PF_STAT("fdir_sb_match.nic", stats.fd_sb_match),
>>       ICE_PF_STAT("fdir_sb_status.nic", stats.fd_sb_status),
>> +    ICE_PF_STAT("link_down_events.nic", link_down_events),
>>       ICE_PF_STAT("tx_hwtstamp_skipped", ptp.tx_hwtstamp_skipped),
>>       ICE_PF_STAT("tx_hwtstamp_timeouts", ptp.tx_hwtstamp_timeouts),
>>       ICE_PF_STAT("tx_hwtstamp_flushed", ptp.tx_hwtstamp_flushed),
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ 
>> ethernet/intel/ice/ice_main.c
>> index a03e1819e6d5..d68dd2a3f4a6 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -1144,6 +1144,9 @@ ice_link_event(struct ice_pf *pf, struct 
>> ice_port_info *pi, bool link_up,
>>       if (link_up == old_link && link_speed == old_link_speed)
>>           return 0;
>> +    if (!link_up && old_link)
>> +        pf->link_down_events++;
>> +
>>       ice_ptp_link_change(pf, link_up);
>>       if (ice_is_dcb_active(pf)) {
> 
> The diff looks good.

Thank you for the review,
Martyna
> 
> 
> Kind regards,
> 
> Paul
> 
>