Re: [RFC PATCH v2 net-next 14/15] net: pcs: mtip_backplane: add driver for MoreThanIP backplane AN/LT core

[Simon Horman]

On Sat, Sep 23, 2023 at 04:49:03PM +0300, Vladimir Oltean wrote:

...

> +static int mtip_rx_c72_coef_update(struct mtip_backplane *priv,
> +				   struct c72_coef_update *upd,
> +				   bool *rx_ready)
> +{
> +	char upd_buf[C72_COEF_UPDATE_BUFSIZ], stat_buf[C72_COEF_STATUS_BUFSIZ];
> +	struct device *dev = &priv->mdiodev->dev;
> +	struct c72_coef_status stat = {};
> +	int err, val;
> +
> +	err = read_poll_timeout(mtip_read_lt_lp_coef_if_not_ready,
> +				val, val < 0 || *rx_ready || LT_COEF_UPD_ANYTHING(val),
> +				MTIP_COEF_STAT_SLEEP_US, MTIP_COEF_STAT_TIMEOUT_US,
> +				false, priv, rx_ready);
> +	if (val < 0)
> +		return val;
> +	if (*rx_ready) {
> +		if (!priv->any_request_received)
> +			dev_warn(dev,
> +				 "LP says its RX is ready, but there was no coefficient request (LP_STAT = 0x%x, LD_STAT = 0x%x)\n",
> +				 mtip_read_lt(priv, LT_LP_STAT),
> +				 mtip_read_lt(priv, LT_LD_STAT));
> +		else
> +			dev_dbg(dev, "LP says its RX is ready\n");
> +		return 0;
> +	}
> +	if (err) {
> +		dev_err(dev,
> +			"LP did not request coefficient updates; LP_COEF = 0x%x\n",
> +			val);
> +		return err;
> +	}
> +
> +	upd->com1 = LT_COM1_X(val);
> +	upd->coz = LT_COZ_X(val);
> +	upd->cop1 = LT_COP1_X(val);
> +	upd->init = !!(val & LT_COEF_UPD_INIT);
> +	upd->preset = !!(val & LT_COEF_UPD_PRESET);
> +	

Hi Vladimir,

I'm unsure if this can actually happen.
But if the while loop runs zero times then err is used uninitialised here.

As flagged by Smatch.

> +		mtip_an_restart_from_lt(priv);
> +
> +	kfree(lt_work);
> +}
> +
> +/* Train the link partner TX, so that the local RX quality improves */
> +static void mtip_remote_tx_lt_work(struct kthread_work *work)
> +{
> +	struct mtip_lt_work *lt_work = container_of(work, struct mtip_lt_work,
> +						    work);
> +	struct mtip_backplane *priv = lt_work->priv;
> +	struct device *dev = &priv->mdiodev->dev;
> +	int err;
> +
> +	while (true) {
> +		struct c72_coef_status status = {};
> +		union phy_configure_opts opts = {
> +			.ethernet = {
> +				.type = C72_REMOTE_TX,
> +			},
> +		};
> +
> +		if (READ_ONCE(priv->lt_stop_request))
> +			goto out;

Likewise, I'm unsure if this can happen.
But if the condition above is met on the first iteration of
the loop then the out label will use err without it being initialised.

Also flagged by Smatch.

> +
> +		err = mtip_lt_in_progress(priv);
> +		if (err) {
> +			dev_err(dev, "Remote TX LT failed: %pe\n", ERR_PTR(err));
> +			goto out;
> +		}
> +
> +		err = phy_configure(priv->serdes, &opts);
> +		if (err) {
> +			dev_err(dev,
> +				"Failed to get remote TX training request from SerDes: %pe\n",
> +				ERR_PTR(err));
> +			goto out;
> +		}
> +
> +		if (opts.ethernet.remote_tx.rx_ready)
> +			break;
> +
> +		err = mtip_tx_c72_coef_update(priv, &opts.ethernet.remote_tx.update,
> +					      &status);
> +		if (opts.ethernet.remote_tx.cb)
> +			opts.ethernet.remote_tx.cb(opts.ethernet.remote_tx.cb_priv,
> +						   err, opts.ethernet.remote_tx.update,
> +						   status);
> +		if (err)
> +			goto out;
> +	}
> +
> +	/* Let the link partner know we're done */
> +	err = mtip_modify_lt(priv, LT_LD_STAT, LT_COEF_STAT_RX_READY,
> +			     LT_COEF_STAT_RX_READY);
> +	if (err < 0) {
> +		dev_err(dev, "Failed to update LT_LD_STAT: %pe\n",
> +			ERR_PTR(err));
> +		goto out;
> +	}
> +
> +	err = mtip_remote_tx_lt_done(priv);
> +	if (err) {
> +		dev_err(dev, "Failed to finalize remote LT: %pe\n",
> +			ERR_PTR(err));
> +		goto out;
> +	}
> +
> +out:
> +	if (err && !READ_ONCE(priv->lt_stop_request))
> +		mtip_an_restart_from_lt(priv);
> +
> +	kfree(lt_work);
> +}

...