netdev - Re: [PATCH] lan743x: Added fixed link support

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: <EB9FB222-D08A-464F-93C0-5885C010D582@berg-solutions.de>
Date:   Mon, 18 May 2020 21:31:00 +0200
From:   Roelof Berg <rberg@...g-solutions.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Bryan Whitehead <bryan.whitehead@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lan743x: Added fixed link support

Hi,

thanks a lot for going into detail. I also want to make sure that everything will be right.

> Am 18.05.2020 um 01:50 schrieb Andrew Lunn <andrew@...n.ch>:
> 
>>>> +			/* Configure MAC to fixed link parameters */
>>>> +			data = lan743x_csr_read(adapter, MAC_CR);
>>>> +			/* Disable auto negotiation */
>>>> +			data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
>>> 
>>> Why does the MAC care about autoneg? In general, all the MAC needs to
>>> know is the speed and duplex.
>>> 
>> 
> 
>> My assumption is, that in fixed-link mode we should switch off the
>> autonegotiation between MAC and remote peer (e.g. a switch). I
>> didn’t test, if it would also wun with the hardware doing
>> auto-negotiation, however it feels cleaner to me to prevent the
>> hardware from initiating any auto-negotiation in fixed-link mode.
> 
> The MAC is not involved in autoneg. autoneg is between two PHYs. They
> talk with each other, and then phylibs sees the results and tells the
> MAC the results of the negotiation. That happens via this call
> back. So i have no idea what this is doing in general in the MAC. And
> in your setup, you don't have any PHYs at all. So there is no
> auto-neg. You should read the datasheet and understand what this is
> controlling. It might need to be disabled in general.

Thanks for making sure we’re doing things right.

I double checked the vendor documentation and according to the data sheet in this device the MAC detects speed and duplex mode. It uses PINs, traces clocks … Also according to an application note of the vendor duplex and speed detection should be enabled in the MAC registers. The current driver version (which is not fixed-link capable) does this. However, in a fixed-link scenario I don’t think that the autodetection, that the vendor recommends to turn on, of speed and duplex would solve problems, it rather likely introduces problems when the auto-detection from the MAC (yes, the MAC) yields different results than configured in the device tree.

So I think we should:
a) Keep the behavior to activate auto detection in the MAC in normal cases, as recommended by the data sheet. (And ensure backwards compatibility this way as well.)
b) But add the behavior to deactivate this kind of MAC auto detection in fixed link cases.

I found no documentation for fixed link operation in the data sheets, so a statement from the Vendor could give us higher confidence here. Unfortunately I have no access to the Vendor’s specialists (also not via the Microchip customer support for some reasons), but I think the vendor is on CC on this thread ;)

> 
>> Using get_phy_mode() in all cases is not possible on a PC as it
>> returns SGMII on a standard PC.
> 
> Why do you think that?

printk made me think so :) But I printk’ed without checking the error return value, so that was maybe just invalid or even random data.

> 
>>> I don't understand this comment.
>>> 
>> 
>> See above the lengthy section. On a PC SGMII is returned when I call of_get_phy_mode(phynode, &phyifc);
> 
> There are two things possible here:
> 
> A PC has no OF support, so you are using:
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/of_net.h#L19
> 
> So you get the error code -ENODEV, and phyifc is not changed.
> 
> Or you are using:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/of/of_net.c#L25
> 
> There is unlikely to be a device node, so phyifc is set to
> PHY_INTERFACE_MODE_NA and -ENODEV is returned.
> 
> So if of_get_phy_mode() returns an error, use RMII. Otherwise use what
> value it set phyifc to.
> 
>      Andrew
> 

Ok, consider it done, thanks :)