Re: [RFC PATCH v2 net-next 15/15] net: pcs: lynx: use MTIP AN/LT block for copper backplanes

[Russell King (Oracle)]

On Sat, Sep 23, 2023 at 04:49:04PM +0300, Vladimir Oltean wrote:
> +static int lynx_pcs_parse_fwnode(struct lynx_pcs *lynx)
> +{
> +	struct fwnode_handle *node = lynx->mdio->dev.fwnode;
> +	enum mtip_model model = MTIP_MODEL_AUTODETECT;
> +	struct device_node *np = to_of_node(node);
> +	struct mdio_device *mdio = lynx->mdio;
> +	struct device *dev = &mdio->dev;
> +	struct phy *phy;
> +	int i, err;
> +
> +	if (!node)
> +		return 0;
> +
> +	lynx->backplane_mode = fwnode_property_present(node, "fsl,backplane-mode");
> +	if (!lynx->backplane_mode)
> +		return 0;
> +
> +	if (fwnode_device_is_compatible(node, "fsl,lx2160a-lynx-pcs"))
> +		model = MTIP_MODEL_LX2160A;
> +
> +	lynx->num_lanes = of_count_phandle_with_args(np, "phys", "#phy-cells");
> +	if (lynx->num_lanes < 0)
> +		return lynx->num_lanes;

Is it possible for ->num_lanes to be zero at this point? If that is
possible, then ->anlt[PRIMARY_LANE] will be NULL but ->backplane_mode
will be set, so won't that cause the mtip_* calls above to pass a
NULL pointer into those functions? Is that safe? Should we trap that
case here?

If that's correct, then I don't see any point in storing
->backplane_mode, since we can then use ->num_lanes > PRIMARY_LANE
or similar instead.

> +
> +	if (WARN_ON(lynx->num_lanes > MAX_NUM_LANES))
> +		return -EINVAL;

Do we need to use WARN_ON() here, or would it be better to print a short
error-level message?

> +
> +	for (i = 0; i < lynx->num_lanes; i++) {
> +		phy = devm_of_phy_get_by_index(dev, np, i);
> +		if (IS_ERR(phy))
> +			return dev_err_probe(dev, PTR_ERR(phy),
> +					     "Failed to get SerDes PHY %d\n", i);
> +
> +		lynx->anlt[i] = mtip_backplane_create(mdio, phy, model);
> +		if (IS_ERR(lynx->anlt[i])) {
> +			err = PTR_ERR(lynx->anlt[i]);
> +
> +			while (i-- > 0)
> +				mtip_backplane_destroy(lynx->anlt[i]);
> +
> +			return err;
> +		}
> +	}
> +
> +	for (i = 1; i < lynx->num_lanes; i++) {
> +		err = mtip_backplane_add_subordinate(lynx->anlt[PRIMARY_LANE],
> +						     lynx->anlt[i]);
> +		if (WARN_ON(err)) {

Again, does this need to be a backtrace-producing WARN_ON()?

> +			/* Too many SerDes lanes in the device tree? */
> +			for (i = 0; i < lynx->num_lanes; i++)
> +				mtip_backplane_destroy(lynx->anlt[i]);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
>  {
>  	struct lynx_pcs *lynx;
> +	int err;
>  
>  	lynx = kzalloc(sizeof(*lynx), GFP_KERNEL);
>  	if (!lynx)
> @@ -327,6 +451,12 @@ static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
>  	lynx->pcs.neg_mode = true;
>  	lynx->pcs.poll = true;
>  
> +	err = lynx_pcs_parse_fwnode(lynx);
> +	if (err) {
> +		kfree(lynx);
> +		return ERR_PTR(err);
> +	}
> +
>  	return lynx_to_phylink_pcs(lynx);
>  }
>  
> @@ -392,6 +522,11 @@ EXPORT_SYMBOL_GPL(lynx_pcs_create_fwnode);
>  void lynx_pcs_destroy(struct phylink_pcs *pcs)
>  {
>  	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
> +	int i;
> +
> +	if (lynx->backplane_mode)
> +		for (i = 0; i < lynx->num_lanes; i++)
> +			mtip_backplane_destroy(lynx->anlt[i]);

Won't ->num_lanes only be non-zero when ->backplane_mode is set, so
isn't the test for ->backplane_mode redundant here?

>  
>  	mdio_device_put(lynx->mdio);
>  	kfree(lynx);
> -- 
> 2.34.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!