Re:Re: [PATCH v2] usb/peak_usb: cleanup code

[z]

At 2022-05-11 22:28:47, "Vincent MAILHOL" <mailhol.vincent@xxxxxxxxxx> wrote:
>On Wed. 11 May 2022 at 22:02, Bernard Zhao <zhaojunkui2008@xxxxxxx> wrote:
>> The variable fi and bi only used in branch if (!dev->prev_siblings)
>> , fi & bi not kmalloc in else branch, so move kfree into branch
>> if (!dev->prev_siblings),this change is to cleanup the code a bit.
>>
>> Signed-off-by: Bernard Zhao <zhaojunkui2008@xxxxxxx>
>>
>> ---
>> Changes since V1:
>> * move all the content of the if (!dev->prev_siblings) to a new
>> function.
>> ---
>>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 57 +++++++++++++--------
>>  1 file changed, 36 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> index ebe087f258e3..5e472fe086a8 100644
>> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> @@ -841,32 +841,28 @@ static int pcan_usb_pro_stop(struct peak_usb_device *dev)
>>         return 0;
>>  }
>>
>> -/*
>> - * called when probing to initialize a device object.
>> - */
>> -static int pcan_usb_pro_init(struct peak_usb_device *dev)
>> +static int pcan_usb_pro_init_first_channel(struct peak_usb_device *dev, struct pcan_usb_pro_interface **usb_if)
>>  {
>> -       struct pcan_usb_pro_device *pdev =
>> -                       container_of(dev, struct pcan_usb_pro_device, dev);
>> -       struct pcan_usb_pro_interface *usb_if = NULL;
>> -       struct pcan_usb_pro_fwinfo *fi = NULL;
>> -       struct pcan_usb_pro_blinfo *bi = NULL;
>> +       struct pcan_usb_pro_interface *pusb_if = NULL;
>
>Nitpick but I would expect the argument of the function to be named pusb_if:
>
>struct pcan_usb_pro_interface **pusb_if
>
>And this variable to be call usb_if:
>
>struct pcan_usb_pro_interface *usb_if = NULL;
>
>This is to be consistent with pcan_usb_pro_init() where the single
>pointer is also named usb_if (and not pusb_if).
>
>Also, you might as well consider not using and intermediate variable
>and just do *pusb_if throughout all this helper function instead.
>
>>         int err;
>>
>>         /* do this for 1st channel only */
>>         if (!dev->prev_siblings) {
>> +               struct pcan_usb_pro_fwinfo *fi = NULL;
>> +               struct pcan_usb_pro_blinfo *bi = NULL;
>> +
>>                 /* allocate netdevices common structure attached to first one */
>> -               usb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
>> +               pusb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
>>                                  GFP_KERNEL);
>>                 fi = kmalloc(sizeof(struct pcan_usb_pro_fwinfo), GFP_KERNEL);
>>                 bi = kmalloc(sizeof(struct pcan_usb_pro_blinfo), GFP_KERNEL);
>> -               if (!usb_if || !fi || !bi) {
>> +               if (!pusb_if || !fi || !bi) {
>>                         err = -ENOMEM;
>>                         goto err_out;
>
>Did you test that code? Here, you are keeping the original err_out
>label, correct? Aren't the variables fi and bi out of scope after the
>err_out label?
>
>>                 }
>>
>>                 /* number of ts msgs to ignore before taking one into account */
>> -               usb_if->cm_ignore_count = 5;
>> +               pusb_if->cm_ignore_count = 5;
>>
>>                 /*
>>                  * explicit use of dev_xxx() instead of netdev_xxx() here:
>> @@ -903,18 +899,14 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>>                      pcan_usb_pro.name,
>>                      bi->hw_rev, bi->serial_num_hi, bi->serial_num_lo,
>>                      pcan_usb_pro.ctrl_count);
>> +
>> +               kfree(bi);
>> +               kfree(fi);
>>         } else {
>> -               usb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
>> +               pusb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
>>         }
>
>Sorry if I was not clear but I was thinking of just moving the if
>block in a new function and leaving the else part of the original one
>(c.f. below). This way, you lose one level on indentation and you can
>have the declaration, the kmalloc() and the err_out label all at the
>same indentation level in the function's main block.
>
>> -       pdev->usb_if = usb_if;
>> -       usb_if->dev[dev->ctrl_idx] = dev;
>> -
>> -       /* set LED in default state (end of init phase) */
>> -       pcan_usb_pro_set_led(dev, PCAN_USBPRO_LED_DEVICE, 1);
>> -
>> -       kfree(bi);
>> -       kfree(fi);
>> +       *usb_if = pusb_if;
>>
>>         return 0;
>>
>> @@ -926,6 +918,29 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>>         return err;
>>  }
>>
>> +/*
>> + * called when probing to initialize a device object.
>> + */
>> +static int pcan_usb_pro_init(struct peak_usb_device *dev)
>> +{
>> +       struct pcan_usb_pro_device *pdev =
>> +                       container_of(dev, struct pcan_usb_pro_device, dev);
>> +       struct pcan_usb_pro_interface *usb_if = NULL;
>> +       int err;
>> +
>> +       err = pcan_usb_pro_init_first_channel(dev, &usb_if);
>> +       if (err)
>> +               return err;
>
>I was thinking of this:
>
>        if (!dev->prev_siblings) {
>              err = pcan_usb_pro_init_first_channel(dev, &usb_if);
>              if (err)
>                     return err;
>       } else {
>               usb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
>        }

Hi Vincent Mailhol:

Sorry that I made a mistake, I will verify it locally, and then upload it again after the verification is OK.
Thanks!

BR//Bernard

>> +
>> +       pdev->usb_if = usb_if;
>> +       usb_if->dev[dev->ctrl_idx] = dev;
>> +
>> +       /* set LED in default state (end of init phase) */
>> +       pcan_usb_pro_set_led(dev, PCAN_USBPRO_LED_DEVICE, 1);
>> +
>> +       return 0;
>> +}
>> +
>>  static void pcan_usb_pro_exit(struct peak_usb_device *dev)
>>  {
>>         struct pcan_usb_pro_device *pdev =
>> --
>> 2.33.1
>>