netdev - RE: [PATCH RFC 4/6] dpaa2-mac: add initial 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]

Message-ID: <VI1PR0402MB2800B4186EE51AE54149AF33E0EE0@VI1PR0402MB2800.eurprd04.prod.outlook.com>
Date:   Fri, 14 Jun 2019 14:08:58 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>
Subject: RE: [PATCH RFC 4/6] dpaa2-mac: add initial driver

> Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
> 
> > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if) {
> > +	switch (eth_if) {
> > +	case DPMAC_ETH_IF_RGMII:
> > +		return PHY_INTERFACE_MODE_RGMII;
> 
> So the MAC cannot insert RGMII delays? I didn't see anything in the PHY object
> about configuring the delays. Does the PCB need to add delays via squiggles in
> the tracks?
> 
> > +static void dpaa2_mac_validate(struct phylink_config *config,
> > +			       unsigned long *supported,
> > +			       struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > +	phylink_set(mask, Autoneg);
> > +	phylink_set_port_modes(mask);
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_10GKR:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;
> > +	default:
> > +		goto empty_set;
> > +	}
> 
> I think this is wrong. This is about validating what the MAC can do. The state-
> >interface should not matter. The PHY will indicate what interface mode should
> be used when auto-neg has completed. The MAC is then expected to change its
> interface to fit.
> 
> But lets see what Russell says.

We cannot do reconfiguration of the interface mode at runtime.
The SERDES speaks an ethernet/sata/pcie  coding that is configurable at reset time.

> 
> > +static void dpaa2_mac_config(struct phylink_config *config, unsigned int
> mode,
> > +			     const struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	struct device *dev = &priv->mc_dev->dev;
> > +	int err;
> > +
> > +	if (state->speed == SPEED_UNKNOWN && state->duplex ==
> DUPLEX_UNKNOWN)
> > +		return;
> > +
> > +	dpmac_state->up = !!state->link;
> > +	if (dpmac_state->up) {
> > +		dpmac_state->rate = state->speed;
> > +
> > +		if (!state->duplex)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_HALF_DUPLEX;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > +
> > +		if (state->an_enabled)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_AUTONEG;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_AUTONEG;
> 
> As Russell pointed out, this auto-neg is only valid in a limited context. The MAC
> generally does not perform auto-neg. The MAC is only involved in auto-neg
> when inband signalling is used between the MAC and PHY in 802.3z.
> 
> As the name says, dpaa2_mac_config is about the MAC.
> 
>    Andrew

Yes, the dpaa2_mac_config should take care of only the MAC but, in this case, we cannot convey
piecemeal link state information through the firmware to the Ethernet driver - it has to come all at once.

--
Ioana