Re: [PATCH v4] drivers/net: process the result of hdlc_open() and add call of hdlc_close() in uhdlc_close()

[Paolo Abeni]

On Mon, 2023-09-04 at 17:03 +0000, Christophe Leroy wrote:
> 
> Le 04/09/2023 à 14:31, Alexandra Diupina a écrit :
> > 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.
> 
> > +		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.
 
> >   	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