| 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: Thu, 8 Dec 2022 19:42:15 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Mengyuan Lou <mengyuanlou@...-swift.com>
Cc: netdev@...r.kernel.org, jiawenwu@...stnetic.com
Subject: Re: [PATCH net-next v2] net: ngbe: Add ngbe mdio bus driver.
On Tue, 6 Dec 2022 19:40:35 +0800 Mengyuan Lou wrote:
> Add mdio bus register for ngbe.
> The internal phy and external phy need to be handled separately.
> Add phy changed event detection.
> static void ngbe_down(struct ngbe_adapter *adapter)
> {
> - netif_carrier_off(adapter->netdev);
> - netif_tx_disable(adapter->netdev);
> + struct ngbe_hw *hw = &adapter->hw;
> +
> + phy_stop(hw->phydev);
> + ngbe_disable_device(adapter);
> };
>
> +static void ngbe_up(struct ngbe_adapter *adapter)
> +{
> + struct ngbe_hw *hw = &adapter->hw;
> +
> + pci_set_master(adapter->pdev);
why set/clear bus master on up/down? This is normally done during probe
and cleared on remove. Having bus mastering enabled doesn't cost
anything.
> + if (hw->gpio_ctrl)
> + /* gpio0 is used to power on control*/
> + wr32(&hw->wxhw, NGBE_GPIO_DR, 0);
> + phy_start(hw->phydev);
> +static int ngbe_phy_read_reg_mdi(struct mii_bus *bus, int phy_addr, int regnum)
> +{
> + u32 command = 0, device_type = 0;
> + struct ngbe_hw *hw = bus->priv;
> + struct wx_hw *wxhw = &hw->wxhw;
> + u32 phy_data = 0;
> + u32 val = 0;
> + int ret = 0;
> +
> + if (regnum & MII_ADDR_C45) {
> + wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0x0);
> + /* setup and write the address cycle command */
> + command = NGBE_MSCA_RA(mdiobus_c45_regad(regnum)) |
> + NGBE_MSCA_PA(phy_addr) |
> + NGBE_MSCA_DA(mdiobus_c45_devad(regnum));
> + } else {
> + wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0xF);
> + /* setup and write the address cycle command */
> + command = NGBE_MSCA_RA(regnum) |
> + NGBE_MSCA_PA(phy_addr) |
> + NGBE_MSCA_DA(device_type);
> + }
> + wr32(wxhw, NGBE_MSCA, command);
> + command = NGBE_MSCC_CMD(NGBE_MSCA_CMD_READ) |
> + NGBE_MSCC_BUSY |
> + NGBE_MDIO_CLK(6);
> + wr32(wxhw, NGBE_MSCC, command);
> +
> + /* wait to complete */
> + ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
> + 100000, false, wxhw, NGBE_MSCC);
> +
spurious new line
> + if (ret)
> + wx_dbg(wxhw, "PHY address command did not complete.\n");
> +
> + /* read data from MSCC */
> + phy_data = 0xffff & rd32(wxhw, NGBE_MSCC);
> +
> + return phy_data;
> +}
> +
> +static int ngbe_phy_write_reg_mdi(struct mii_bus *bus, int phy_addr, int regnum, u16 value)
> +{
> + u32 command = 0, device_type = 0;
> + struct ngbe_hw *hw = bus->priv;
> + struct wx_hw *wxhw = &hw->wxhw;
> + int ret = 0;
> + u16 val = 0;
Don't pointlessly initialize all variables.
There is a compiler option which does that, for the paranoid.
> + if (regnum & MII_ADDR_C45) {
> + wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0x0);
> + /* setup and write the address cycle command */
> + command = NGBE_MSCA_RA(mdiobus_c45_regad(regnum)) |
> + NGBE_MSCA_PA(phy_addr) |
> + NGBE_MSCA_DA(mdiobus_c45_devad(regnum));
> + } else {
> + wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0xF);
> + /* setup and write the address cycle command */
> + command = NGBE_MSCA_RA(regnum) |
> + NGBE_MSCA_PA(phy_addr) |
> + NGBE_MSCA_DA(device_type);
> + }
> + wr32(wxhw, NGBE_MSCA, command);
> + command = value |
> + NGBE_MSCC_CMD(NGBE_MSCA_CMD_WRITE) |
> + NGBE_MSCC_BUSY |
> + NGBE_MDIO_CLK(6);
> + wr32(wxhw, NGBE_MSCC, command);
> +
> + /* wait to complete */
> + ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
> + 100000, false, wxhw, NGBE_MSCC);
> +
spurious empty line
> + if (ret)
> + wx_dbg(wxhw, "PHY address command did not complete.\n");
> +
> + return ret;
> +}
> +
> +static int ngbe_phy_read_reg(struct mii_bus *bus, int phy_addr, int regnum)
> +{
> + struct ngbe_hw *hw = bus->priv;
> + u16 phy_data = 0;
Why 0? If the mac_type is not known error would seem more appropriate?
> + if (hw->mac_type == ngbe_mac_type_mdi)
> + phy_data = ngbe_phy_read_reg_internal(bus, phy_addr, regnum);
> + else if (hw->mac_type == ngbe_mac_type_rgmii)
> + phy_data = ngbe_phy_read_reg_mdi(bus, phy_addr, regnum);
> +
> + return phy_data;
> +}
> +
> +static int ngbe_phy_write_reg(struct mii_bus *bus, int phy_addr, int regnum, u16 value)
> +{
> + struct ngbe_hw *hw = bus->priv;
> + int ret = 0;
Why 0? If the mac_type is not known error would seem more appropriate?
> + if (hw->mac_type == ngbe_mac_type_mdi)
> + ret = ngbe_phy_write_reg_internal(bus, phy_addr, regnum, value);
> + else if (hw->mac_type == ngbe_mac_type_rgmii)
> + ret = ngbe_phy_write_reg_mdi(bus, phy_addr, regnum, value);
> + return ret;
> +}
> +
> +static void ngbe_handle_link_change(struct net_device *dev)
> +{
> + struct ngbe_adapter *adapter = netdev_priv(dev);
> + struct ngbe_hw *hw = &adapter->hw;
> + struct wx_hw *wxhw = &hw->wxhw;
> + u32 lan_speed = 2, reg;
> + bool changed = false;
> +
> + struct phy_device *phydev = hw->phydev;
Local variables should be in one block.
If there is a problem with the ordering because of inter-dependencies
just initialize in the code rather than inline.
> + if (hw->link != phydev->link ||
> + hw->speed != phydev->speed ||
> + hw->duplex != phydev->duplex) {
> + changed = true;
This seems unnecessary, flip the condition and return here
without the need for @changed.
> + hw->link = phydev->link;
> + hw->speed = phydev->speed;
> + hw->duplex = phydev->duplex;
> + }
> +int ngbe_phy_connect(struct ngbe_hw *hw)
> +{
> + struct ngbe_adapter *adapter = container_of(hw,
> + struct ngbe_adapter,
> + hw);
> + int ret;
> +
> + ret = phy_connect_direct(adapter->netdev,
> + hw->phydev,
> + ngbe_handle_link_change,
> + PHY_INTERFACE_MODE_RGMII_ID);
> + if (ret) {
> + wx_err(&hw->wxhw,
> + "PHY connect failed.\n");
Unnecessary line break
> + snprintf(hw->mii_bus->id, MII_BUS_ID_SIZE, "ngbe-%x",
> + (pdev->bus->number << 8) |
> + pdev->devfn);
Unnecessary line break
Powered by blists - more mailing lists