netdev - Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY 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: <20181213143352.GA12808@lunn.ch>
Date:   Thu, 13 Dec 2018 15:33:52 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Marek Vasut <marex@...x.de>
Cc:     netdev@...r.kernel.org, f.fainelli@...il.com
Subject: Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

On Thu, Dec 13, 2018 at 02:27:51PM +0100, Marek Vasut wrote:
> On 12/13/2018 09:38 AM, Andrew Lunn wrote:
> >> +static int tja11xx_config_aneg(struct phy_device *phydev)
> >> +{
> >> +	phydev->autoneg = 0;
> >> +	phydev->speed = SPEED_100;
> >> +	phydev->duplex = DUPLEX_FULL;
> >> +	phydev->pause = 0;
> >> +	phydev->asym_pause = 0;
> >> +
> >> +	return 0;
> >> +}
> > 
> > Hi Marek
> 
> Hi,
> 
> > That is, err, interesting. Are you saying the PHY cannot do auto-neg?
> 
> There isn't much to auto-negotiate and some of the bits in the standard
> registers behave weirdly.

Hi Marek

Hopefully the config_aneg callback is not called if you don't list
autoneg to the .features. The microchip_t1 driver just uses
genphy_config_aneg, but if a NULL works, i would prefer that.

> > Does it happen to be a 100T1 device for automotive?
> 
> Yes, that's what the commit message says, why ?

Sorry, i missed it in the commit message.  We now have enough bits in
the link_mode that we should explicitly say this is 100BaseT2_Full,
not 100BaseT_Full. So please add a new member to
ethtool_link_mode_bit_indices ...

> >> +
> >> +static int tja11xx_config_init(struct phy_device *phydev)
> >> +{
> >> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;

And make it part of the .features. Well, you need to add a new
PHY_BASIC_T2_FEATURES macro, which uses it, and use that in the
phy_driver structure. There should not be any need to modify it in the
config_init call.

We also need to think about what we do with the PHY_BASIC_T1_FEATURES
macro. Ideally we want to swap that to also make use of a new
ethtool_link_mode_bit_indices, but i've no idea at the moment if that
will break something.

  Andrew