netdev - Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run mt7530_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231207184015.u7uoyfhdxiyuw6hh@skbuf>
Date: Thu, 7 Dec 2023 20:40:15 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
	Simon Horman <horms@...nel.org>,
	Daniel Golle <daniel@...rotopia.org>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Frank Wunderlich <frank-w@...lic-files.de>,
	Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
	mithat.guner@...ont.com, erkin.bozoglu@...ont.com
Subject: Re: [PATCH net-next 07/15] net: dsa: mt7530: do not run
 mt7530_setup_port5() if port 5 is disabled

On Thu, Dec 07, 2023 at 09:51:07AM +0300, Dan Carpenter wrote:
> On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> > 
> > I'm not sure why it doesn't catch that for mt7530_setup_port5() to run
> > here, priv->p5_intf_sel must be either P5_INTF_SEL_PHY_P0 or
> > P5_INTF_SEL_PHY_P4. And for that to happen, the interface variable will be
> > initialised.
> > 
> > for_each_child_of_node(dn, mac_np) {
> > 	if (!of_device_is_compatible(mac_np,
> > 				     "mediatek,eth-mac"))
> > 		continue;
> > 
> > 	ret = of_property_read_u32(mac_np, "reg", &id);
> > 	if (ret < 0 || id != 1)
> > 		continue;
> > 
> > 	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
> > 	if (!phy_node)
> > 		continue;
> > 
> > 	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);
> > 			of_node_put(phy_node);
> > 			return ret;
> > 		}
> > 		id = of_mdio_parse_addr(ds->dev, phy_node);
> > 		if (id == 0)
> > 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
> > 		if (id == 4)
> > 			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
> > 	}
> > 	of_node_put(mac_np);
> > 	of_node_put(phy_node);
> > 	break;
> > }
> > 
> > if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> >     priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > 	mt7530_setup_port5(ds, interface);
> 
> Smatch doesn't know:
> 1) What the value of priv->p5_intf_sel is going into this function
> 2) We enter the for_each_child_of_node() loop
> 3) That if (phy_node->parent == priv->dev->of_node->parent) { is
>    definitely true for one element on the list.
> 
> Looking at how Smatch parses this code, I could probably improve problem
> #1 a bit.  Right now Smatch sees "struct mt7530_priv *priv = ds->priv;"
> and "priv->p5_intf_sel" is unknown, but I could probably improve it to
> where it says that it's in the 1-3 range.  But that doesn't help here
> and it doesn't address problems 2 and 3.
> 
> It's a hard problem.
> 
> regards,
> dan carpenter
> 

We could be more pragmatic about this whole sparse false positive warning,
and just move the "if" block which calls mt7530_setup_port5() right
after the priv->p5_intf_sel assignments, instead of waiting to "break;"
from the for_each_child_of_node() loop.

for_each_child_of_node(dn, mac_np) {
	if (!of_device_is_compatible(mac_np,
				     "mediatek,eth-mac"))
		continue;

	ret = of_property_read_u32(mac_np, "reg", &id);
	if (ret < 0 || id != 1)
		continue;

	phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
	if (!phy_node)
		continue;

	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);
			of_node_put(phy_node);
			return ret;
		}
		id = of_mdio_parse_addr(ds->dev, phy_node);
		if (id == 0)
			priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
		if (id == 4)
			priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;

		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 || <---- here
		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
			mt7530_setup_port5(ds, interface);
	}
	of_node_put(mac_np);
	of_node_put(phy_node);
	break;
}

I hope it's now much clearer to sparse that "interface" is used within
the same basic block in which it also got assigned, and that determination
does not depend upon the values taken by a second variable.

Maybe it's also a bit clearer for us humans.

What would also help us humans even more is to extract the entire "dn"
handling from mt7530_setup() into a separate mt7530_setup_phy_muxing()
function, and put a good comment there about what's going on with this
PHY muxing thing.

The advantage of splitting this up is that we don't pollute mt7530_setup()
with finding the "dn" if dsa_is_unused_port(ds, 5) returns false.

Also, reducing the indentation level of for_each_child_of_node() by one
can't be bad. Maybe even by more. There's this pattern:

for_each_child_of_node(dn, mac_np) {
	// do stuff with mac_np
	break;
}

aka we only care about the first child of dn. We could find the mac_np
as the only operation inside for_each_child_of_node(), break directly,
and "do stuff with mac_np" could be done outside, further reducing the
indentation by 1 level.