netdev - Re: [PATCH net-next v6] net: ngbe: Add ngbe mdio bus driver.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <6A65AA55-3962-4E48-A778-7D1EF0820D89@net-swift.com>
Date:   Mon, 9 Jan 2023 10:40:37 +0800
From:   "mengyuanlou@...-swift.com" <mengyuanlou@...-swift.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, jiawenwu@...-swift.com
Subject: Re: [PATCH net-next v6] net: ngbe: Add ngbe mdio bus driver.



> 2023年1月9日 00:00，Andrew Lunn <andrew@...n.ch> 写道：
> 
>> +static int ngbe_phy_read_reg_internal(struct mii_bus *bus, int phy_addr, int regnum)
>> +{
>> + struct wx *wx = bus->priv;
>> +
>> + if (regnum & MII_ADDR_C45)
>> + return -EOPNOTSUPP;
>> + return (u16)rd32(wx, NGBE_PHY_CONFIG(regnum));
> 
> You ignore phy_addr. Which suggests you only allow one internal
> PHY. Best practice here is to put the internal PHY on phy_addr 0, and
> return 0xffff for all other phy_addr values. If phylib probes the full
> range, or userspace tries to access the full range, it will look like
> there is no PHY at these other addresses.
> 
>> +}
>> +
>> +static int ngbe_phy_write_reg_internal(struct mii_bus *bus, int phy_addr, int regnum, u16 value)
>> +{
>> + struct wx *wx = bus->priv;
>> +
>> + if (regnum & MII_ADDR_C45)
>> + return -EOPNOTSUPP;
>> + wr32(wx, NGBE_PHY_CONFIG(regnum), value);
>> + return 0;
> 
> Here, silently ignore writes to phy_addr != 0.
> 
>> + /* wait to complete */
>> + ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
>> + 100000, false, wx, NGBE_MSCC);
>> + if (ret) {
>> + wx_err(wx, "PHY address command did not complete.\n");
>> + return ret;
>> + }
>> +
>> + return (u16)rd32(wx, NGBE_MSCC);
>> +}
>> +
>> + /* wait to complete */
>> + ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
>> + 100000, false, wx, NGBE_MSCC);
>> + if (ret)
>> + wx_err(wx, "PHY address command did not complete.\n");
> 
> You have the exact same error message. When you see such an error in
> the log, it can sometimes be useful to know was it a read or a write
> which failed. So i would suggest you put read/write into the message.
> 
>> +static void ngbe_phy_fixup(struct wx *wx)
>> +{
>> + struct phy_device *phydev = wx->phydev;
>> + struct ethtool_eee eee;
>> +
>> + if (wx->mac_type != em_mac_type_mdi)
>> + return;
> 
> Does this mean that if using the internal PHY the MAC does support EEE
> and half duplex?


1) The MAC does not support half duplex. 
   Internal phy and external phys all need to close half duplex.

2) The internal phy does not support eee. 
   When using the internal phy, we disable eee.
> 
>> + /* disable EEE, EEE not supported by mac */
>> + memset(&eee, 0, sizeof(eee));
>> + phy_ethtool_set_eee(phydev, &eee);
>> +
>> + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
>> + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
>> + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
>> +}
>> +
>> +int ngbe_mdio_init(struct wx *wx)
>> +{
>> + struct pci_dev *pdev = wx->pdev;
>> + struct mii_bus *mii_bus;
>> + int ret;
>> +
>> + mii_bus = devm_mdiobus_alloc(&pdev->dev);
>> + if (!mii_bus)
>> + return -ENOMEM;
>> +
>> + mii_bus->name = "ngbe_mii_bus";
>> + mii_bus->read = ngbe_phy_read_reg;
>> + mii_bus->write = ngbe_phy_write_reg;
>> + mii_bus->phy_mask = GENMASK(31, 4);
>> + mii_bus->probe_capabilities = MDIOBUS_C22_C45;
> 
> That is not strictly true. The internal MDIO bus does not suport
> C45. In practice, it probably does not matter.

>> mii_bus->probe_capabilities = MDIOBUS_C22_C45;
So, it is not necessary?
If I correct handling in read/write.
> 
>     Andrew
>