netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] usb/peak_usb: cleanup code
@ 2022-05-11 13:02 Bernard Zhao
  2022-05-11 14:28 ` Vincent MAILHOL
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Bernard Zhao @ 2022-05-11 13:02 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	Jakub Kicinski, Paolo Abeni, Vincent Mailhol, Stefan Mätje,
	Bernard Zhao, linux-can, netdev, linux-kernel
  Cc: bernard

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@126.com>

---
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;
 	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;
 		}
 
 		/* 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);
 	}
 
-	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;
+
+	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


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb/peak_usb: cleanup code
  2022-05-11 13:02 [PATCH v2] usb/peak_usb: cleanup code Bernard Zhao
@ 2022-05-11 14:28 ` Vincent MAILHOL
  2022-05-12  1:15   ` z
  2022-05-11 19:42 ` kernel test robot
  2022-05-11 20:43 ` kernel test robot
  2 siblings, 1 reply; 5+ messages in thread
From: Vincent MAILHOL @ 2022-05-11 14:28 UTC (permalink / raw)
  To: Bernard Zhao
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	Jakub Kicinski, Paolo Abeni, Stefan Mätje, linux-can,
	netdev, linux-kernel, bernard

On Wed. 11 May 2022 at 22:02, Bernard Zhao <zhaojunkui2008@126.com> 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@126.com>
>
> ---
> 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);
        }
> +
> +       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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb/peak_usb: cleanup code
  2022-05-11 13:02 [PATCH v2] usb/peak_usb: cleanup code Bernard Zhao
  2022-05-11 14:28 ` Vincent MAILHOL
@ 2022-05-11 19:42 ` kernel test robot
  2022-05-11 20:43 ` kernel test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2022-05-11 19:42 UTC (permalink / raw)
  To: Bernard Zhao, Wolfgang Grandegger, Marc Kleine-Budde,
	David S. Miller, Jakub Kicinski, Paolo Abeni, Vincent Mailhol,
	Stefan Mätje, linux-can, linux-kernel
  Cc: kbuild-all, netdev, bernard

Hi Bernard,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkl-can-next/testing]
[also build test ERROR on v5.18-rc6 next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Bernard-Zhao/usb-peak_usb-cleanup-code/20220511-210544
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing
config: ia64-allmodconfig (https://download.01.org/0day-ci/archive/20220512/202205120331.JrfCulTC-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/126e94285ae6302c0b5ef6ec5174ebc2685ff220
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bernard-Zhao/usb-peak_usb-cleanup-code/20220511-210544
        git checkout 126e94285ae6302c0b5ef6ec5174ebc2685ff220
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/net/can/usb/peak_usb/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/can/usb/peak_usb/pcan_usb_pro.c: In function 'pcan_usb_pro_init_first_channel':
>> drivers/net/can/usb/peak_usb/pcan_usb_pro.c:914:15: error: 'bi' undeclared (first use in this function); did you mean 'bio'?
     914 |         kfree(bi);
         |               ^~
         |               bio
   drivers/net/can/usb/peak_usb/pcan_usb_pro.c:914:15: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/net/can/usb/peak_usb/pcan_usb_pro.c:915:15: error: 'fi' undeclared (first use in this function); did you mean 'fd'?
     915 |         kfree(fi);
         |               ^~
         |               fd


vim +914 drivers/net/can/usb/peak_usb/pcan_usb_pro.c

d8a199355f8f8a Stephane Grosjean 2012-03-02  843  
126e94285ae630 Bernard Zhao      2022-05-11  844  static int pcan_usb_pro_init_first_channel(struct peak_usb_device *dev, struct pcan_usb_pro_interface **usb_if)
d8a199355f8f8a Stephane Grosjean 2012-03-02  845  {
126e94285ae630 Bernard Zhao      2022-05-11  846  	struct pcan_usb_pro_interface *pusb_if = NULL;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  847  	int err;
d8a199355f8f8a Stephane Grosjean 2012-03-02  848  
d8a199355f8f8a Stephane Grosjean 2012-03-02  849  	/* do this for 1st channel only */
d8a199355f8f8a Stephane Grosjean 2012-03-02  850  	if (!dev->prev_siblings) {
126e94285ae630 Bernard Zhao      2022-05-11  851  		struct pcan_usb_pro_fwinfo *fi = NULL;
126e94285ae630 Bernard Zhao      2022-05-11  852  		struct pcan_usb_pro_blinfo *bi = NULL;
126e94285ae630 Bernard Zhao      2022-05-11  853  
d8a199355f8f8a Stephane Grosjean 2012-03-02  854  		/* allocate netdevices common structure attached to first one */
126e94285ae630 Bernard Zhao      2022-05-11  855  		pusb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
d8a199355f8f8a Stephane Grosjean 2012-03-02  856  				 GFP_KERNEL);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  857  		fi = kmalloc(sizeof(struct pcan_usb_pro_fwinfo), GFP_KERNEL);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  858  		bi = kmalloc(sizeof(struct pcan_usb_pro_blinfo), GFP_KERNEL);
126e94285ae630 Bernard Zhao      2022-05-11  859  		if (!pusb_if || !fi || !bi) {
f14e22435a27ef Marc Kleine-Budde 2013-05-16  860  			err = -ENOMEM;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  861  			goto err_out;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  862  		}
d8a199355f8f8a Stephane Grosjean 2012-03-02  863  
d8a199355f8f8a Stephane Grosjean 2012-03-02  864  		/* number of ts msgs to ignore before taking one into account */
126e94285ae630 Bernard Zhao      2022-05-11  865  		pusb_if->cm_ignore_count = 5;
d8a199355f8f8a Stephane Grosjean 2012-03-02  866  
d8a199355f8f8a Stephane Grosjean 2012-03-02  867  		/*
d8a199355f8f8a Stephane Grosjean 2012-03-02  868  		 * explicit use of dev_xxx() instead of netdev_xxx() here:
d8a199355f8f8a Stephane Grosjean 2012-03-02  869  		 * information displayed are related to the device itself, not
d8a199355f8f8a Stephane Grosjean 2012-03-02  870  		 * to the canx netdevices.
d8a199355f8f8a Stephane Grosjean 2012-03-02  871  		 */
d8a199355f8f8a Stephane Grosjean 2012-03-02  872  		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
d8a199355f8f8a Stephane Grosjean 2012-03-02  873  					    PCAN_USBPRO_INFO_FW,
f14e22435a27ef Marc Kleine-Budde 2013-05-16  874  					    fi, sizeof(*fi));
d8a199355f8f8a Stephane Grosjean 2012-03-02  875  		if (err) {
d8a199355f8f8a Stephane Grosjean 2012-03-02  876  			dev_err(dev->netdev->dev.parent,
d8a199355f8f8a Stephane Grosjean 2012-03-02  877  				"unable to read %s firmware info (err %d)\n",
d8a199355f8f8a Stephane Grosjean 2012-03-02  878  				pcan_usb_pro.name, err);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  879  			goto err_out;
d8a199355f8f8a Stephane Grosjean 2012-03-02  880  		}
d8a199355f8f8a Stephane Grosjean 2012-03-02  881  
d8a199355f8f8a Stephane Grosjean 2012-03-02  882  		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
d8a199355f8f8a Stephane Grosjean 2012-03-02  883  					    PCAN_USBPRO_INFO_BL,
f14e22435a27ef Marc Kleine-Budde 2013-05-16  884  					    bi, sizeof(*bi));
d8a199355f8f8a Stephane Grosjean 2012-03-02  885  		if (err) {
d8a199355f8f8a Stephane Grosjean 2012-03-02  886  			dev_err(dev->netdev->dev.parent,
d8a199355f8f8a Stephane Grosjean 2012-03-02  887  				"unable to read %s bootloader info (err %d)\n",
d8a199355f8f8a Stephane Grosjean 2012-03-02  888  				pcan_usb_pro.name, err);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  889  			goto err_out;
d8a199355f8f8a Stephane Grosjean 2012-03-02  890  		}
d8a199355f8f8a Stephane Grosjean 2012-03-02  891  
f14e22435a27ef Marc Kleine-Budde 2013-05-16  892  		/* tell the device the can driver is running */
f14e22435a27ef Marc Kleine-Budde 2013-05-16  893  		err = pcan_usb_pro_drv_loaded(dev, 1);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  894  		if (err)
f14e22435a27ef Marc Kleine-Budde 2013-05-16  895  			goto err_out;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  896  
d8a199355f8f8a Stephane Grosjean 2012-03-02  897  		dev_info(dev->netdev->dev.parent,
d8a199355f8f8a Stephane Grosjean 2012-03-02  898  		     "PEAK-System %s hwrev %u serial %08X.%08X (%u channels)\n",
d8a199355f8f8a Stephane Grosjean 2012-03-02  899  		     pcan_usb_pro.name,
f14e22435a27ef Marc Kleine-Budde 2013-05-16  900  		     bi->hw_rev, bi->serial_num_hi, bi->serial_num_lo,
d8a199355f8f8a Stephane Grosjean 2012-03-02  901  		     pcan_usb_pro.ctrl_count);
d8a199355f8f8a Stephane Grosjean 2012-03-02  902  
20fb4eb96fb035 Marc Kleine-Budde 2013-12-14  903  		kfree(bi);
20fb4eb96fb035 Marc Kleine-Budde 2013-12-14  904  		kfree(fi);
126e94285ae630 Bernard Zhao      2022-05-11  905  	} else {
126e94285ae630 Bernard Zhao      2022-05-11  906  		pusb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
126e94285ae630 Bernard Zhao      2022-05-11  907  	}
126e94285ae630 Bernard Zhao      2022-05-11  908  
126e94285ae630 Bernard Zhao      2022-05-11  909  	*usb_if = pusb_if;
20fb4eb96fb035 Marc Kleine-Budde 2013-12-14  910  
d8a199355f8f8a Stephane Grosjean 2012-03-02  911  	return 0;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  912  
f14e22435a27ef Marc Kleine-Budde 2013-05-16  913   err_out:
f14e22435a27ef Marc Kleine-Budde 2013-05-16 @914  	kfree(bi);
f14e22435a27ef Marc Kleine-Budde 2013-05-16 @915  	kfree(fi);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  916  	kfree(usb_if);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  917  
f14e22435a27ef Marc Kleine-Budde 2013-05-16  918  	return err;
d8a199355f8f8a Stephane Grosjean 2012-03-02  919  }
d8a199355f8f8a Stephane Grosjean 2012-03-02  920  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb/peak_usb: cleanup code
  2022-05-11 13:02 [PATCH v2] usb/peak_usb: cleanup code Bernard Zhao
  2022-05-11 14:28 ` Vincent MAILHOL
  2022-05-11 19:42 ` kernel test robot
@ 2022-05-11 20:43 ` kernel test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2022-05-11 20:43 UTC (permalink / raw)
  To: Bernard Zhao, Wolfgang Grandegger, Marc Kleine-Budde,
	David S. Miller, Jakub Kicinski, Paolo Abeni, Vincent Mailhol,
	Stefan Mätje, linux-can, linux-kernel
  Cc: llvm, kbuild-all, netdev, bernard

Hi Bernard,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkl-can-next/testing]
[also build test ERROR on v5.18-rc6 next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Bernard-Zhao/usb-peak_usb-cleanup-code/20220511-210544
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing
config: hexagon-randconfig-r023-20220509 (https://download.01.org/0day-ci/archive/20220512/202205120402.hmn6WJGb-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 18dd123c56754edf62c7042dcf23185c3727610f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/126e94285ae6302c0b5ef6ec5174ebc2685ff220
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bernard-Zhao/usb-peak_usb-cleanup-code/20220511-210544
        git checkout 126e94285ae6302c0b5ef6ec5174ebc2685ff220
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/net/can/usb/peak_usb/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/can/usb/peak_usb/pcan_usb_pro.c:914:8: error: use of undeclared identifier 'bi'
           kfree(bi);
                 ^
>> drivers/net/can/usb/peak_usb/pcan_usb_pro.c:915:8: error: use of undeclared identifier 'fi'
           kfree(fi);
                 ^
   2 errors generated.


vim +/bi +914 drivers/net/can/usb/peak_usb/pcan_usb_pro.c

d8a199355f8f8a Stephane Grosjean 2012-03-02  843  
126e94285ae630 Bernard Zhao      2022-05-11  844  static int pcan_usb_pro_init_first_channel(struct peak_usb_device *dev, struct pcan_usb_pro_interface **usb_if)
d8a199355f8f8a Stephane Grosjean 2012-03-02  845  {
126e94285ae630 Bernard Zhao      2022-05-11  846  	struct pcan_usb_pro_interface *pusb_if = NULL;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  847  	int err;
d8a199355f8f8a Stephane Grosjean 2012-03-02  848  
d8a199355f8f8a Stephane Grosjean 2012-03-02  849  	/* do this for 1st channel only */
d8a199355f8f8a Stephane Grosjean 2012-03-02  850  	if (!dev->prev_siblings) {
126e94285ae630 Bernard Zhao      2022-05-11  851  		struct pcan_usb_pro_fwinfo *fi = NULL;
126e94285ae630 Bernard Zhao      2022-05-11  852  		struct pcan_usb_pro_blinfo *bi = NULL;
126e94285ae630 Bernard Zhao      2022-05-11  853  
d8a199355f8f8a Stephane Grosjean 2012-03-02  854  		/* allocate netdevices common structure attached to first one */
126e94285ae630 Bernard Zhao      2022-05-11  855  		pusb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
d8a199355f8f8a Stephane Grosjean 2012-03-02  856  				 GFP_KERNEL);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  857  		fi = kmalloc(sizeof(struct pcan_usb_pro_fwinfo), GFP_KERNEL);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  858  		bi = kmalloc(sizeof(struct pcan_usb_pro_blinfo), GFP_KERNEL);
126e94285ae630 Bernard Zhao      2022-05-11  859  		if (!pusb_if || !fi || !bi) {
f14e22435a27ef Marc Kleine-Budde 2013-05-16  860  			err = -ENOMEM;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  861  			goto err_out;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  862  		}
d8a199355f8f8a Stephane Grosjean 2012-03-02  863  
d8a199355f8f8a Stephane Grosjean 2012-03-02  864  		/* number of ts msgs to ignore before taking one into account */
126e94285ae630 Bernard Zhao      2022-05-11  865  		pusb_if->cm_ignore_count = 5;
d8a199355f8f8a Stephane Grosjean 2012-03-02  866  
d8a199355f8f8a Stephane Grosjean 2012-03-02  867  		/*
d8a199355f8f8a Stephane Grosjean 2012-03-02  868  		 * explicit use of dev_xxx() instead of netdev_xxx() here:
d8a199355f8f8a Stephane Grosjean 2012-03-02  869  		 * information displayed are related to the device itself, not
d8a199355f8f8a Stephane Grosjean 2012-03-02  870  		 * to the canx netdevices.
d8a199355f8f8a Stephane Grosjean 2012-03-02  871  		 */
d8a199355f8f8a Stephane Grosjean 2012-03-02  872  		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
d8a199355f8f8a Stephane Grosjean 2012-03-02  873  					    PCAN_USBPRO_INFO_FW,
f14e22435a27ef Marc Kleine-Budde 2013-05-16  874  					    fi, sizeof(*fi));
d8a199355f8f8a Stephane Grosjean 2012-03-02  875  		if (err) {
d8a199355f8f8a Stephane Grosjean 2012-03-02  876  			dev_err(dev->netdev->dev.parent,
d8a199355f8f8a Stephane Grosjean 2012-03-02  877  				"unable to read %s firmware info (err %d)\n",
d8a199355f8f8a Stephane Grosjean 2012-03-02  878  				pcan_usb_pro.name, err);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  879  			goto err_out;
d8a199355f8f8a Stephane Grosjean 2012-03-02  880  		}
d8a199355f8f8a Stephane Grosjean 2012-03-02  881  
d8a199355f8f8a Stephane Grosjean 2012-03-02  882  		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
d8a199355f8f8a Stephane Grosjean 2012-03-02  883  					    PCAN_USBPRO_INFO_BL,
f14e22435a27ef Marc Kleine-Budde 2013-05-16  884  					    bi, sizeof(*bi));
d8a199355f8f8a Stephane Grosjean 2012-03-02  885  		if (err) {
d8a199355f8f8a Stephane Grosjean 2012-03-02  886  			dev_err(dev->netdev->dev.parent,
d8a199355f8f8a Stephane Grosjean 2012-03-02  887  				"unable to read %s bootloader info (err %d)\n",
d8a199355f8f8a Stephane Grosjean 2012-03-02  888  				pcan_usb_pro.name, err);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  889  			goto err_out;
d8a199355f8f8a Stephane Grosjean 2012-03-02  890  		}
d8a199355f8f8a Stephane Grosjean 2012-03-02  891  
f14e22435a27ef Marc Kleine-Budde 2013-05-16  892  		/* tell the device the can driver is running */
f14e22435a27ef Marc Kleine-Budde 2013-05-16  893  		err = pcan_usb_pro_drv_loaded(dev, 1);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  894  		if (err)
f14e22435a27ef Marc Kleine-Budde 2013-05-16  895  			goto err_out;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  896  
d8a199355f8f8a Stephane Grosjean 2012-03-02  897  		dev_info(dev->netdev->dev.parent,
d8a199355f8f8a Stephane Grosjean 2012-03-02  898  		     "PEAK-System %s hwrev %u serial %08X.%08X (%u channels)\n",
d8a199355f8f8a Stephane Grosjean 2012-03-02  899  		     pcan_usb_pro.name,
f14e22435a27ef Marc Kleine-Budde 2013-05-16  900  		     bi->hw_rev, bi->serial_num_hi, bi->serial_num_lo,
d8a199355f8f8a Stephane Grosjean 2012-03-02  901  		     pcan_usb_pro.ctrl_count);
d8a199355f8f8a Stephane Grosjean 2012-03-02  902  
20fb4eb96fb035 Marc Kleine-Budde 2013-12-14  903  		kfree(bi);
20fb4eb96fb035 Marc Kleine-Budde 2013-12-14  904  		kfree(fi);
126e94285ae630 Bernard Zhao      2022-05-11  905  	} else {
126e94285ae630 Bernard Zhao      2022-05-11  906  		pusb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
126e94285ae630 Bernard Zhao      2022-05-11  907  	}
126e94285ae630 Bernard Zhao      2022-05-11  908  
126e94285ae630 Bernard Zhao      2022-05-11  909  	*usb_if = pusb_if;
20fb4eb96fb035 Marc Kleine-Budde 2013-12-14  910  
d8a199355f8f8a Stephane Grosjean 2012-03-02  911  	return 0;
f14e22435a27ef Marc Kleine-Budde 2013-05-16  912  
f14e22435a27ef Marc Kleine-Budde 2013-05-16  913   err_out:
f14e22435a27ef Marc Kleine-Budde 2013-05-16 @914  	kfree(bi);
f14e22435a27ef Marc Kleine-Budde 2013-05-16 @915  	kfree(fi);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  916  	kfree(usb_if);
f14e22435a27ef Marc Kleine-Budde 2013-05-16  917  
f14e22435a27ef Marc Kleine-Budde 2013-05-16  918  	return err;
d8a199355f8f8a Stephane Grosjean 2012-03-02  919  }
d8a199355f8f8a Stephane Grosjean 2012-03-02  920  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re:Re: [PATCH v2] usb/peak_usb: cleanup code
  2022-05-11 14:28 ` Vincent MAILHOL
@ 2022-05-12  1:15   ` z
  0 siblings, 0 replies; 5+ messages in thread
From: z @ 2022-05-12  1:15 UTC (permalink / raw)
  To: Vincent MAILHOL
  Cc: Wolfgang Grandegger, Marc Kleine-Budde, David S. Miller,
	Jakub Kicinski, Paolo Abeni, Stefan Mätje, linux-can,
	netdev, linux-kernel, bernard



At 2022-05-11 22:28:47, "Vincent MAILHOL" <mailhol.vincent@wanadoo.fr> wrote:
>On Wed. 11 May 2022 at 22:02, Bernard Zhao <zhaojunkui2008@126.com> 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@126.com>
>>
>> ---
>> 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
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-12  1:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-11 13:02 [PATCH v2] usb/peak_usb: cleanup code Bernard Zhao
2022-05-11 14:28 ` Vincent MAILHOL
2022-05-12  1:15   ` z
2022-05-11 19:42 ` kernel test robot
2022-05-11 20:43 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).