| Message ID | 20240607084321.7254-11-justinlai0215@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add Realtek automotive PCIe driver | expand |
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. > +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? > + 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() > + 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 > + 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? > + 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? > + 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? 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? > + 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,
> 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,
> > > + 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. https://elixir.bootlin.com/linux/latest/source/net/ethtool/ioctl.c#L710 if (ops->get_drvinfo) { ops->get_drvinfo(dev, &rsp->info); if (!rsp->info.bus_info[0] && parent) strscpy(rsp->info.bus_info, dev_name(parent), sizeof(rsp->info.bus_info)); This suggests you have not set the parent device. Andrew
On Mon, 17 Jun 2024 06:54:59 +0000 Justin Lai wrote: > > 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. Is this a FIFO which silicon designers say can't overflow? Or it will overflow if the host is busy and doesn't pick up packets? If it's the latter I recommend using stats64::rx_missed_errors. That's the start we try to steer all NIC drivers towards for "host is too slow". > > 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. Are you basically saying that since its an error it only matters if its zero or not? It's not going to be a great experience for anyone trying to use this driver. You can read this counter periodically from a timer and accumulate a fuller value in the driver. There's even struct ethtool_coalesce::stats_block_coalesce_usecs if you want to let user configure the period.
> On Mon, 17 Jun 2024 06:54:59 +0000 Justin Lai wrote: > > > 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. > > Is this a FIFO which silicon designers say can't overflow? > Or it will overflow if the host is busy and doesn't pick up packets? > If it's the latter I recommend using stats64::rx_missed_errors. > That's the start we try to steer all NIC drivers towards for "host > is too slow". Our fifo falls under the second situation you mentioned, and we are currently already using stats64::rx_missed_errors. Therefore, we will remove the parts that use 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. > > Are you basically saying that since its an error it only matters if > its zero or not? It's not going to be a great experience for anyone > trying to use this driver. You can read this counter periodically from > a timer and accumulate a fuller value in the driver. There's even > struct ethtool_coalesce::stats_block_coalesce_usecs if you want to let > user configure the period. As we've discussed, as long as this counter has a value, it can inform the user that the host speed is too slow, and it will not affect other transmission functions. Can we add this periodic reading function after this patch version is merged?
> > > > > + 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. > > https://elixir.bootlin.com/linux/latest/source/net/ethtool/ioctl.c#L710 > > if (ops->get_drvinfo) { > ops->get_drvinfo(dev, &rsp->info); > if (!rsp->info.bus_info[0] && parent) > strscpy(rsp->info.bus_info, dev_name(parent), > sizeof(rsp->info.bus_info)); > > This suggests you have not set the parent device. > > Andrew Hi Andrew, I understand your explanation. However, when we input ethtool -i, shouldn't we aim to get the bus info of the actual device rather than the parent device? That's why I think we need to add this line.
On Tue, 18 Jun 2024 07:28:13 +0000 Justin Lai wrote: > > Are you basically saying that since its an error it only matters if > > its zero or not? It's not going to be a great experience for anyone > > trying to use this driver. You can read this counter periodically from > > a timer and accumulate a fuller value in the driver. There's even > > struct ethtool_coalesce::stats_block_coalesce_usecs if you want to let > > user configure the period. > > As we've discussed, as long as this counter has a value, it can inform > the user that the host speed is too slow, and it will not affect other > transmission functions. Can we add this periodic reading function > after this patch version is merged? You'd have to remove reporting of all 16b packet statistics and 32b byte statistics completely for now, and then follow up adding them with the periodic overflow checks.
> > On Tue, 18 Jun 2024 07:28:13 +0000 Justin Lai wrote: > > > Are you basically saying that since its an error it only matters if > > > its zero or not? It's not going to be a great experience for anyone > > > trying to use this driver. You can read this counter periodically > > > from a timer and accumulate a fuller value in the driver. There's > > > even struct ethtool_coalesce::stats_block_coalesce_usecs if you want > > > to let user configure the period. > > > > As we've discussed, as long as this counter has a value, it can inform > > the user that the host speed is too slow, and it will not affect other > > transmission functions. Can we add this periodic reading function > > after this patch version is merged? > > You'd have to remove reporting of all 16b packet statistics and 32b byte > statistics completely for now, and then follow up adding them with the > periodic overflow checks. Thank you for your reply, I will remove the 16b packet statistics and 32b byte statistics report in this version.
diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c index e9bbafc212c5..da2627ea99fe 100644 --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c @@ -1758,9 +1758,112 @@ static void rtase_get_mac_address(struct net_device *dev) rtase_rar_set(tp, dev->dev_addr); } +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); + strscpy(drvinfo->bus_info, pci_name(tp->pdev), 32); +} + +static int rtase_get_settings(struct net_device *dev, + struct ethtool_link_ksettings *cmd) +{ + u32 supported = SUPPORTED_MII | SUPPORTED_Pause | SUPPORTED_Asym_Pause; + + ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported, + supported); + cmd->base.speed = SPEED_5000; + cmd->base.duplex = DUPLEX_FULL; + cmd->base.port = PORT_MII; + cmd->base.autoneg = AUTONEG_DISABLE; + + return 0; +} + +static void rtase_get_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); + + pause->autoneg = AUTONEG_DISABLE; + + 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)) { + pause->tx_pause = 1; + } else if ((value & RTASE_FORCE_RXFLOW_EN)) { + 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) + 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); + 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); + 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,
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. Signed-off-by: Justin Lai <justinlai0215@realtek.com> --- .../net/ethernet/realtek/rtase/rtase_main.c | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+)