| Message ID | 20230602173252.35711-1-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ionic: add support for ethtool extended stat link_down_count | expand |
On 02-06-2023 23:02, Shannon Nelson wrote: > From: Nitya Sunkad <nitya.sunkad@amd.com> > > Following the example of 9a0f830f8026 ("ethtool: linkstate: add a statistic > for PHY down events"), added support for link down events. s/added/add. > > Added callback ionic_get_link_ext_stats to ionic_ethtool.c to support s/Added/Add. > link_down_count, a property of netdev that gets incremented every time > the device link goes down. Please use imperative mood when writing commit messages. Also, I think it is a good practice to Cc all the email IDs generated by ./scripts/get_maintainer.pl. > > Run ethtool -I <devname> to display the device link down count. > > Signed-off-by: Nitya Sunkad <nitya.sunkad@amd.com> > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> Apart from my comments above, the patch looks good to me. Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 9 +++++++++ > drivers/net/ethernet/pensando/ionic/ionic_lif.c | 1 + > drivers/net/ethernet/pensando/ionic/ionic_lif.h | 1 + > 3 files changed, 11 insertions(+) > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c > index 9b2b96fa36af..4c527a06e7d9 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c > @@ -104,6 +104,14 @@ static void ionic_get_regs(struct net_device *netdev, struct ethtool_regs *regs, > memcpy_fromio(p + offset, lif->ionic->idev.dev_cmd_regs->words, size); > } > > +static void ionic_get_link_ext_stats(struct net_device *netdev, > + struct ethtool_link_ext_stats *stats) > +{ > + struct ionic_lif *lif = netdev_priv(netdev); > + > + stats->link_down_events = lif->link_down_count; > +} > + > static int ionic_get_link_ksettings(struct net_device *netdev, > struct ethtool_link_ksettings *ks) > { > @@ -1074,6 +1082,7 @@ static const struct ethtool_ops ionic_ethtool_ops = { > .get_regs_len = ionic_get_regs_len, > .get_regs = ionic_get_regs, > .get_link = ethtool_op_get_link, > + .get_link_ext_stats = ionic_get_link_ext_stats, > .get_link_ksettings = ionic_get_link_ksettings, > .set_link_ksettings = ionic_set_link_ksettings, > .get_coalesce = ionic_get_coalesce, > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > index 957027e546b3..6ccc1ea91992 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > @@ -168,6 +168,7 @@ 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); > } > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h > index c9c4c46d5a16..fd2ea670e7d8 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h > @@ -201,6 +201,7 @@ struct ionic_lif { > u64 hw_features; > bool registered; > u16 lif_type; > + unsigned int link_down_count; > unsigned int nmcast; > unsigned int nucast; > unsigned int nvlans;
On Fri, 2 Jun 2023 10:32:52 -0700 Shannon Nelson wrote: > Following the example of 9a0f830f8026 ("ethtool: linkstate: add a statistic > for PHY down events"), added support for link down events. > > Added callback ionic_get_link_ext_stats to ionic_ethtool.c to support > link_down_count, a property of netdev that gets incremented every time > the device link goes down. Hm, could you say more about motivation? The ethtool stat is supposed to come from HW and represent PHY-level flap count. It's used primarily to find bad cables in a datacenter. Is this also the use case for ionic? It's unclear to me whether ionic interfaces are seeing an actual PHY or just some virtual link state of the IPU and in the latter case it's not really a match.
On 6/2/23 11:47 PM, Jakub Kicinski wrote: > On Fri, 2 Jun 2023 10:32:52 -0700 Shannon Nelson wrote: >> Following the example of 9a0f830f8026 ("ethtool: linkstate: add a statistic >> for PHY down events"), added support for link down events. >> >> Added callback ionic_get_link_ext_stats to ionic_ethtool.c to support >> link_down_count, a property of netdev that gets incremented every time >> the device link goes down. > > Hm, could you say more about motivation? The ethtool stat is supposed to > come from HW and represent PHY-level flap count. It's used primarily to > find bad cables in a datacenter. Is this also the use case for ionic? > It's unclear to me whether ionic interfaces are seeing an actual PHY or > just some virtual link state of the IPU and in the latter case it's not > really a match. This is a fair question - yes, it is possible that the FW could fake it, but normally this is a signal from the HW. However, I suppose it would be best to only have the PF reporting this counter, and not the VFs, which is currently the case here. I'll have Nitya modify this for PF only. Thanks, sln
On 6/2/23 11:11 PM, Siddharth Vadapalli wrote: > On 02-06-2023 23:02, Shannon Nelson wrote: >> From: Nitya Sunkad <nitya.sunkad@amd.com> >> >> Following the example of 9a0f830f8026 ("ethtool: linkstate: add a statistic >> for PHY down events"), added support for link down events. > > s/added/add. > >> >> Added callback ionic_get_link_ext_stats to ionic_ethtool.c to support > > s/Added/Add > >> link_down_count, a property of netdev that gets incremented every time >> the device link goes down. > > Please use imperative mood when writing commit messages. We'll get these fixed up in a re-spin. > Also, I think it is a good practice to Cc all the email IDs generated by > ./scripts/get_maintainer.pl. Thanks, I'll keep that in mind. sln > >> >> Run ethtool -I <devname> to display the device link down count. >> >> Signed-off-by: Nitya Sunkad <nitya.sunkad@amd.com> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > Apart from my comments above, the patch looks good to me. > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >> --- >> drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 9 +++++++++ >> drivers/net/ethernet/pensando/ionic/ionic_lif.c | 1 + >> drivers/net/ethernet/pensando/ionic/ionic_lif.h | 1 + >> 3 files changed, 11 insertions(+) >> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c >> index 9b2b96fa36af..4c527a06e7d9 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c >> @@ -104,6 +104,14 @@ static void ionic_get_regs(struct net_device *netdev, struct ethtool_regs *regs, >> memcpy_fromio(p + offset, lif->ionic->idev.dev_cmd_regs->words, size); >> } >> >> +static void ionic_get_link_ext_stats(struct net_device *netdev, >> + struct ethtool_link_ext_stats *stats) >> +{ >> + struct ionic_lif *lif = netdev_priv(netdev); >> + >> + stats->link_down_events = lif->link_down_count; >> +} >> + >> static int ionic_get_link_ksettings(struct net_device *netdev, >> struct ethtool_link_ksettings *ks) >> { >> @@ -1074,6 +1082,7 @@ static const struct ethtool_ops ionic_ethtool_ops = { >> .get_regs_len = ionic_get_regs_len, >> .get_regs = ionic_get_regs, >> .get_link = ethtool_op_get_link, >> + .get_link_ext_stats = ionic_get_link_ext_stats, >> .get_link_ksettings = ionic_get_link_ksettings, >> .set_link_ksettings = ionic_set_link_ksettings, >> .get_coalesce = ionic_get_coalesce, >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> index 957027e546b3..6ccc1ea91992 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> @@ -168,6 +168,7 @@ 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); >> } >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h >> index c9c4c46d5a16..fd2ea670e7d8 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h >> @@ -201,6 +201,7 @@ struct ionic_lif { >> u64 hw_features; >> bool registered; >> u16 lif_type; >> + unsigned int link_down_count; >> unsigned int nmcast; >> unsigned int nucast; >> unsigned int nvlans; > > -- > Regards, > Siddharth.
On Mon, 5 Jun 2023 09:26:25 -0700 Shannon Nelson wrote: > > Hm, could you say more about motivation? The ethtool stat is supposed to > > come from HW and represent PHY-level flap count. It's used primarily to > > find bad cables in a datacenter. Is this also the use case for ionic? > > It's unclear to me whether ionic interfaces are seeing an actual PHY or > > just some virtual link state of the IPU and in the latter case it's not > > really a match. > > This is a fair question - yes, it is possible that the FW could fake it, > but normally this is a signal from the HW. However, I suppose it would > be best to only have the PF reporting this counter, and not the VFs, > which is currently the case here. I'll have Nitya modify this for PF only. Perfect, thanks!
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c index 9b2b96fa36af..4c527a06e7d9 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c @@ -104,6 +104,14 @@ static void ionic_get_regs(struct net_device *netdev, struct ethtool_regs *regs, memcpy_fromio(p + offset, lif->ionic->idev.dev_cmd_regs->words, size); } +static void ionic_get_link_ext_stats(struct net_device *netdev, + struct ethtool_link_ext_stats *stats) +{ + struct ionic_lif *lif = netdev_priv(netdev); + + stats->link_down_events = lif->link_down_count; +} + static int ionic_get_link_ksettings(struct net_device *netdev, struct ethtool_link_ksettings *ks) { @@ -1074,6 +1082,7 @@ static const struct ethtool_ops ionic_ethtool_ops = { .get_regs_len = ionic_get_regs_len, .get_regs = ionic_get_regs, .get_link = ethtool_op_get_link, + .get_link_ext_stats = ionic_get_link_ext_stats, .get_link_ksettings = ionic_get_link_ksettings, .set_link_ksettings = ionic_set_link_ksettings, .get_coalesce = ionic_get_coalesce, diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c index 957027e546b3..6ccc1ea91992 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c @@ -168,6 +168,7 @@ 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); } diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h index c9c4c46d5a16..fd2ea670e7d8 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h @@ -201,6 +201,7 @@ struct ionic_lif { u64 hw_features; bool registered; u16 lif_type; + unsigned int link_down_count; unsigned int nmcast; unsigned int nucast; unsigned int nvlans;