[PATCH V2] net: dsa: Add of_node_put() before break statement

[PATCH V2] net: dsa: Add of_node_put() before break statement

[Sumera Priyadarsini]

Every iteration of for_each_child_of_node() decrements
the reference count of the previous node, however when control
is transferred from the middle of the loop, as in the case of
a return or break or goto, there is no decrement thus ultimately
resulting in a memory leak.

Fix a potential memory leak in mt7530.c by inserting of_node_put()
before the break statement.

Issue found with Coccinelle.

---
Changes in v2:
	Add another of_node_put() in for_each_child_of_node() as pointed
out by Andrew.

---

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
---
 drivers/net/dsa/mt7530.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8dcb8a49ab67..e81198a65c26 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1327,6 +1327,7 @@ mt7530_setup(struct dsa_switch *ds)
 			if (phy_node->parent == priv->dev->of_node->parent) {
 				ret = of_get_phy_mode(mac_np, &interface);
 				if (ret && ret != -ENODEV)
+					of_node_put(mac_np)
 					return ret;
 				id = of_mdio_parse_addr(ds->dev, phy_node);
 				if (id == 0)
@@ -1334,6 +1335,7 @@ mt7530_setup(struct dsa_switch *ds)
 				if (id == 4)
 					priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
 			}
+			of_node_put(mac_np);
 			of_node_put(phy_node);
 			break;
 		}
-- 
2.17.1

Re: [PATCH V2] net: dsa: Add of_node_put() before break statement

[Andrew Lunn]

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 8dcb8a49ab67..e81198a65c26 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1327,6 +1327,7 @@ mt7530_setup(struct dsa_switch *ds)
>  			if (phy_node->parent == priv->dev->of_node->parent) {
>  				ret = of_get_phy_mode(mac_np, &interface);
>  				if (ret && ret != -ENODEV)
> +					of_node_put(mac_np)
>  					return ret;
>  				id = of_mdio_parse_addr(ds->dev, phy_node);
>  				if (id == 0)

You are missing some { }. And a ; . I'm surprised gcc did not warn
you!

    Andrew

Re: [PATCH V2] net: dsa: Add of_node_put() before break statement

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6700 bytes --]

Hi Sumera,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master sparc-next/master v5.9-rc1 next-20200821]
[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/0day-ci/linux/commits/Sumera-Priyadarsini/net-dsa-Add-of_node_put-before-break-statement/20200824-025301
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d7223aa5867134b9923b42e1245801bd790a1d8c
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

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/dsa/mt7530.c: In function 'mt7530_setup':
>> drivers/net/dsa/mt7530.c:1330:25: error: expected ';' before 'return'
    1330 |      of_node_put(mac_np)
         |                         ^
         |                         ;
    1331 |      return ret;
         |      ~~~~~~              

# https://github.com/0day-ci/linux/commit/9bc2e04821fee9caceb61a8fa42ab7d85fe73364
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sumera-Priyadarsini/net-dsa-Add-of_node_put-before-break-statement/20200824-025301
git checkout 9bc2e04821fee9caceb61a8fa42ab7d85fe73364
vim +1330 drivers/net/dsa/mt7530.c

  1205	
  1206	static int
  1207	mt7530_setup(struct dsa_switch *ds)
  1208	{
  1209		struct mt7530_priv *priv = ds->priv;
  1210		struct device_node *phy_node;
  1211		struct device_node *mac_np;
  1212		struct mt7530_dummy_poll p;
  1213		phy_interface_t interface;
  1214		struct device_node *dn;
  1215		u32 id, val;
  1216		int ret, i;
  1217	
  1218		/* The parent node of master netdev which holds the common system
  1219		 * controller also is the container for two GMACs nodes representing
  1220		 * as two netdev instances.
  1221		 */
  1222		dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent;
  1223		ds->configure_vlan_while_not_filtering = true;
  1224	
  1225		if (priv->id == ID_MT7530) {
  1226			regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
  1227			ret = regulator_enable(priv->core_pwr);
  1228			if (ret < 0) {
  1229				dev_err(priv->dev,
  1230					"Failed to enable core power: %d\n", ret);
  1231				return ret;
  1232			}
  1233	
  1234			regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
  1235			ret = regulator_enable(priv->io_pwr);
  1236			if (ret < 0) {
  1237				dev_err(priv->dev, "Failed to enable io pwr: %d\n",
  1238					ret);
  1239				return ret;
  1240			}
  1241		}
  1242	
  1243		/* Reset whole chip through gpio pin or memory-mapped registers for
  1244		 * different type of hardware
  1245		 */
  1246		if (priv->mcm) {
  1247			reset_control_assert(priv->rstc);
  1248			usleep_range(1000, 1100);
  1249			reset_control_deassert(priv->rstc);
  1250		} else {
  1251			gpiod_set_value_cansleep(priv->reset, 0);
  1252			usleep_range(1000, 1100);
  1253			gpiod_set_value_cansleep(priv->reset, 1);
  1254		}
  1255	
  1256		/* Waiting for MT7530 got to stable */
  1257		INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
  1258		ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
  1259					 20, 1000000);
  1260		if (ret < 0) {
  1261			dev_err(priv->dev, "reset timeout\n");
  1262			return ret;
  1263		}
  1264	
  1265		id = mt7530_read(priv, MT7530_CREV);
  1266		id >>= CHIP_NAME_SHIFT;
  1267		if (id != MT7530_ID) {
  1268			dev_err(priv->dev, "chip %x can't be supported\n", id);
  1269			return -ENODEV;
  1270		}
  1271	
  1272		/* Reset the switch through internal reset */
  1273		mt7530_write(priv, MT7530_SYS_CTRL,
  1274			     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
  1275			     SYS_CTRL_REG_RST);
  1276	
  1277		/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
  1278		val = mt7530_read(priv, MT7530_MHWTRAP);
  1279		val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
  1280		val |= MHWTRAP_MANUAL;
  1281		mt7530_write(priv, MT7530_MHWTRAP, val);
  1282	
  1283		priv->p6_interface = PHY_INTERFACE_MODE_NA;
  1284	
  1285		/* Enable and reset MIB counters */
  1286		mt7530_mib_reset(ds);
  1287	
  1288		for (i = 0; i < MT7530_NUM_PORTS; i++) {
  1289			/* Disable forwarding by default on all ports */
  1290			mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
  1291				   PCR_MATRIX_CLR);
  1292	
  1293			if (dsa_is_cpu_port(ds, i))
  1294				mt7530_cpu_port_enable(priv, i);
  1295			else
  1296				mt7530_port_disable(ds, i);
  1297	
  1298			/* Enable consistent egress tag */
  1299			mt7530_rmw(priv, MT7530_PVC_P(i), PVC_EG_TAG_MASK,
  1300				   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
  1301		}
  1302	
  1303		/* Setup port 5 */
  1304		priv->p5_intf_sel = P5_DISABLED;
  1305		interface = PHY_INTERFACE_MODE_NA;
  1306	
  1307		if (!dsa_is_unused_port(ds, 5)) {
  1308			priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
  1309			ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
  1310			if (ret && ret != -ENODEV)
  1311				return ret;
  1312		} else {
  1313			/* Scan the ethernet nodes. look for GMAC1, lookup used phy */
  1314			for_each_child_of_node(dn, mac_np) {
  1315				if (!of_device_is_compatible(mac_np,
  1316							     "mediatek,eth-mac"))
  1317					continue;
  1318	
  1319				ret = of_property_read_u32(mac_np, "reg", &id);
  1320				if (ret < 0 || id != 1)
  1321					continue;
  1322	
  1323				phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
  1324				if (!phy_node)
  1325					continue;
  1326	
  1327				if (phy_node->parent == priv->dev->of_node->parent) {
  1328					ret = of_get_phy_mode(mac_np, &interface);
  1329					if (ret && ret != -ENODEV)
> 1330						of_node_put(mac_np)
  1331						return ret;
  1332					id = of_mdio_parse_addr(ds->dev, phy_node);
  1333					if (id == 0)
  1334						priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
  1335					if (id == 4)
  1336						priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
  1337				}
  1338				of_node_put(mac_np);
  1339				of_node_put(phy_node);
  1340				break;
  1341			}
  1342		}
  1343	
  1344		mt7530_setup_port5(ds, interface);
  1345	
  1346		/* Flush the FDB table */
  1347		ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
  1348		if (ret < 0)
  1349			return ret;
  1350	
  1351		return 0;
  1352	}
  1353	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75898 bytes --]

Re: [PATCH V2] net: dsa: Add of_node_put() before break statement

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7069 bytes --]

Hi Sumera,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master sparc-next/master v5.9-rc2 next-20200821]
[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/0day-ci/linux/commits/Sumera-Priyadarsini/net-dsa-Add-of_node_put-before-break-statement/20200824-025301
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d7223aa5867134b9923b42e1245801bd790a1d8c
config: x86_64-randconfig-a012-20200823 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project b587ca93be114d07ec3bf654add97d7872325281)
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 errors (new ones prefixed by >>):

>> drivers/net/dsa/mt7530.c:1330:25: error: expected ';' after expression
                                           of_node_put(mac_np)
                                                              ^
                                                              ;
   1 error generated.

# https://github.com/0day-ci/linux/commit/9bc2e04821fee9caceb61a8fa42ab7d85fe73364
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sumera-Priyadarsini/net-dsa-Add-of_node_put-before-break-statement/20200824-025301
git checkout 9bc2e04821fee9caceb61a8fa42ab7d85fe73364
vim +1330 drivers/net/dsa/mt7530.c

  1205	
  1206	static int
  1207	mt7530_setup(struct dsa_switch *ds)
  1208	{
  1209		struct mt7530_priv *priv = ds->priv;
  1210		struct device_node *phy_node;
  1211		struct device_node *mac_np;
  1212		struct mt7530_dummy_poll p;
  1213		phy_interface_t interface;
  1214		struct device_node *dn;
  1215		u32 id, val;
  1216		int ret, i;
  1217	
  1218		/* The parent node of master netdev which holds the common system
  1219		 * controller also is the container for two GMACs nodes representing
  1220		 * as two netdev instances.
  1221		 */
  1222		dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent;
  1223		ds->configure_vlan_while_not_filtering = true;
  1224	
  1225		if (priv->id == ID_MT7530) {
  1226			regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
  1227			ret = regulator_enable(priv->core_pwr);
  1228			if (ret < 0) {
  1229				dev_err(priv->dev,
  1230					"Failed to enable core power: %d\n", ret);
  1231				return ret;
  1232			}
  1233	
  1234			regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
  1235			ret = regulator_enable(priv->io_pwr);
  1236			if (ret < 0) {
  1237				dev_err(priv->dev, "Failed to enable io pwr: %d\n",
  1238					ret);
  1239				return ret;
  1240			}
  1241		}
  1242	
  1243		/* Reset whole chip through gpio pin or memory-mapped registers for
  1244		 * different type of hardware
  1245		 */
  1246		if (priv->mcm) {
  1247			reset_control_assert(priv->rstc);
  1248			usleep_range(1000, 1100);
  1249			reset_control_deassert(priv->rstc);
  1250		} else {
  1251			gpiod_set_value_cansleep(priv->reset, 0);
  1252			usleep_range(1000, 1100);
  1253			gpiod_set_value_cansleep(priv->reset, 1);
  1254		}
  1255	
  1256		/* Waiting for MT7530 got to stable */
  1257		INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
  1258		ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
  1259					 20, 1000000);
  1260		if (ret < 0) {
  1261			dev_err(priv->dev, "reset timeout\n");
  1262			return ret;
  1263		}
  1264	
  1265		id = mt7530_read(priv, MT7530_CREV);
  1266		id >>= CHIP_NAME_SHIFT;
  1267		if (id != MT7530_ID) {
  1268			dev_err(priv->dev, "chip %x can't be supported\n", id);
  1269			return -ENODEV;
  1270		}
  1271	
  1272		/* Reset the switch through internal reset */
  1273		mt7530_write(priv, MT7530_SYS_CTRL,
  1274			     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
  1275			     SYS_CTRL_REG_RST);
  1276	
  1277		/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
  1278		val = mt7530_read(priv, MT7530_MHWTRAP);
  1279		val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
  1280		val |= MHWTRAP_MANUAL;
  1281		mt7530_write(priv, MT7530_MHWTRAP, val);
  1282	
  1283		priv->p6_interface = PHY_INTERFACE_MODE_NA;
  1284	
  1285		/* Enable and reset MIB counters */
  1286		mt7530_mib_reset(ds);
  1287	
  1288		for (i = 0; i < MT7530_NUM_PORTS; i++) {
  1289			/* Disable forwarding by default on all ports */
  1290			mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
  1291				   PCR_MATRIX_CLR);
  1292	
  1293			if (dsa_is_cpu_port(ds, i))
  1294				mt7530_cpu_port_enable(priv, i);
  1295			else
  1296				mt7530_port_disable(ds, i);
  1297	
  1298			/* Enable consistent egress tag */
  1299			mt7530_rmw(priv, MT7530_PVC_P(i), PVC_EG_TAG_MASK,
  1300				   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
  1301		}
  1302	
  1303		/* Setup port 5 */
  1304		priv->p5_intf_sel = P5_DISABLED;
  1305		interface = PHY_INTERFACE_MODE_NA;
  1306	
  1307		if (!dsa_is_unused_port(ds, 5)) {
  1308			priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
  1309			ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
  1310			if (ret && ret != -ENODEV)
  1311				return ret;
  1312		} else {
  1313			/* Scan the ethernet nodes. look for GMAC1, lookup used phy */
  1314			for_each_child_of_node(dn, mac_np) {
  1315				if (!of_device_is_compatible(mac_np,
  1316							     "mediatek,eth-mac"))
  1317					continue;
  1318	
  1319				ret = of_property_read_u32(mac_np, "reg", &id);
  1320				if (ret < 0 || id != 1)
  1321					continue;
  1322	
  1323				phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
  1324				if (!phy_node)
  1325					continue;
  1326	
  1327				if (phy_node->parent == priv->dev->of_node->parent) {
  1328					ret = of_get_phy_mode(mac_np, &interface);
  1329					if (ret && ret != -ENODEV)
> 1330						of_node_put(mac_np)
  1331						return ret;
  1332					id = of_mdio_parse_addr(ds->dev, phy_node);
  1333					if (id == 0)
  1334						priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
  1335					if (id == 4)
  1336						priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
  1337				}
  1338				of_node_put(mac_np);
  1339				of_node_put(phy_node);
  1340				break;
  1341			}
  1342		}
  1343	
  1344		mt7530_setup_port5(ds, interface);
  1345	
  1346		/* Flush the FDB table */
  1347		ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
  1348		if (ret < 0)
  1349			return ret;
  1350	
  1351		return 0;
  1352	}
  1353	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41382 bytes --]