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: <Y7wSZJC3F5liYvTe@lunn.ch>
Date:   Mon, 9 Jan 2023 14:11:00 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     "mengyuanlou@...-swift.com" <mengyuanlou@...-swift.com>
Cc:     netdev@...r.kernel.org, jiawenwu@...-swift.com
Subject: Re: [PATCH net-next v6] net: ngbe: Add ngbe mdio bus driver.

> >> +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.

So this condition is wrong and need deleting?

> >> +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.

mii_bus->probe_capabilities controls how the MDIO bus is probed for
devices. MDIOBUS_C22_C45 means it will first look for C22 devices, and
then look for C45 devices. One of your busses does not support C45,
and will always return -EOPNOTSUPP. So it is just a waste of time
probing that bus for C45 devices. But it will not break, which is why
i said it probably does not matter. You could if you wanted set
mii_bus->probe_capabilities based on which bus is being used, internal
or external, and that might speed up the driver loading a little.

	Andrew