Le 04/09/2023 à 19:03, Christophe Leroy a écrit : > > > Le 04/09/2023 à 14:31, Alexandra Diupina a écrit : >> Process the result of hdlc_open() and call uhdlc_close() >> in case of an error. It is necessary to pass the error >> code up the control flow, similar to a possible >> error in request_irq(). >> Also add a hdlc_close() call to the uhdlc_close() >> because the comment to hdlc_close() says it must be called >> by the hardware driver when the HDLC device is being closed >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC") >> Signed-off-by: Alexandra Diupina <adiup...@astralinux.ru> >> --- >> v4: undo all the things done prior to hdlc_open() as >> Jakub Kicinski <k...@kernel.org> suggested, >> add hdlc_close() call to the uhdlc_close() to match the function comment, >> add uhdlc_close() declaration to the top of the file not to put the >> uhdlc_close() function definition before uhdlc_open() >> v3: Fix the commits tree >> v2: Remove the 'rc' variable (stores the return value of the >> hdlc_open()) as Christophe Leroy <christophe.le...@csgroup.eu> suggested >> drivers/net/wan/fsl_ucc_hdlc.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> 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)
Of course should be: 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; > } > > > >> 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()