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]

Date:   Fri, 14 Dec 2018 16:44:14 +0100
From:   Marek Vasut <marex@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, f.fainelli@...il.com
Subject: Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

On 12/14/2018 09:23 AM, Andrew Lunn wrote:
>> 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. 

The read_status just changes the link, nothing else.

But, I dropped all the aneg stuff and that seems to do the right thing,
so I now have a fixed 100/Full link. I think I'll just submit a V2,
since the patch changed a lot.

>> Oh, nice. Shouldn't this be basic_t1 , which is already supported ?
> 
> I'm getting the various T mixed up. Yes, basic_t1.

Hmmm, is there a 10/Full variant of the 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.

Fine

-- 
Best regards,
Marek Vasut