On Mon, 2023-09-04 at 17:03 +0000, Christophe Leroy wrote:

Le 04/09/2023 à 14:31, Alexandra Diupina a écrit :

quoted

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 47c2ad7a3e42..fd999dabdd39 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c

@@ -34,6 +34,8 @@
  #define TDM_PPPOHT_SLIC_MAXIN
  #define RX_BD_ERRORS (R_CD_S | R_OV_S | R_CR_S | R_AB_S | R_NO_S | R_LG_S)
  
+static int uhdlc_close(struct net_device *dev);
+
  static struct ucc_tdm_info utdm_primary_info = {
  	.uf_info = {
  		.tsa = 0,

@@ -731,7 +733,9 @@ static int uhdlc_open(struct net_device *dev)
  		napi_enable(&priv->napi);
  		netdev_reset_queue(dev);
  		netif_start_queue(dev);
-		hdlc_open(dev);
+
+		int rc = hdlc_open(dev);

Do not mix declarations and code. Please put all declaration at the top 
of the block.

quoted

+		return rc == 0 ? 0 : (uhdlc_close(dev), rc);
  	}

That's not easy to read.

I know that's more changes, but I'd prefer something like:

static int uhdlc_open(struct net_device *dev)
{
	u32 cecr_subblock;
	hdlc_device *hdlc = dev_to_hdlc(dev);
	struct ucc_hdlc_private *priv = hdlc->priv;
	struct ucc_tdm *utdm = priv->utdm;
	int rc;

	if (priv->hdlc_busy != 1)
		return 0;

	if (request_irq(priv->ut_info->uf_info.irq,
			ucc_hdlc_irq_handler, 0, "hdlc", priv))
		return -ENODEV;

	cecr_subblock = ucc_fast_get_qe_cr_subblock(
				priv->ut_info->uf_info.ucc_num);

	qe_issue_cmd(QE_INIT_TX_RX, cecr_subblock,
		     QE_CR_PROTOCOL_UNSPECIFIED, 0);

	ucc_fast_enable(priv->uccf, COMM_DIR_RX | COMM_DIR_TX);

	/* Enable the TDM port */
	if (priv->tsa)
		qe_setbits_8(&utdm->si_regs->siglmr1_h, 0x1 << utdm->tdm_port);

	priv->hdlc_busy = 1;
	netif_device_attach(priv->ndev);
	napi_enable(&priv->napi);
	netdev_reset_queue(dev);
	netif_start_queue(dev);

	rc = hdlc_open(dev);
	if (rc)
		uhdlc_close(dev);

	return rc;
}

I agree the above is more readable, but I don't think the whole
refactor is not worthy for a -net fix. I think simply rewriting the
final statements as:

		rc = hdlc_open(dev);
		if (rc)
			uhdlc_close(dev);

		return rc;	

would be good for -net.
 

quoted

  	return 0;

@@ -824,6 +828,8 @@ static int uhdlc_close(struct net_device *dev)
  	netdev_reset_queue(dev);
  	priv->hdlc_busy = 0;
  
+	hdlc_close(dev);
+
  return 0;
    

And while you are looking at the correctness of this code, is it sure 
that uhdlc_open() cannot be called twice in parallele ?
If it can be called in parallèle I think the "if (priv->hdlc_busy != 1)" 
should be replaced by something using cmpxchg()

That part is safe, ndo_open() is invoked under the rtnl lock.

The other comments are IMHO relevant, @Alexandra: please address them.

Thanks!

Paolo