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] [day] [month] [year] [list]

Date:   Mon, 9 Jan 2023 22:38:36 +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日 21:11，Andrew Lunn <andrew@...n.ch> 写道：
> 
>>>> +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?

The condition is only used to dis eee for internal phy.
I will fix it. Thanks.
> 
>>>> +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