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
