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 <[email protected]>
---
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
On Wed. 11 May 2022 at 22:02, Bernard Zhao <[email protected]> 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 <[email protected]>
>
> ---
> 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
>
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/[email protected]/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 <[email protected]>
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
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/[email protected]/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 <[email protected]>
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
At 2022-05-11 22:28:47, "Vincent MAILHOL" <[email protected]> wrote:
>On Wed. 11 May 2022 at 22:02, Bernard Zhao <[email protected]> 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 <[email protected]>
>>
>> ---
>> 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
>>