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]
Date:   Thu, 15 Aug 2019 18:34:11 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Christian Herber <christian.herber@....com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem

On 15.08.2019 17:56, Andrew Lunn wrote:
> On Thu, Aug 15, 2019 at 03:32:29PM +0000, Christian Herber wrote:
>> BASE-T1 is a category of Ethernet PHYs.
>> They use a single copper pair for transmission.
>> This patch add basic support for this category of PHYs.
>> It coveres the discovery of abilities and basic configuration.
>> It includes setting fixed speed and enabling auto-negotiation.
>> BASE-T1 devices should always Clause-45 managed.
>> Therefore, this patch extends phy-c45.c.
>> While for some functions like auto-neogtiation different registers are
>> used, the layout of these registers is the same for the used fields.
>> Thus, much of the logic of basic Clause-45 devices can be reused.
>>
>> Signed-off-by: Christian Herber <christian.herber@....com>
>> ---
>>  drivers/net/phy/phy-c45.c    | 113 +++++++++++++++++++++++++++++++----
>>  drivers/net/phy/phy-core.c   |   4 +-
>>  include/uapi/linux/ethtool.h |   2 +
>>  include/uapi/linux/mdio.h    |  21 +++++++
>>  4 files changed, 129 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index b9d4145781ca..9ff0b8c785de 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -8,13 +8,23 @@
>>  #include <linux/mii.h>
>>  #include <linux/phy.h>
>>  
>> +#define IS_100BASET1(phy) (linkmode_test_bit( \
>> +			   ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
>> +			   (phy)->supported))
>> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
>> +			    ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
>> +			    (phy)->supported))
> 
> Hi Christian
> 
> We already have the flag phydev->is_gigabit_capable. Maybe add a flag
> phydev->is_t1_capable
> 
>> +
>> +static u32 get_aneg_ctrl(struct phy_device *phydev);
>> +static u32 get_aneg_stat(struct phy_device *phydev);
> 
> No forward declarations please. Put the code in the right order so
> they are not needed.
> 
> Thanks
> 
>      Andrew
> 

For whatever reason I don't have the original mail in my netdev inbox (yet).

+	if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
+		ctrl = MDIO_AN_BT1_CTRL;

Code like this could be problematic once a PHY supports one of the T1 modes
AND normal modes. Then normal modes would be unusable.

I think this scenario isn't completely hypothetical. See the Aquantia
AQCS109 that supports normal modes and (proprietary) 1000Base-T2.

Maybe we need separate versions of the generic functions for T1.
Then it would be up to the PHY driver to decide when to use which
version.

Heiner

Powered by blists - more mailing lists