netdev - Re: [PATCH 2/2] at803x: double check SGMII side autoneg

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:   Thu, 19 Jan 2017 20:38:15 -0600
From:   Timur Tabi <timur@...eaurora.org>
To:     Zefir Kurtisi <zefir.kurtisi@...atec.com>, netdev@...r.kernel.org
Cc:     andrew@...n.ch, f.fainelli@...il.com
Subject: Re: [PATCH 2/2] at803x: double check SGMII side autoneg

Zefir Kurtisi wrote:
> It always operates at 675MHz, which with two lines gives 1.25Gbps,
> which at 10/8 coding gives exactly 1Gbps net data rate. If the
> at803x's copper side autonegotiates to 1Gbps, the bits traversing
> over the SGMII match the copper side 1:1. In case the copper side
> autonegotiates to e.g. 100Mbps, each bit at the copper side on the
> SGMII bus is replicated and sent 10x times - or 100x times in case of
> 10Mbps. The MAC side of the ETH needs to be aware of how the SGMII
> data has to be interpreted, and this is why you have to set the bits
> you are referring to.

So does this mean that the SGMII link should not be autonegotiated? I 
currently have this code:

     if (phydev->autoneg == AUTONEG_ENABLE) {
         val &= ~(FORCE_AN_RX_CFG | FORCE_AN_TX_CFG);
         val |= AN_ENABLE;
         writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
     } else {
         ...

So if the external PHY is set to autonegotiate, then the SGMII block is 
set to also negotiate.  Now that I think about it, this seems wrong. 
And in fact, I'm not sure how it works.  It seems that the this only 
makes sense if the SGMII block is configured to act as the only PHY. 
This is an option that the hardware supports but my driver does not.  So 
perhaps I should remove this part, and just do the rest:


	u32 speed_cfg;

	switch (phydev->speed) {
	case SPEED_10:
		speed_cfg = SPDMODE_10;
		break;
	case SPEED_100:
		speed_cfg = SPDMODE_100;
		break;
	case SPEED_1000:
		speed_cfg = SPDMODE_1000;
		break;
	default:
		return -EINVAL;
	}

	if (phydev->duplex == DUPLEX_FULL)
		speed_cfg |= DUPLEX_MODE;

	val &= ~AN_ENABLE;
	writel(speed_cfg, phy->base + EMAC_SGMII_PHY_SPEED_CFG1);
	writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);

Should I be doing this all the time?

> To track down who is causing the additional message, I would proceed
> with following technique that helped me dig down a similar problem:
> since you control the events in question and there is no risk of
> flooding the kernel log, in the top of phy.c::phy_print_status() add
> a dump_stack() call. In the debug log ensure that all of the traces
> end up in the same phydev->adjust_link() callback handler (in your
> case emac_adjust_link()).

That's a good idea, thanks.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.