Messages in this thread Patch in this message | | | Subject | Re: [PATCH v4] drivers/net: process the result of hdlc_open() and add call of hdlc_close() in uhdlc_close() | From | Paolo Abeni <> | Date | Tue, 05 Sep 2023 12:46:26 +0200 |
| |
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
| |