diff mbox series

[v3,1/3] net: phy: mscc: move shared probe code into a helper

Message ID 20200615144501.1140870-1-heiko@sntech.de
State Changes Requested
Delegated to: David Miller
Headers show
Series [v3,1/3] net: phy: mscc: move shared probe code into a helper | expand

Commit Message

Heiko Stuebner June 15, 2020, 2:44 p.m. UTC 
From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The different probe functions share a lot of code, so move the
common parts into a helper to reduce duplication.

This moves the devm_phy_package_join below the general allocation
but as all components just allocate things, this should be ok.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
changes in v3:
- adapt to 5.8 merge-window results
changes in v2:
- new patch as suggested by Andrew

 drivers/net/phy/mscc/mscc_main.c | 123 +++++++++++++++----------------
 1 file changed, 60 insertions(+), 63 deletions(-)

Comments

kernel test robot June 15, 2020, 8:56 p.m. UTC | #1 
Hi Heiko,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on sparc-next/master net/master linus/master v5.8-rc1 next-20200615]
[cannot apply to robh/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Heiko-Stuebner/net-phy-mscc-move-shared-probe-code-into-a-helper/20200615-224727
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git cb8e59cc87201af93dfbb6c3dccc8fcad72a09c2
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3d8149c2a1228609fd7d7c91a04681304a2f0ca9)
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
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

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

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/net/phy/mscc/mscc_main.c:2002:10: warning: variable 'vsc8531' is uninitialized when used here [-Wuninitialized]
vsc8531->base_addr, 0);
^~~~~~~
drivers/net/phy/mscc/mscc_main.c:1986:33: note: initialize the variable 'vsc8531' to silence this warning
struct vsc8531_private *vsc8531;
^
= NULL
1 warning generated.

vim +/vsc8531 +2002 drivers/net/phy/mscc/mscc_main.c

  1983	
  1984	static int vsc8574_probe(struct phy_device *phydev)
  1985	{
  1986		struct vsc8531_private *vsc8531;
  1987		int rc;
  1988		u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
  1989		   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
  1990		   VSC8531_DUPLEX_COLLISION};
  1991	
  1992		rc = vsc85xx_probe_helper(phydev, default_mode,
  1993					  ARRAY_SIZE(default_mode),
  1994					  VSC8584_SUPP_LED_MODES,
  1995					  vsc8584_hw_stats,
  1996					  ARRAY_SIZE(vsc8584_hw_stats));
  1997		if (rc < 0)
  1998			return rc;
  1999	
  2000		vsc8584_get_base_addr(phydev);
  2001		return devm_phy_package_join(&phydev->mdio.dev, phydev,
> 2002					     vsc8531->base_addr, 0);
  2003	}
  2004	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
David Miller June 16, 2020, 1:11 a.m. UTC | #2 
From: Heiko Stuebner <heiko@sntech.de>
Date: Mon, 15 Jun 2020 16:44:59 +0200

>  static int vsc8584_probe(struct phy_device *phydev)
>  {
>  	struct vsc8531_private *vsc8531;
> +	int rc;
>  	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
>  	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
>  	   VSC8531_DUPLEX_COLLISION};
> @@ -2005,32 +2015,24 @@ static int vsc8584_probe(struct phy_device *phydev)
>  		return -ENOTSUPP;
>  	}
>  
> -	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
> -	if (!vsc8531)
> -		return -ENOMEM;

Because you removed this devm_kzalloc() code, vsc8531 is never initialized.

> +	return devm_phy_package_join(&phydev->mdio.dev, phydev,
> +				     vsc8531->base_addr, 0);

But it is still dereferenced here.

Did the compiler really not warn you about this when you test built
these changes?
David Miller June 16, 2020, 1:12 a.m. UTC | #3 
From: David Miller <davem@davemloft.net>
Date: Mon, 15 Jun 2020 18:11:29 -0700 (PDT)

> Because you removed this devm_kzalloc() code, vsc8531 is never initialized.

You also need to provide a proper header posting when you repost this series
after fixing this bug.
Heiko Stuebner June 16, 2020, 9:10 a.m. UTC | #4 
Hi,

Am Dienstag, 16. Juni 2020, 03:12:25 CEST schrieb David Miller:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 15 Jun 2020 18:11:29 -0700 (PDT)
> > +	return devm_phy_package_join(&phydev->mdio.dev, phydev,
> > +				     vsc8531->base_addr, 0);
> 
> But it is still dereferenced here.
> 
> Did the compiler really not warn you about this when you test built
> these changes?

I'm wondering that myself ... it probably did and I overlooked it, which
also is indicated by the fact that  I did add the declaration of the
vsc8531 when rebasing.

> > Because you removed this devm_kzalloc() code, vsc8531 is never initialized.
> 
> You also need to provide a proper header posting when you repost this series
> after fixing this bug.

not sure I understand what you mean with "header posting" here.

Thanks
Heiko
Russell King (Oracle) June 16, 2020, 10:13 a.m. UTC | #5 
On Tue, Jun 16, 2020 at 11:10:27AM +0200, Heiko Stübner wrote:
> > 
> > You also need to provide a proper header posting when you repost this series
> > after fixing this bug.
> 
> not sure I understand what you mean with "header posting" here.

David is requesting that you send a "0/N" email summarising the purpose
of the patch series and any other relevant information to the series as
a whole.  The subsequent patches should be threaded to the 0/N email.

The 0/N email should also contain the overall diffstat for the series.
Dan Carpenter June 16, 2020, 12:27 p.m. UTC | #6 
Hi Heiko,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on sparc-next/master net/master linus/master v5.8-rc1 next-20200616]
[cannot apply to robh/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Heiko-Stuebner/net-phy-mscc-move-shared-probe-code-into-a-helper/20200615-224727
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git cb8e59cc87201af93dfbb6c3dccc8fcad72a09c2
config: i386-randconfig-m021-20200616 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

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

smatch warnings:
drivers/net/phy/mscc/mscc_main.c:2002 vsc8574_probe() error: potentially dereferencing uninitialized 'vsc8531'.

# https://github.com/0day-ci/linux/commit/5be350208c014ad5e0afd06868ccaaefd6216345
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 5be350208c014ad5e0afd06868ccaaefd6216345
vim +/vsc8531 +2002 drivers/net/phy/mscc/mscc_main.c

5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1984  static int vsc8574_probe(struct phy_device *phydev)
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1985  {
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1986  	struct vsc8531_private *vsc8531;
                                                                                                                ^^^^^^^

5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1987  	int rc;
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1988  	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1989  	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1990  	   VSC8531_DUPLEX_COLLISION};
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1991  
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1992  	rc = vsc85xx_probe_helper(phydev, default_mode,
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1993  				  ARRAY_SIZE(default_mode),
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1994  				  VSC8584_SUPP_LED_MODES,
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1995  				  vsc8584_hw_stats,
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1996  				  ARRAY_SIZE(vsc8584_hw_stats));
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1997  	if (rc < 0)
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  1998  		return rc;
00d70d8e0e7811 drivers/net/phy/mscc.c           Quentin Schulz 2018-10-08  1999  
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  2000  	vsc8584_get_base_addr(phydev);
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15  2001  	return devm_phy_package_join(&phydev->mdio.dev, phydev,
5be350208c014a drivers/net/phy/mscc/mscc_main.c Heiko Stuebner 2020-06-15 @2002  				     vsc8531->base_addr, 0);
                                                                                                                     ^^^^^^^^^^^^^^^^^^
Not initialized.

00d70d8e0e7811 drivers/net/phy/mscc.c           Quentin Schulz 2018-10-08  2003  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 5ddc44f87eaf..68308d3e9589 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1935,12 +1935,11 @@  static int vsc85xx_read_status(struct phy_device *phydev)
 	return genphy_read_status(phydev);
 }
 
-static int vsc8514_probe(struct phy_device *phydev)
+static int vsc85xx_probe_helper(struct phy_device *phydev,
+				u32 *leds, int num_leds, u16 led_modes,
+				const struct vsc85xx_hw_stat *stats, int nstats)
 {
 	struct vsc8531_private *vsc8531;
-	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
-	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
-	   VSC8531_DUPLEX_COLLISION};
 
 	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
 	if (!vsc8531)
@@ -1948,54 +1947,65 @@  static int vsc8514_probe(struct phy_device *phydev)
 
 	phydev->priv = vsc8531;
 
-	vsc8584_get_base_addr(phydev);
-	devm_phy_package_join(&phydev->mdio.dev, phydev,
-			      vsc8531->base_addr, 0);
-
-	vsc8531->nleds = 4;
-	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc85xx_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
+	vsc8531->nleds = num_leds;
+	vsc8531->supp_led_modes = led_modes;
+	vsc8531->hw_stats = stats;
+	vsc8531->nstats = nstats;
 	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
 				      sizeof(u64), GFP_KERNEL);
 	if (!vsc8531->stats)
 		return -ENOMEM;
 
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	return vsc85xx_dt_led_modes_get(phydev, leds);
 }
 
-static int vsc8574_probe(struct phy_device *phydev)
+static int vsc8514_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
+	int rc;
 	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
 	   VSC8531_DUPLEX_COLLISION};
 
-	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
-	if (!vsc8531)
-		return -ENOMEM;
-
-	phydev->priv = vsc8531;
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC85XX_SUPP_LED_MODES,
+				  vsc85xx_hw_stats,
+				  ARRAY_SIZE(vsc85xx_hw_stats));
+	if (rc < 0)
+		return rc;
 
+	vsc8531 = phydev->priv;
 	vsc8584_get_base_addr(phydev);
-	devm_phy_package_join(&phydev->mdio.dev, phydev,
-			      vsc8531->base_addr, 0);
+	return devm_phy_package_join(&phydev->mdio.dev, phydev,
+				     vsc8531->base_addr, 0);
+}
 
-	vsc8531->nleds = 4;
-	vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc8584_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc8584_hw_stats);
-	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
-				      sizeof(u64), GFP_KERNEL);
-	if (!vsc8531->stats)
-		return -ENOMEM;
+static int vsc8574_probe(struct phy_device *phydev)
+{
+	struct vsc8531_private *vsc8531;
+	int rc;
+	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
+	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
+	   VSC8531_DUPLEX_COLLISION};
+
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC8584_SUPP_LED_MODES,
+				  vsc8584_hw_stats,
+				  ARRAY_SIZE(vsc8584_hw_stats));
+	if (rc < 0)
+		return rc;
 
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	vsc8584_get_base_addr(phydev);
+	return devm_phy_package_join(&phydev->mdio.dev, phydev,
+				     vsc8531->base_addr, 0);
 }
 
 static int vsc8584_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
+	int rc;
 	u32 default_mode[4] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY, VSC8531_LINK_ACTIVITY,
 	   VSC8531_DUPLEX_COLLISION};
@@ -2005,32 +2015,24 @@  static int vsc8584_probe(struct phy_device *phydev)
 		return -ENOTSUPP;
 	}
 
-	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
-	if (!vsc8531)
-		return -ENOMEM;
-
-	phydev->priv = vsc8531;
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC8584_SUPP_LED_MODES,
+				  vsc8584_hw_stats,
+				  ARRAY_SIZE(vsc8584_hw_stats));
+	if (rc < 0)
+		return rc;
 
+	vsc8531 = phydev->priv;
 	vsc8584_get_base_addr(phydev);
-	devm_phy_package_join(&phydev->mdio.dev, phydev,
-			      vsc8531->base_addr, 0);
-
-	vsc8531->nleds = 4;
-	vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc8584_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc8584_hw_stats);
-	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
-				      sizeof(u64), GFP_KERNEL);
-	if (!vsc8531->stats)
-		return -ENOMEM;
-
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	return devm_phy_package_join(&phydev->mdio.dev, phydev,
+				     vsc8531->base_addr, 0);
 }
 
 static int vsc85xx_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
-	int rate_magic;
+	int rate_magic, rc;
 	u32 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY};
 
@@ -2038,23 +2040,18 @@  static int vsc85xx_probe(struct phy_device *phydev)
 	if (rate_magic < 0)
 		return rate_magic;
 
-	vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
-	if (!vsc8531)
-		return -ENOMEM;
-
-	phydev->priv = vsc8531;
+	rc = vsc85xx_probe_helper(phydev, default_mode,
+				  ARRAY_SIZE(default_mode),
+				  VSC85XX_SUPP_LED_MODES,
+				  vsc85xx_hw_stats,
+				  ARRAY_SIZE(vsc85xx_hw_stats));
+	if (rc < 0)
+		return rc;
 
+	vsc8531 = phydev->priv;
 	vsc8531->rate_magic = rate_magic;
-	vsc8531->nleds = 2;
-	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
-	vsc8531->hw_stats = vsc85xx_hw_stats;
-	vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
-	vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
-				      sizeof(u64), GFP_KERNEL);
-	if (!vsc8531->stats)
-		return -ENOMEM;
 
-	return vsc85xx_dt_led_modes_get(phydev, default_mode);
+	return 0;
 }
 
 /* Microsemi VSC85xx PHYs */