| 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
| ||
|
Message-ID: <bd1c2858-c353-456f-865d-a9e85756e8f6@lunn.ch>
Date: Fri, 22 Nov 2024 23:34:07 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Frank Sae <Frank.Sae@...or-comm.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, xiaogang.fan@...or-comm.com,
fei.zhang@...or-comm.com, hua.sun@...or-comm.com
Subject: Re: [PATCH net-next v2 13/21] motorcomm:yt6801: Implement some
ethtool_ops function
> +static const struct fxgmac_stats_desc fxgmac_gstring_stats[] = {
> + /* MMC TX counters */
> + FXGMAC_STAT("tx_bytes", txoctetcount_gb),
> + FXGMAC_STAT("tx_bytes_good", txoctetcount_g),
> + FXGMAC_STAT("tx_packets", txframecount_gb),
> + FXGMAC_STAT("tx_packets_good", txframecount_g),
> + FXGMAC_STAT("tx_unicast_packets", txunicastframes_gb),
> + FXGMAC_STAT("tx_broadcast_packets", txbroadcastframes_gb),
> + FXGMAC_STAT("tx_broadcast_packets_good", txbroadcastframes_g),
> + FXGMAC_STAT("tx_multicast_packets", txmulticastframes_gb),
> + FXGMAC_STAT("tx_multicast_packets_good", txmulticastframes_g),
> + FXGMAC_STAT("tx_vlan_packets_good", txvlanframes_g),
> + FXGMAC_STAT("tx_64_byte_packets", tx64octets_gb),
> + FXGMAC_STAT("tx_65_to_127_byte_packets", tx65to127octets_gb),
> + FXGMAC_STAT("tx_128_to_255_byte_packets", tx128to255octets_gb),
> + FXGMAC_STAT("tx_256_to_511_byte_packets", tx256to511octets_gb),
> + FXGMAC_STAT("tx_512_to_1023_byte_packets", tx512to1023octets_gb),
> + FXGMAC_STAT("tx_1024_to_max_byte_packets", tx1024tomaxoctets_gb),
> + FXGMAC_STAT("tx_underflow_errors", txunderflowerror),
> + FXGMAC_STAT("tx_pause_frames", txpauseframes),
> + FXGMAC_STAT("tx_single_collision", txsinglecollision_g),
> + FXGMAC_STAT("tx_multiple_collision", txmultiplecollision_g),
> + FXGMAC_STAT("tx_deferred_frames", txdeferredframes),
> + FXGMAC_STAT("tx_late_collision_frames", txlatecollisionframes),
> + FXGMAC_STAT("tx_excessive_collision_frames",
> + txexcessivecollisionframes),
> + FXGMAC_STAT("tx_carrier_error_frames", txcarriererrorframes),
> + FXGMAC_STAT("tx_excessive_deferral_error", txexcessivedeferralerror),
> + FXGMAC_STAT("tx_oversize_frames_good", txoversize_g),
Please look at the rmon group.
> +static void fxgmac_ethtool_get_drvinfo(struct net_device *netdev,
> + struct ethtool_drvinfo *drvinfo)
> +{
> + struct fxgmac_pdata *pdata = netdev_priv(netdev);
> + u32 ver = pdata->hw_feat.version;
> + u32 sver, devid, userver;
> +
> + strscpy(drvinfo->version, pdata->drv_ver, sizeof(drvinfo->version));
Please leave this blank, so the core fills it with the git hash of the
kernel.
> +
> + /* S|SVER: MAC Version
> + * D|DEVID: Indicates the Device family
> + * U|USERVER: User-defined Version
> + */
> + sver = FXGMAC_GET_BITS(ver, MAC_VR_SVER_POS, MAC_VR_SVER_LEN);
> + devid = FXGMAC_GET_BITS(ver, MAC_VR_DEVID_POS, MAC_VR_DEVID_LEN);
> + userver = FXGMAC_GET_BITS(ver, MAC_VR_USERVER_POS, MAC_VR_USERVER_LEN);
> +
> + snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
> + "S.D.U: %x.%x.%x", sver, devid, userver);
You might want to consider devlink, which gives you more flexibility
in reporting versions.
> +static void fxgmac_ethtool_set_msglevel(struct net_device *netdev, u32 msglevel)
> +{
> + struct fxgmac_pdata *pdata = netdev_priv(netdev);
> +
> + yt_dbg(pdata, "set msglvl from %08x to %08x\n", pdata->msg_enable,
> + msglevel);
> + pdata->msg_enable = msglevel;
This is an example of a yt_dbg() which seems pointless now that you
have debugged it. Maybe you did have a bug in the first version of
this one line function, but it looks reasonably bug free now, so i
don't see the need for the print.
> +static void fxgmac_get_regs(struct net_device *netdev,
> + struct ethtool_regs *regs, void *p)
> +{
> + struct fxgmac_pdata *pdata = netdev_priv(netdev);
> + u32 *regs_buff = p;
> +
> + memset(p, 0, FXGMAC_PHY_REG_CNT * sizeof(u32));
> + for (u32 i = MII_BMCR; i < FXGMAC_PHY_REG_CNT; i++)
> + regs_buff[i] = phy_read(pdata->phydev, i);
A MAC driver should not be touching the PHY. There are other
standardised ways of dumping PHY registers, which don't involve the
MAC driver.
> +static void fxgmac_get_pauseparam(struct net_device *dev,
> + struct ethtool_pauseparam *data)
> +{
> + struct fxgmac_pdata *yt = netdev_priv(dev);
Please be consistent with your naming. The function above calls this
pdata. I don't particularly like pdata, because that often means
platform_data, which you don't have. priv is more common.
> + bool tx_pause, rx_pause;
> +
> + phy_get_pause(yt->phydev, &tx_pause, &rx_pause);
> +
> + data->autoneg = yt->phydev->autoneg;
This is wrong. You can be doing autoneg in general,
i.e. phydev->autoneg, but not using autoneg for pause. You should
remember the value set in set_pauseparam().
> + data->tx_pause = tx_pause ? 1 : 0;
> + data->rx_pause = rx_pause ? 1 : 0;
Why the ? 1: 0 ?
> +
> + yt_dbg(yt, "%s, rx=%d, tx=%d\n", __func__, data->rx_pause,
> + data->tx_pause);
> +}
> +
> +static int fxgmac_set_pauseparam(struct net_device *dev,
> + struct ethtool_pauseparam *data)
> +{
> + struct fxgmac_pdata *yt = netdev_priv(dev);
> +
> + if (dev->mtu > ETH_DATA_LEN)
> + return -EOPNOTSUPP;
A comment about why jumbo packets breaks pause would be good. I also
assume your set_mtu code disables pause when jumbo is enabled?
get_pauseparam should also make it clear pause is disabled.
> + phy_set_asym_pause(yt->phydev, data->rx_pause, data->tx_pause);
You are ignoring ethtool_pauseparam autoneg.
> +static void fxgmac_ethtool_get_strings(struct net_device *netdev, u32 stringset,
> + u8 *data)
> +{
> + switch (stringset) {
> + case ETH_SS_STATS:
> + for (u32 i = 0; i < FXGMAC_STATS_COUNT; i++) {
> + memcpy(data, fxgmac_gstring_stats[i].stat_string,
> + strlen(fxgmac_gstring_stats[i].stat_string));
> + data += ETH_GSTRING_LEN;
> + }
> + break;
> + default:
> + WARN_ON(1);
You need to be careful with WARN_ON() particularly if it can be
triggered from user space. Maybe build cause a WARN to result in a
reboot.
> +static int fxgmac_ethtool_reset(struct net_device *netdev, u32 *flag)
> +{
> + struct fxgmac_pdata *pdata = netdev_priv(netdev);
> + int ret = 0;
> + u32 val;
> +
> + val = *flag & (ETH_RESET_ALL | ETH_RESET_PHY);
> + if (!val) {
> + yt_err(pdata, "Operation not support.\n");
> + return -EOPNOTSUPP;
> + }
> +
> + switch (*flag) {
> + case ETH_RESET_ALL:
> + fxgmac_restart(pdata);
> + *flag = 0;
> + break;
> + case ETH_RESET_PHY:
> + /* Power off and on the phy in order to properly
> + * configure the MAC timing
> + */
> + ret = phy_modify(pdata->phydev, MII_BMCR, BMCR_PDOWN,
> + BMCR_PDOWN);
> + if (ret < 0)
> + return ret;
> + fsleep(9000);
> +
> + ret = phy_modify(pdata->phydev, MII_BMCR, BMCR_PDOWN, 0);
> + if (ret < 0)
> + return ret;
> + *flag = 0;
This is a bad idea, since depending on the PHY, you have thrown away
all the configuration, and the PHY is now probably dead until you
admin down/admin up the interface. In general a MAC driver should
never directly touch the PHY, it should just use the API phylib
exposes.
Andrew
---
pw-bot: cr
Powered by blists - more mailing lists