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

[Christophe Leroy]

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 <adiupina@xxxxxxxxxxxxx>
>> ---
>> v4: undo all the things done prior to hdlc_open() as
>> Jakub Kicinski <kuba@xxxxxxxxxx> 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.leroy@xxxxxxxxxx> 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()