linux-kernel - Re: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY interface

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: <20190624093533.4vhvjmqqrucq2ixf@shell.armlinux.org.uk>
Date:   Mon, 24 Jun 2019 10:35:33 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Parshuram Raju Thombare <pthombar@...ence.com>
Cc:     "andrew@...n.ch" <andrew@...n.ch>,
        "nicolas.ferre@...rochip.com" <nicolas.ferre@...rochip.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rafal Ciepiela <rafalc@...ence.com>,
        Anil Joy Varughese <aniljoy@...ence.com>,
        Piotr Sroka <piotrs@...ence.com>
Subject: Re: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY interface

On Mon, Jun 24, 2019 at 06:35:44AM +0000, Parshuram Raju Thombare wrote:
> 
> >> +	if (change_interface) {
> >> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> >> +			gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) &
> >> +				   ~GEM_BIT(PCSSEL) &
> >> +				   gem_readl(bp, NCFGR));
> >> +			gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
> >> +				   gem_readl(bp, NCR));
> >> +			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
> >> +				   GEM_BIT(PCS_CTRL_RST));
> >> +		}
> >I still don't think this makes much sense, splitting the interface
> >configuration between here and below.
> Do you mean splitting mac_config in two *_configure functions ?
> This was done as per Andrew's suggestion to make code mode readable
> and easy to manage by splitting MAC configuration for different interfaces.

No, I mean here you disable SGMII if we're switching away from SGMII
mode.... (note, this means there is more to come for this sentence)

> 
> >> +		bp->phy_interface = state->interface;
> >> +	}
> >> +
> >>  	if (!phylink_autoneg_inband(mode) &&
> >>  	    (bp->speed != state->speed ||
> >> -	     bp->duplex != state->duplex)) {
> >> +	     bp->duplex != state->duplex ||
> >> +	     change_interface)) {
> >>  		u32 reg;
> >>
> >>  		reg = macb_readl(bp, NCFGR);
> >>  		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> >>  		if (macb_is_gem(bp))
> >>  			reg &= ~GEM_BIT(GBE);
> >> +		macb_or_gem_writel(bp, NCFGR, reg);
> >> +
> >> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
> >> +			gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
> >> +				   GEM_BIT(PCSSEL) |
> >> +				   gem_readl(bp, NCFGR));
> >This will only be executed when we are not using inband mode, which
> >basically means it's not possible to switch to SGMII in-band mode.
> SGMII is used in default PHY mode. And above code is to program MAC to 
> select PCS and SGMII interface.

... and here you enable it for SGMII mode, but only for non-inband
modes.

For inband modes, you do not have any code that enables SGMII mode.
Since the only inband mode you support is SGMII, this is not very
good behaviour.

Why not:

	if (change_interface) {
		u32 ncfgr;

		bp->phy_interface = state->interface;

		// We don't support 2.5G modes
		gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
			   gem_readl(bp, NCR));

		ncfgr = gem_readl(bp, NCFGR);
		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
			// Enable SGMII mode and PCS
			gem_writel(bp, NCFGR, ncfgr | GEM_BIT(SGMIIEN) |
				   GEM_BIT(PCSSEL));
		} else {
			// Disable SGMII mode and PCS
			gem_writel(bp, NCFGR, ncfgr & ~(GEM_BIT(SGMIIEN) |
				   GEM_BIT(PCSSEL)));

			// Reset PCS
			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
				   GEM_BIT(PCS_CTRL_RST));
		}
	}

	if (!phylink_autoneg_inband(mode) &&
	    (bp->speed != state->speed || bp->duplex != state->duplex)) {

?

> 
> >> +
> >> +		if (!interface_supported) {
> >> +			netdev_err(dev, "Phy mode %s not supported",
> >> +				   phy_modes(phy_mode));
> >> +			goto err_out_free_netdev;
> >> +		}
> >> +
> >>  		bp->phy_interface = phy_mode;
> >> +	} else {
> >> +		bp->phy_interface = phy_mode;
> >> +	}
> >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and mac_config()
> >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then
> >mac_config() won't configure the MAC for the interface type - is that
> >intentional?
> 
> In mac_config configure MAC for non in-band mode, there is also check for speed, duplex
> changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN and DUPLEX_UNKNOWN
> values so it is expected that for non in band mode state contains valid speed and duplex mode
> which are different from *_UNKNOWN values.

Sorry, this reply doesn't answer my question.  I'm not asking about
bp->speed and bp->duplex.  I'm asking:

1) why you are initialising bp->phy_interface here
2) you to consider the impact that has on the mac_config() implementation
   you are proposing

because I think it's buggy.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up