netdev - Re: [PATCH net-next v4 6/6] net: tn40xx: add PHYLIB 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] [day] [month] [year] [list]

Message-Id: <20240512.142001.791439237151047486.fujita.tomonori@gmail.com>
Date: Sun, 12 May 2024 14:20:01 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, kuba@...nel.org,
 jiri@...nulli.us, horms@...nel.org
Subject: Re: [PATCH net-next v4 6/6] net: tn40xx: add PHYLIB support

Hi,

On Thu, 9 May 2024 15:37:55 +0200
Andrew Lunn <andrew@...n.ch> wrote:

> On Thu, May 09, 2024 at 01:23:41PM +0900, FUJITA Tomonori wrote:
>> Hi,
>> 
>> On Wed, 08 May 2024 22:18:51 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@...il.com> wrote:
>> 
>> >>>  		priv->link = 0;
>> >>>  		if (priv->link_loop_cnt++ > TN40_LINK_LOOP_MAX) {
>> >>>  			/* MAC reset */
>> >>>  			tn40_set_link_speed(priv, 0);
>> >>> +			tn40_set_link_speed(priv, priv->speed);
>> >>>  			priv->link_loop_cnt = 0;
>> >> 
>> >> This should move into the link_down callback.
>> > 
>> > I'll try phylink callbacks to see if they would work. 
>> 
>> I found that the link_down callback doesn't work well for the MAC
>> reset above.
>> 
>> Currently, when TN40_REG_MAC_LNK_STAT register tells that the link is
>> off, the driver configures the MAC to generate an interrupt
>> periodically; tn40_write_reg(priv, 0x5150, 1000000) is called in
>> tn40_link_changed().
>> 
>> Eventually, the counter is over TN40_LINK_LOOP_MAX and then the driver
>> executes the MAC reset. Without the MAC reset, the NIC will not work.
>> 
>> The link_down callback is called only when the link becomes down so it
>> can't be used to trigger the MAC reset.
> 
> So this sounds like a hardware bug workaround.

Yeah, looks so.

> But it might also be to do with auto-neg. The MAC PCS/SERDES and the
> PHY PCS/SERDES, depending on the mode, should be performing auto-neg,
> to indicate things like pause. For some hardware, you need to restart
> autoneg when the line sides gets link. It could be this hardware has
> no way to do that, other than hit the whole thing with a reset?

I can't find a function to do such in the original driver.


> Take a look at struct phylink_pcs_ops and see if you can map bits of
> the driver to this structure. It might be you can implement a PCS, and
> have the pcs_an_restart do the MAC reset.

I can't find anything that checks the link periodically until the link
is recovered.

I dropped the workaround and does a reset every time the link is down
(mac_link_down). Seems that it works.