| 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
| ||
|
Date: Mon, 17 Jun 2024 06:54:59 +0000
From: Justin Lai <justinlai0215@...ltek.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com"
<edumazet@...gle.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"andrew@...n.ch"
<andrew@...n.ch>,
"jiri@...nulli.us" <jiri@...nulli.us>,
"horms@...nel.org"
<horms@...nel.org>,
"rkannoth@...vell.com" <rkannoth@...vell.com>,
"Ping-Ke
Shih" <pkshih@...ltek.com>,
Larry Chiu <larry.chiu@...ltek.com>
Subject: RE: [PATCH net-next v20 10/13] rtase: Implement ethtool function
> 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,
Powered by blists - more mailing lists