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
