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: <20181214082313.GA3703@lunn.ch>
Date:   Fri, 14 Dec 2018 09:23:13 +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

> Hi,
> 
> > 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.
> 
> Without the custom config_aneg which sets speed and duplex, I get a
> report claiming the link is at 10/Half , while the link is at 100/Full.
> If I force this in the custom config_aneg, the communication works fine.
> Do you have a hint for me ?

I can make some guesses, but i've never used a PHY which does not have
auto-neg.

If the config_aneg function is being called, i wonder if
AUTONEG_DISABLE == phydev->autoneg is true? 

Is the read_status function doing the right thing? Does it set the
speed, duplex and link up? I'm not sure that is needed, it looks like
phy_sanitize_settings() should do that. 

> Oh, nice. Shouldn't this be basic_t1 , which is already supported ?

I'm getting the various T mixed up. Yes, basic_t1.

> > 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.
> 
> Do you mind if I skip this part for now , until I get the driver into
> better shape ?

For the moment, we can postpone that. Lets get the basics working.
What i'm slightly worried about is this could be an ABI change, and
break older systems. If things work correctly such that T1 is selected
without userspace being involved, no need to set the link parameters,
then we are probably safe. We can also ask Microchip to test there T1
driver to make sure it still works.

       Andrew