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

[Andrew Lunn <andrew@lunn.ch>]

Hi Ioana

> +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....

> +/* 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.

> +
> +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.

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

  Andrew