| 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: <8BEEC6D9-EB5F-4A66-8BFD-E8FEE4EB723F@net-swift.com>
Date: Sun, 11 Dec 2022 15:35:04 +0800
From: "mengyuanlou@...-swift.com" <mengyuanlou@...-swift.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, jiawenwu@...stnetic.com
Subject: Re: [PATCH net-next v2] net: ngbe: Add ngbe mdio bus driver.
> 2022年12月10日 18:47，Andrew Lunn <andrew@...n.ch> 写道：
>
>> int ngbe_reset_hw(struct ngbe_hw *hw)
>> {
>> struct wx_hw *wxhw = &hw->wxhw;
>> - int status = 0;
>> - u32 reset = 0;
>> + u32 val = 0;
>> + int ret = 0;
>>
>> /* Call adapter stop to disable tx/rx and clear interrupts */
>> - status = wx_stop_adapter(wxhw);
>> - if (status != 0)
>> - return status;
>> - reset = WX_MIS_RST_LAN_RST(wxhw->bus.func);
>> - wr32(wxhw, WX_MIS_RST, reset | rd32(wxhw, WX_MIS_RST));
>> + ret = wx_stop_adapter(wxhw);
>> + if (ret != 0)
>> + return ret;
>> +
>> + if (hw->mac_type != ngbe_mac_type_mdi) {
>> + val = WX_MIS_RST_LAN_RST(wxhw->bus.func);
>> + wr32(wxhw, WX_MIS_RST, val | rd32(wxhw, WX_MIS_RST));
>> +
>> + ret = read_poll_timeout(rd32, val,
>> + !(val & (BIT(9) << wxhw->bus.func)), 1000,
>> + 100000, false, wxhw, 0x10028);
>> + if (ret)
>> + wx_dbg(wxhw, "Lan reset exceed s maximum times.\n");
>
> This is a real error. Return it up the call stack so the user gets to
> know. This should be netdev_err(), since it is an error.
>
>> +static void ngbe_up(struct ngbe_adapter *adapter)
>> +{
>> + struct ngbe_hw *hw = &adapter->hw;
>> +
>> + pci_set_master(adapter->pdev);
>> + if (hw->gpio_ctrl)
>> + /* gpio0 is used to power on control*/
>> + wr32(&hw->wxhw, NGBE_GPIO_DR, 0);
>
> Control of what?
Control for sfp modules power down/up.
The chip has not i2c interface, so I do not use phylink.
>
>> +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);
>> +
>> + if (ret)
>> + wx_dbg(wxhw, "PHY address command did not complete.\n");
>
> netdev_err() and return the error code.
>
>> +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;
>> +
>> + 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);
>> +
>> + if (ret)
>> + wx_dbg(wxhw, "PHY address command did not complete.\n");
>
> dev_error().
>
>> +
>> + 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;
>> +
>> + 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;
>
> You return 0 if it is neither MDI or RGMII. It would be better to
> either return -EINVAL, or 0xffff. A read on an MDIO bus for an address
> without a device returns 0xffff due to the pull up resisters. That
> will let Linux know there is no device there.
>
>> +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;
>> +
>> + if (hw->link != phydev->link ||
>> + hw->speed != phydev->speed ||
>> + hw->duplex != phydev->duplex) {
>> + changed = true;
>> + hw->link = phydev->link;
>> + hw->speed = phydev->speed;
>> + hw->duplex = phydev->duplex;
>> + }
>> +
>> + if (!changed)
>> + return;
>> +
>> + switch (phydev->speed) {
>> + case SPEED_1000:
>> + lan_speed = 2;
>> + break;
>> + case SPEED_100:
>> + lan_speed = 1;
>> + break;
>> + case SPEED_10:
>> + lan_speed = 0;
>> + break;
>> + default:
>> + break;
>> + }
>> + wr32m(wxhw, NGBE_CFG_LAN_SPEED, 0x3, lan_speed);
>> +
>> + if (phydev->link) {
>> + if (phydev->speed & (SPEED_1000 | SPEED_100 | SPEED_10)) {
>
> You logically OR 10, 100 and 1000? These SPEED_FOO macros are not
> bits.
>
> Andrew
>
Powered by blists - more mailing lists