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
>>
