RE: [PATCH net-next v20 10/13] rtase: Implement ethtool function

[Justin Lai <justinlai0215@realtek.com>]

> On Fri, 7 Jun 2024 16:43:18 +0800 Justin Lai wrote:
> > Implement the ethtool function to support users to obtain network card
> > information, including obtaining various device settings, Report
> > whether physical link is up, Report pause parameters, Set pause
> > parameters, Return a set of strings that describe the requested
> > objects, Get number of strings that @get_strings will write, Return
> > extended statistics about the device.
> 
> You don't implement get_strings any more.

Sorry, I will modify it.

> 
> > +static void rtase_get_drvinfo(struct net_device *dev,
> > +                           struct ethtool_drvinfo *drvinfo) {
> > +     const struct rtase_private *tp = netdev_priv(dev);
> > +
> > +     strscpy(drvinfo->driver, KBUILD_MODNAME, 32);
> 
> sizeof(drvinfo->driver) instead of the literal 32?

It seems like a better approach, I'll switch to using
sizeof(drvinfo->driver), thank you.

> 
> > +     strscpy(drvinfo->bus_info, pci_name(tp->pdev), 32);
> 
> Can you double check that overwriting these fields is actually needed?
> I think core will fill this in for you in ethtool_get_drvinfo()

I have removed this line of code for testing. Before removing the code,
I could obtain bus info by entering "ethtool -i". However, after removing
the code, entering "ethtool -i" no longer retrieves the bus info.
Therefore, I believe that this line of code is still necessary.

> 
> > +     if ((value & (RTASE_FORCE_TXFLOW_EN |
> RTASE_FORCE_RXFLOW_EN)) ==
> > +         (RTASE_FORCE_TXFLOW_EN | RTASE_FORCE_RXFLOW_EN)) {
> > +             pause->rx_pause = 1;
> > +             pause->tx_pause = 1;
> > +     } else if ((value & RTASE_FORCE_TXFLOW_EN)) {
> 
> unnecessary parenthesis
> 
> > +             pause->tx_pause = 1;
> > +     } else if ((value & RTASE_FORCE_RXFLOW_EN)) {
> 
> same here
> 

Sorry, I will remove the unnecessary parentheses, thank you.

> > +             pause->rx_pause = 1;
> > +     }
> > +}
> > +
> > +static int rtase_set_pauseparam(struct net_device *dev,
> > +                             struct ethtool_pauseparam *pause) {
> > +     const struct rtase_private *tp = netdev_priv(dev);
> > +     u16 value = rtase_r16(tp, RTASE_CPLUS_CMD);
> > +
> > +     if (pause->autoneg)
> > +             return -EOPNOTSUPP;
> > +
> > +     value &= ~(RTASE_FORCE_TXFLOW_EN |
> RTASE_FORCE_RXFLOW_EN);
> > +
> > +     if (pause->tx_pause)
> > +             value |= RTASE_FORCE_TXFLOW_EN;
> > +
> > +     if (pause->rx_pause)
> > +             value |= RTASE_FORCE_RXFLOW_EN;
> > +
> > +     rtase_w16(tp, RTASE_CPLUS_CMD, value);
> > +     return 0;
> > +}
> > +
> > +static void rtase_get_eth_mac_stats(struct net_device *dev,
> > +                                 struct ethtool_eth_mac_stats
> *stats)
> > +{
> > +     struct rtase_private *tp = netdev_priv(dev);
> > +     const struct rtase_counters *counters;
> > +
> > +     counters = tp->tally_vaddr;
> > +     if (!counters)
> 
> you fail probe if this is NULL, why check if here?
> 

Sorry, this check seems unnecessary, I will remove it.

> > +             return;
> > +
> > +     rtase_dump_tally_counter(tp);
> > +
> > +     stats->FramesTransmittedOK = le64_to_cpu(counters->tx_packets);
> > +     stats->SingleCollisionFrames =
> le32_to_cpu(counters->tx_one_collision);
> > +     stats->MultipleCollisionFrames =
> > +             le32_to_cpu(counters->tx_multi_collision);
> > +     stats->FramesReceivedOK = le64_to_cpu(counters->rx_packets);
> > +     stats->FrameCheckSequenceErrors =
> > + le32_to_cpu(counters->rx_errors);
> 
> You dont report this in rtase_get_stats64() as crc errors, are these really CRC /
> FCS errors or other errors?

Our rx_error indeed refers to crc_error. I will assign the value of
rx_error to the crc_error in rtase_get_stats64().

> 
> > +     stats->AlignmentErrors = le16_to_cpu(counters->align_errors);
> > +     stats->FramesAbortedDueToXSColls =
> le16_to_cpu(counters->tx_aborted);
> > +     stats->FramesLostDueToIntMACXmitError =
> > +             le64_to_cpu(counters->tx_errors);
> > +     stats->FramesLostDueToIntMACRcvError =
> > +             le16_to_cpu(counters->rx_missed);
> 
> Are you sure this is the correct statistic to report as?
> What's the definition of rx_missed in the datasheet?

What we refer to as rx miss is the packets that can't be received because
the fifo in the MAC is full. We consider this a type of MAC error, identical
to the definition of FramesLostDueToIntMACRcvError.

> 
> Also is 16 bits enough for a packet counter at 5Gbps?
> Don't you have to periodically accumulate this counter so that it doesn't wrap
> around?

Indeed, this counter may wrap, but we don't need to accumulate it, because
an increase in the number of rx_miss largely indicates that the system
processing speed is not fast enough. Therefore, the size of this counter
doesn't need to be too large.

> 
> > +     stats->MulticastFramesReceivedOK =
> le32_to_cpu(counters->rx_multicast);
> > +     stats->BroadcastFramesReceivedOK =
> > +le64_to_cpu(counters->rx_broadcast);
> > +}
> > +
> > +static const struct ethtool_ops rtase_ethtool_ops = {
> > +     .get_drvinfo = rtase_get_drvinfo,
> > +     .get_link = ethtool_op_get_link,
> > +     .get_link_ksettings = rtase_get_settings,
> > +     .get_pauseparam = rtase_get_pauseparam,
> > +     .set_pauseparam = rtase_set_pauseparam,
> > +     .get_eth_mac_stats = rtase_get_eth_mac_stats,
> > +     .get_ts_info = ethtool_op_get_ts_info, };
> > +
> >  static void rtase_init_netdev_ops(struct net_device *dev)  {
> >       dev->netdev_ops = &rtase_netdev_ops;
> > +     dev->ethtool_ops = &rtase_ethtool_ops;
> >  }
> >
> >  static void rtase_reset_interrupt(struct pci_dev *pdev,