netdev - RE: [PATCH net-next 3/4] dpaa2-eth: add MAC/PHY support through phylink

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:   Tue, 22 Oct 2019 12:08:44 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "rmk@...linux.org.uk" <rmk@...linux.org.uk>
Subject: RE: [PATCH net-next 3/4] dpaa2-eth: add MAC/PHY support through
 phylink

> Hi Ioana
> 

Hi Andrew

> > +static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv) {
> > +	struct fsl_mc_device *dpni_dev, *dpmac_dev;
> > +	struct dpaa2_mac *mac;
> > +	int err;
> > +
> > +	dpni_dev = to_fsl_mc_device(priv->net_dev->dev.parent);
> > +	dpmac_dev = fsl_mc_get_endpoint(dpni_dev);
> > +	if (!dpmac_dev || dpmac_dev->dev.type !=
> &fsl_mc_bus_dpmac_type)
> > +		return 0;
> > +
> > +	if (dpaa2_mac_is_type_fixed(dpmac_dev, priv->mc_io))
> > +		return 0;
> > +
> > +	mac = kzalloc(sizeof(struct dpaa2_mac), GFP_KERNEL);
> > +	if (!mac)
> > +		return -ENOMEM;
> > +
> > +	mac->mc_dev = dpmac_dev;
> > +	mac->mc_io = priv->mc_io;
> > +	mac->net_dev = priv->net_dev;
> > +
> > +	err = dpaa2_mac_connect(mac);
> > +	if (err) {
> > +		netdev_err(priv->net_dev, "Error connecting to the MAC
> endpoint\n");
> > +		kfree(mac);
> > +		return err;
> > +	}
> > +	priv->mac = mac;
> > +
> > +	return 0;
> > +}
> > +
> > +static void dpaa2_eth_disconnect_mac(struct dpaa2_eth_priv *priv) {
> > +	if (!priv->mac)
> > +		return;
> > +
> > +	rtnl_lock();
> > +	dpaa2_mac_disconnect(priv->mac);
> > +	kfree(priv->mac);
> > +	priv->mac = NULL;
> > +	rtnl_unlock();
> > +}
> 
> dpaa2_eth_connect_mac() does not take the rtnl lock.
> dpaa2_eth_disconnect_mac() does. This asymmetry makes me think
> something is wrong. But it could be correct....

It seems that phylink_of_phy_connect() does not do an ASSERT_RTNL() as phylink_disconnect_phy() does and that is why I didn't catch this.

Should I submit a patch adding the assert in phylink_of_phy_connect() and  phylink_of_phy_connect() ?
Also, I'll fix the asymmetry in the dpaa2-eth in v2.

> 
> > +/* Caller must call of_node_put on the returned value */ static
> > +struct device_node *dpaa2_mac_get_node(u16 dpmac_id) {
> > +	struct device_node *dpmacs, *dpmac = NULL;
> > +	u32 id;
> > +	int err;
> > +
> > +	dpmacs = of_find_node_by_name(NULL, "dpmacs");
> > +	if (!dpmacs)
> > +		return NULL;
> > +
> > +	while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
> > +		err = of_property_read_u32(dpmac, "reg", &id);
> > +		if (err)
> > +			continue;
> > +		if (id == dpmac_id)
> > +			break;
> > +	}
> 
> of_get_next_child() takes a reference on the child. So you need to release
> that reference. It is better to make use of something like
> for_each_child_of_node() or for_each_available_child_of_node() which
> release the reference at the end of each loop, so long as you don't
> break/return out of the loop.

It seems that of_get_next_child() also releases the reference took on the
previous node in each iteration of the loop. At the end of this function,
the only reference still held is the one to the node to be returned which
is ok because the caller should release it after usage.
( A function comment is added to specify this.)

Am I missing something here?

> 
> > +
> > +static void dpaa2_mac_validate(struct phylink_config *config,
> > +			       unsigned long *supported,
> > +			       struct phylink_link_state *state) {
> > +	struct dpaa2_mac *mac = phylink_to_dpaa2_mac(config);
> > +	struct dpmac_link_state *dpmac_state = &mac->state;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > +	if (state->interface != PHY_INTERFACE_MODE_NA &&
> > +	    dpaa2_mac_phy_mode_mismatch(mac, state->interface)) {
> > +		goto empty_set;
> > +	}
> > +
> > +	phylink_set_port_modes(mask);
> > +	phylink_set(mask, Autoneg);
> > +	phylink_set(mask, Pause);
> > +	phylink_set(mask, Asym_Pause);
> > +
> > +	switch (state->interface) {
> > +	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;
> > +	default:
> > +		goto empty_set;
> > +	}
> > +
> > +	linkmode_and(supported, supported, mask);
> > +	linkmode_and(state->advertising, state->advertising, mask);
> > +
> > +	dpaa2_mac_linkmode2dpmac(supported, &dpmac_state-
> >supported);
> > +	dpaa2_mac_linkmode2dpmac(state->advertising,
> > +&dpmac_state->advertising);
> 
> Humm. Not sure about these last two lines. Validate should be about if the
> MAC can support something. I don't think you should be setting any state
> here. That should happen in mac_config, when the state really is configured.
> 

I agree with the fact that .validate() is not the best place to setup these. 
Those two lines are there more because the API requests a proper supported/advertised (which had a specific purpose in the previous solution with two separate drivers).
I'll check if we can just omit them.

Thanks,
Ioana

> > +
> > +	return;
> > +
> > +empty_set:
> > +	linkmode_zero(supported);
> > +}
> > +
> 
>   Andrew