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: <23d2bbcca442457fa3efa5533b0a4246@realtek.com> Date: Fri, 18 Aug 2023 12:01:17 +0000 From: Justin Lai <justinlai0215@...ltek.com> To: Andrew Lunn <andrew@...n.ch> CC: "kuba@...nel.org" <kuba@...nel.org>, "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> Subject: RE: [PATCH net-next v4 1/2] net/ethernet/realtek: Add Realtek automotive PCIe driver code > +static inline void rtase_enable_hw_interrupt(const struct > rtase_private *tp) > > If you read comments given to other developers, you would of seen that > inline functions are not allowed in .C files. Let the compiler decide. > > > +static int rtase_get_settings(struct net_device *dev, struct > > +ethtool_link_ksettings *cmd) { > > + const struct rtase_private *tp = netdev_priv(dev); > > + u32 advertising = 0; > > + u32 supported = 0; > > + u32 speed = 0; > > + u16 value = 0; > > + > > + supported |= SUPPORTED_MII | SUPPORTED_Pause; > > + > > + advertising |= ADVERTISED_MII; > > You don't advertise anything, because you don't have a PHY. > > > + > > + /* speed */ > > + switch (tp->pdev->bus->cur_bus_speed) { > > Speed is meant to be line side speed. That is fixed at 5G. So i would > expect a hard coded value. > > > +static void rtase_get_pauseparam(struct net_device *dev, struct > > +ethtool_pauseparam *pause) { > > + const struct rtase_private *tp = netdev_priv(dev); > > + u16 value = RTL_R16(tp, CPLUS_CMD); > > + > > + if ((value & (FORCE_TXFLOW_EN | FORCE_RXFLOW_EN)) == > (FORCE_TXFLOW_EN | FORCE_RXFLOW_EN)) { > > + pause->rx_pause = 1; > > + pause->tx_pause = 1; > > + } else if ((value & FORCE_TXFLOW_EN) != 0u) { > > + pause->tx_pause = 1; > > + } else if ((value & FORCE_RXFLOW_EN) != 0u) { > > + pause->rx_pause = 1; > > + } else { > > + /* not enable pause */ > > + } > > Probably not required, but it would be good to set pause.autoneg to > false, just to make is clear you don't support negotiating it. > > > +} > > + > > +static int rtase_set_pauseparam(struct net_device *dev, struct > > +ethtool_pauseparam *pause) { > > + const struct rtase_private *tp = netdev_priv(dev); > > + u16 value = RTL_R16(tp, CPLUS_CMD); > > Similar to above, you should return EOPNOTSUPP if pause.autoneg is true. > > > + > > + value &= ~(FORCE_TXFLOW_EN | FORCE_RXFLOW_EN); > > + > > + if (pause->tx_pause == 1u) > > + value |= FORCE_TXFLOW_EN; > > + > > + if (pause->rx_pause == 1u) > > You can treat these as boolean. > > > + value |= FORCE_RXFLOW_EN; > > + > > + RTL_W16(tp, CPLUS_CMD, value); > > + return 0; > > +} > > > Andrew Hi, Andrew Thanks for your quick review, I will check our code again and make changes. > > --- > pw-bot: cr >
Powered by blists - more mailing lists