[net-next,07/15] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

Commit Message

Arınç ÜNAL Nov. 18, 2023, 12:31 p.m. UTC

There's no need to run all the code on mt7530_setup_port5() if port 5 is
disabled. The only case for calling mt7530_setup_port5() from
mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
defined as a port on the devicetree, therefore, it cannot be controlled by
phylink.

Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
mt7530_setup_port5().

Stop initialising the interface variable as the remaining cases will always
call mt7530_setup_port5() with it initialised.

Signed-off-by: Arınç ÜNAL <[e-mail]>
Reviewed-by: Vladimir Oltean <[e-mail]>
---
 drivers/net/dsa/mt7530.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Simon Horman Nov. 21, 2023, 6:53 p.m. UTC | #1

On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> There's no need to run all the code on mt7530_setup_port5() if port 5 is
> disabled. The only case for calling mt7530_setup_port5() from
> mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> defined as a port on the devicetree, therefore, it cannot be controlled by
> phylink.
> 
> Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> mt7530_setup_port5().
> 
> Stop initialising the interface variable as the remaining cases will always
> call mt7530_setup_port5() with it initialised.
> 
> Signed-off-by: Arınç ÜNAL <[e-mail]>
> Reviewed-by: Vladimir Oltean <[e-mail]>
> ---
>  drivers/net/dsa/mt7530.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fc87ec817672..1aab4c3f28b0 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>  		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
>  		val &= ~MHWTRAP_P5_DIS;
>  		break;
> -	case P5_DISABLED:
> -		interface = PHY_INTERFACE_MODE_NA;
> -		break;
>  	default:
>  		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
>  			priv->p5_intf_sel);
> @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
>  		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
>  		 * is detected.
>  		 */
> -		interface = PHY_INTERFACE_MODE_NA;
> -
>  		for_each_child_of_node(dn, mac_np) {
>  			if (!of_device_is_compatible(mac_np,
>  						     "mediatek,eth-mac"))
> @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
>  			break;
>  		}
>  
> -		mt7530_setup_port5(ds, interface);
> +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> +			mt7530_setup_port5(ds, interface);

Hi Arınç,

It appears that interface is now uninitialised here.

Flagged by Smatch.

>  	}
>  
>  #ifdef CONFIG_GPIOLIB
> -- 
> 2.40.1
>

Arınç ÜNAL Dec. 2, 2023, 8:45 a.m. UTC | #2

Hi Simon.

On 21.11.2023 21:53, Simon Horman wrote:
> On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
>> There's no need to run all the code on mt7530_setup_port5() if port 5 is
>> disabled. The only case for calling mt7530_setup_port5() from
>> mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
>> defined as a port on the devicetree, therefore, it cannot be controlled by
>> phylink.
>>
>> Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
>> P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
>> mt7530_setup_port5().
>>
>> Stop initialising the interface variable as the remaining cases will always
>> call mt7530_setup_port5() with it initialised.
>>
>> Signed-off-by: Arınç ÜNAL <[e-mail]>
>> Reviewed-by: Vladimir Oltean <[e-mail]>
>> ---
>>   drivers/net/dsa/mt7530.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index fc87ec817672..1aab4c3f28b0 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>>   		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
>>   		val &= ~MHWTRAP_P5_DIS;
>>   		break;
>> -	case P5_DISABLED:
>> -		interface = PHY_INTERFACE_MODE_NA;
>> -		break;
>>   	default:
>>   		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
>>   			priv->p5_intf_sel);
>> @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
>>   		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
>>   		 * is detected.
>>   		 */
>> -		interface = PHY_INTERFACE_MODE_NA;
>> -
>>   		for_each_child_of_node(dn, mac_np) {
>>   			if (!of_device_is_compatible(mac_np,
>>   						     "mediatek,eth-mac"))
>> @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
>>   			break;
>>   		}
>>   
>> -		mt7530_setup_port5(ds, interface);
>> +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
>> +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
>> +			mt7530_setup_port5(ds, interface);
> 
> Hi Arınç,
> 
> It appears that interface is now uninitialised here.
> 
> Flagged by Smatch.

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);

Arınç

Russell King (Oracle) Dec. 2, 2023, 9:30 a.m. UTC | #3

On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> Hi Simon.
> 
> On 21.11.2023 21:53, Simon Horman wrote:
> > On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> > > There's no need to run all the code on mt7530_setup_port5() if port 5 is
> > > disabled. The only case for calling mt7530_setup_port5() from
> > > mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> > > defined as a port on the devicetree, therefore, it cannot be controlled by
> > > phylink.
> > > 
> > > Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> > > P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> > > mt7530_setup_port5().
> > > 
> > > Stop initialising the interface variable as the remaining cases will always
> > > call mt7530_setup_port5() with it initialised.
> > > 
> > > Signed-off-by: Arınç ÜNAL <[e-mail]>
> > > Reviewed-by: Vladimir Oltean <[e-mail]>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 9 +++------
> > >   1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index fc87ec817672..1aab4c3f28b0 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> > >   		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
> > >   		val &= ~MHWTRAP_P5_DIS;
> > >   		break;
> > > -	case P5_DISABLED:
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -		break;
> > >   	default:
> > >   		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
> > >   			priv->p5_intf_sel);
> > > @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
> > >   		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
> > >   		 * is detected.
> > >   		 */
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -
> > >   		for_each_child_of_node(dn, mac_np) {
> > >   			if (!of_device_is_compatible(mac_np,
> > >   						     "mediatek,eth-mac"))
> > > @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
> > >   			break;
> > >   		}
> > > -		mt7530_setup_port5(ds, interface);
> > > +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> > > +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > > +			mt7530_setup_port5(ds, interface);
> > 
> > Hi Arınç,
> > 
> > It appears that interface is now uninitialised here.
> > 
> > Flagged by Smatch.
> 
> 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.

It's probably due to the complexities involved in analysing the values
of variables, especially when they're in structures that are passed in.

Simon Horman Dec. 6, 2023, 9:46 p.m. UTC | #4

+ Dan Carpenter <[e-mail]>

On Sat, Dec 02, 2023 at 11:45:42AM +0300, Arınç ÜNAL wrote:
> Hi Simon.
> 
> On 21.11.2023 21:53, Simon Horman wrote:
> > On Sat, Nov 18, 2023 at 03:31:57PM +0300, Arınç ÜNAL wrote:
> > > There's no need to run all the code on mt7530_setup_port5() if port 5 is
> > > disabled. The only case for calling mt7530_setup_port5() from
> > > mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
> > > defined as a port on the devicetree, therefore, it cannot be controlled by
> > > phylink.
> > > 
> > > Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
> > > P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
> > > mt7530_setup_port5().
> > > 
> > > Stop initialising the interface variable as the remaining cases will always
> > > call mt7530_setup_port5() with it initialised.
> > > 
> > > Signed-off-by: Arınç ÜNAL <[e-mail]>
> > > Reviewed-by: Vladimir Oltean <[e-mail]>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 9 +++------
> > >   1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index fc87ec817672..1aab4c3f28b0 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -942,9 +942,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
> > >   		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
> > >   		val &= ~MHWTRAP_P5_DIS;
> > >   		break;
> > > -	case P5_DISABLED:
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -		break;
> > >   	default:
> > >   		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
> > >   			priv->p5_intf_sel);
> > > @@ -2313,8 +2310,6 @@ mt7530_setup(struct dsa_switch *ds)
> > >   		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
> > >   		 * is detected.
> > >   		 */
> > > -		interface = PHY_INTERFACE_MODE_NA;
> > > -
> > >   		for_each_child_of_node(dn, mac_np) {
> > >   			if (!of_device_is_compatible(mac_np,
> > >   						     "mediatek,eth-mac"))
> > > @@ -2346,7 +2341,9 @@ mt7530_setup(struct dsa_switch *ds)
> > >   			break;
> > >   		}
> > > -		mt7530_setup_port5(ds, interface);
> > > +		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
> > > +		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
> > > +			mt7530_setup_port5(ds, interface);
> > 
> > Hi Arınç,
> > 
> > It appears that interface is now uninitialised here.
> > 
> > Flagged by Smatch.
> 
> 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.

Yes, I see your point now. At a guess, perhaps it because:

1. It doesn't know that of_get_phy_mode will set the value of interface
2. It doesn't know if the loop will run (more than zero times)

I CCed Dan Carpenter, who is surely more knowledgeable about this than I,
in case he wants to add anything.

> 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);
> 
> Arınç

Dan Carpenter Dec. 7, 2023, 6:51 a.m. UTC | #5

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

Vladimir Oltean Dec. 7, 2023, 6:40 p.m. UTC | #6

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.

Vladimir Oltean Dec. 7, 2023, 8:01 p.m. UTC | #7

On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> 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.

I noticed just now that there is further filtering on the child OF node
by of_device_is_compatible(). In that case, of_get_compatible_child()
could be used to eliminate for_each_child_of_node() completely.

Dan Carpenter Dec. 8, 2023, 4:23 a.m. UTC | #8

On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> 
> 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);

This doesn't solve the problem that Smatch doesn't know what the
original value of priv->p5_intf_sel.  And also I don't like this code
because now we call mt7530_setup_port5() on every iteration after
we find the first P5_INTF_SEL_PHY_P0.

> 	}
> 	of_node_put(mac_np);
> 	of_node_put(phy_node);
> 	break;
> }

Let's not worry too much about false positives.  We can just ignore
them.  There is always a trade off between false positives and false
negatives.  With GCC we try to get a clean run with no warnings, but
with Smatch I'm targetting the false positive ratio at "this is worth
reviewing one time."

regards,
dan carpenter

Vladimir Oltean Dec. 8, 2023, 6:46 p.m. UTC | #9

On Fri, Dec 08, 2023 at 07:23:38AM +0300, Dan Carpenter wrote:
> On Thu, Dec 07, 2023 at 08:40:15PM +0200, Vladimir Oltean wrote:
> > 
> > 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);
> 
> This doesn't solve the problem that Smatch doesn't know what the
> original value of priv->p5_intf_sel.  And also I don't like this code
> because now we call mt7530_setup_port5() on every iteration after
> we find the first P5_INTF_SEL_PHY_P0.

You seem to have not parsed the "break" from 4 lines below. There is at
most one iteration through for_each_child_of_node().

And why would the "original" value of priv->p5_intf_sel matter? Original
or modified by the "if (id == 0)" and "if (id == 4)" blocks, the code
has already executed the of_get_phy_mode(&interface) call, by the time
we reach the "if" that calls mt7530_setup_port5().

Hmm, maybe the problem, all along, was that we let the -ENODEV return
code from of_get_phy_mode() pass through? "interface" will really be
uninitialized in that case. It's not a false positive.

Instead of:

	ret = of_get_phy_mode(mac_np, &interface);
	if (ret && ret != -ENODEV) {
		...
		return ret;
	}

it should have been like this, to not complain:

	ret = of_get_phy_mode(mac_np, &interface);
	if (ret) {
		...
		return ret;
	}

> > 	}
> > 	of_node_put(mac_np);
> > 	of_node_put(phy_node);
> > 	break;
> > }

Arınç ÜNAL Dec. 17, 2023, 12:22 p.m. UTC | #10

On 8.12.2023 21:46, Vladimir Oltean wrote:
> Hmm, maybe the problem, all along, was that we let the -ENODEV return
> code from of_get_phy_mode() pass through? "interface" will really be
> uninitialized in that case. It's not a false positive.
> 
> Instead of:
> 
> 	ret = of_get_phy_mode(mac_np, &interface);
> 	if (ret && ret != -ENODEV) {
> 		...
> 		return ret;
> 	}
> 
> it should have been like this, to not complain:
> 
> 	ret = of_get_phy_mode(mac_np, &interface);
> 	if (ret) {
> 		...
> 		return ret;
> 	}
> 

I just tried this, smatch still reports "interface" as uninitialised.

$ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
$ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c

   UPD     include/config/kernel.release
   UPD     include/generated/utsrelease.h
   CHECK   scripts/mod/empty.c
   CALL    scripts/checksyscalls.sh
   CC      drivers/net/dsa/mt7530.o
   CHECK   drivers/net/dsa/mt7530.c
drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.

Arınç

Dan Carpenter Jan. 2, 2024, 11:16 a.m. UTC | #11

On Sun, Dec 17, 2023 at 03:22:27PM +0300, Arınç ÜNAL wrote:
> On 8.12.2023 21:46, Vladimir Oltean wrote:
> > Hmm, maybe the problem, all along, was that we let the -ENODEV return
> > code from of_get_phy_mode() pass through? "interface" will really be
> > uninitialized in that case. It's not a false positive.
> > 
> > Instead of:
> > 
> > 	ret = of_get_phy_mode(mac_np, &interface);
> > 	if (ret && ret != -ENODEV) {
> > 		...
> > 		return ret;
> > 	}
> > 
> > it should have been like this, to not complain:
> > 
> > 	ret = of_get_phy_mode(mac_np, &interface);
> > 	if (ret) {
> > 		...
> > 		return ret;
> > 	}
> > 
> 
> I just tried this, smatch still reports "interface" as uninitialised.
> 
> $ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
> 
>   UPD     include/config/kernel.release
>   UPD     include/generated/utsrelease.h
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   CC      drivers/net/dsa/mt7530.o
>   CHECK   drivers/net/dsa/mt7530.c
> drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
> drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
> drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.

That's so strange.

Vladimir was right that I was misreading what he said and also hadn't
noticed the break.

For me, his approach silences the warning with or without the cross
function DB.  Also of_get_phy_mode() initializes interface on all paths
so checking for -EINVAL doesn't matter as far as this warning is
concerned.

regards,
dan carpenter

Arınç ÜNAL Jan. 6, 2024, 6:01 p.m. UTC | #12

On 1/2/24 13:16, Dan Carpenter wrote:
> On Sun, Dec 17, 2023 at 03:22:27PM +0300, Arınç ÜNAL wrote:
>> On 8.12.2023 21:46, Vladimir Oltean wrote:
>>> Hmm, maybe the problem, all along, was that we let the -ENODEV return
>>> code from of_get_phy_mode() pass through? "interface" will really be
>>> uninitialized in that case. It's not a false positive.
>>>
>>> Instead of:
>>>
>>> 	ret = of_get_phy_mode(mac_np, &interface);
>>> 	if (ret && ret != -ENODEV) {
>>> 		...
>>> 		return ret;
>>> 	}
>>>
>>> it should have been like this, to not complain:
>>>
>>> 	ret = of_get_phy_mode(mac_np, &interface);
>>> 	if (ret) {
>>> 		...
>>> 		return ret;
>>> 	}
>>>
>>
>> I just tried this, smatch still reports "interface" as uninitialised.
>>
>> $ export ARCH=mips CROSS_COMPILE=mips-linux-gnu-
>> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
>>
>>    UPD     include/config/kernel.release
>>    UPD     include/generated/utsrelease.h
>>    CHECK   scripts/mod/empty.c
>>    CALL    scripts/checksyscalls.sh
>>    CC      drivers/net/dsa/mt7530.o
>>    CHECK   drivers/net/dsa/mt7530.c
>> drivers/net/dsa/mt7530.c:217 mt7530_mii_read() warn: call of 'warn_slowpath_fmt' with non-constant format argument
>> drivers/net/dsa/mt7530.c:454 mt7530_setup_port6() error: uninitialized symbol 'ncpo1'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
>> drivers/net/dsa/mt7530.c:868 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
>> drivers/net/dsa/mt7530.c:2324 mt7530_setup() error: uninitialized symbol 'interface'.
> 
> That's so strange.
> 
> Vladimir was right that I was misreading what he said and also hadn't
> noticed the break.
> 
> For me, his approach silences the warning with or without the cross
> function DB.  Also of_get_phy_mode() initializes interface on all paths
> so checking for -EINVAL doesn't matter as far as this warning is
> concerned.

I must be missing something.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 7f9d63b61168..05ce3face47c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2325,7 +2325,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) {
+				if (ret) {
  					of_node_put(mac_np);
  					of_node_put(phy_node);
  					return ret;

$ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
   CHECK   scripts/mod/empty.c
   CALL    scripts/checksyscalls.sh
   DESCEND objtool
   INSTALL libsubcmd_headers
   CC      drivers/net/dsa/mt7530.o
   CHECK   drivers/net/dsa/mt7530.c
drivers/net/dsa/mt7530.c:469 mt7530_pad_clk_setup() error: uninitialized symbol 'ncpo1'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
drivers/net/dsa/mt7530.c:2346 mt7530_setup() error: uninitialized symbol 'interface'.

Just so you know, I intend to remove this whole PHY muxing feature once I
bring changing DSA conduit support to this subdriver. I've got two strong
reasons for this.

- Changing the DSA conduit achieves the same result with the only overhead
   being the DSA header included on every frame.

- There can't be proper dt-bindings for it as the nature of the feature
   shows that it represents an optional way to operate the hardware, it does
   not represent a hardware design. Overall, the implementation is a hack to
   make it work for specific hardware (switch must be connected to gmac1 of
   a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
   of the SoC. It should rather be configurable on userspace. Which will
   never happen as it is specific to this switch and the changing DSA
   conduit feature is the perfect substitute for this.

Let me know if you've got any suggestions that can get rid of the warning
without reworking the whole code block. Otherwise, I'm just going to ignore
it until I get rid of the whole code block.

Arınç

Vladimir Oltean Jan. 9, 2024, 2:57 p.m. UTC | #13

On Sat, Jan 06, 2024 at 08:01:25PM +0200, Arınç ÜNAL wrote:
> I must be missing something.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 7f9d63b61168..05ce3face47c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2325,7 +2325,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) {
> +				if (ret) {
>  					of_node_put(mac_np);
>  					of_node_put(phy_node);
>  					return ret;
> 
> $ ../smatch/smatch_scripts/kchecker --spammy drivers/net/dsa/mt7530.c
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   DESCEND objtool
>   INSTALL libsubcmd_headers
>   CC      drivers/net/dsa/mt7530.o
>   CHECK   drivers/net/dsa/mt7530.c
> drivers/net/dsa/mt7530.c:469 mt7530_pad_clk_setup() error: uninitialized symbol 'ncpo1'.
> drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_count'.
> drivers/net/dsa/mt7530.c:895 mt7530_set_ageing_time() error: uninitialized symbol 'age_unit'.
> drivers/net/dsa/mt7530.c:2346 mt7530_setup() error: uninitialized symbol 'interface'.

Yes, well _now_ it is a false positive, probably because smatch cannot
determine that when priv->p5_intf_sel has been set to P5_INTF_SEL_PHY_P0
or P5_INTF_SEL_PHY_P4, "interface" should have been also initialized.
But it doesn't matter, you can ignore a false positive. I'm also seeing it.
Although you should check whether treating -ENODEV as a hard error is fine
and won't cause regressions.

> Just so you know, I intend to remove this whole PHY muxing feature once I
> bring changing DSA conduit support to this subdriver. I've got two strong
> reasons for this.
> - Changing the DSA conduit achieves the same result with the only overhead
>   being the DSA header included on every frame.
> 
> - There can't be proper dt-bindings for it as the nature of the feature
>   shows that it represents an optional way to operate the hardware, it does
>   not represent a hardware design. Overall, the implementation is a hack to
>   make it work for specific hardware (switch must be connected to gmac1 of
>   a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
>   of the SoC. It should rather be configurable on userspace. Which will
>   never happen as it is specific to this switch and the changing DSA
>   conduit feature is the perfect substitute for this.

Is PHY muxing a "true" switch bypass, or is it just a route through the
switch for all packets coming from GMAC5 to go to phy0 or phy4? If the
latter, I agree that dynamic conduit changing is a more flexible option,
not to mention the user space tooling is already there.

Are there existing systems that use PHY muxing? The possible problem I
see is breaking those boards which have a phy-handle on gmac5, if the
mt7530 driver is no longer going to modify its HWTRAP register.

> 
> Let me know if you've got any suggestions that can get rid of the warning
> without reworking the whole code block. Otherwise, I'm just going to ignore
> it until I get rid of the whole code block.

The obvious way would be to leave the initialization to PHY_INTERFACE_MODE_NA
there. Or to just ignore the warning.

Arınç ÜNAL Jan. 10, 2024, 7:26 a.m. UTC | #14

On 9.01.2024 17:57, Vladimir Oltean wrote:
> Yes, well _now_ it is a false positive, probably because smatch cannot
> determine that when priv->p5_intf_sel has been set to P5_INTF_SEL_PHY_P0
> or P5_INTF_SEL_PHY_P4, "interface" should have been also initialized.
> But it doesn't matter, you can ignore a false positive. I'm also seeing it.
> Although you should check whether treating -ENODEV as a hard error is fine
> and won't cause regressions.
> 
>> Just so you know, I intend to remove this whole PHY muxing feature once I
>> bring changing DSA conduit support to this subdriver. I've got two strong
>> reasons for this.
>> - Changing the DSA conduit achieves the same result with the only overhead
>>    being the DSA header included on every frame.
>>
>> - There can't be proper dt-bindings for it as the nature of the feature
>>    shows that it represents an optional way to operate the hardware, it does
>>    not represent a hardware design. Overall, the implementation is a hack to
>>    make it work for specific hardware (switch must be connected to gmac1 of
>>    a MediaTek SoC, no PHY must be present at address 0 or 4 on the MDIO bus
>>    of the SoC. It should rather be configurable on userspace. Which will
>>    never happen as it is specific to this switch and the changing DSA
>>    conduit feature is the perfect substitute for this.
> 
> Is PHY muxing a "true" switch bypass, or is it just a route through the
> switch for all packets coming from GMAC5 to go to phy0 or phy4? If the
> latter, I agree that dynamic conduit changing is a more flexible option,
> not to mention the user space tooling is already there.

It's the latter, and that's exactly what I think.

> 
> Are there existing systems that use PHY muxing? The possible problem I
> see is breaking those boards which have a phy-handle on gmac5, if the
> mt7530 driver is no longer going to modify its HWTRAP register.

Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
We don't even define gmac5 as a port on the switch dt-bindings.

While none of the DTs on the Linux repository utilise this, some of the
mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
will be inaccessible from the SoC MAC's network interface. I de-facto
maintain the mt7621 device tree source files there. I intend to revert it
along with adding port 5 as a CPU port so that the conduit changing feature
becomes available.

> 
>>
>> Let me know if you've got any suggestions that can get rid of the warning
>> without reworking the whole code block. Otherwise, I'm just going to ignore
>> it until I get rid of the whole code block.
> 
> The obvious way would be to leave the initialization to PHY_INTERFACE_MODE_NA
> there. Or to just ignore the warning.

I'll ignore.

Arınç

Vladimir Oltean Jan. 10, 2024, 6:23 p.m. UTC | #15

On Wed, Jan 10, 2024 at 10:26:54AM +0300, Arınç ÜNAL wrote:
> > Are there existing systems that use PHY muxing? The possible problem I
> > see is breaking those boards which have a phy-handle on gmac5, if the
> > mt7530 driver is no longer going to modify its HWTRAP register.
> 
> Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
> on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
> We don't even define gmac5 as a port on the switch dt-bindings.

I noticed that from the code already. Maybe I shouldn't have said
"gmac5" when I meant "the GMAC attached to switch port 5, aka GMAC0".
I was under the impression that you were also using this slightly
incorrect terminology, to keep a numerical association between the CPU
port number and its directly attached GMAC.

> While none of the DTs on the Linux repository utilise this, some of the
> mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
> will be inaccessible from the SoC MAC's network interface. I de-facto
> maintain the mt7621 device tree source files there. I intend to revert it
> along with adding port 5 as a CPU port so that the conduit changing feature
> becomes available.

If OpenWrt kernels are always shipped in tandem with updated device
trees (i.e. no Arm SystemReady IR platforms, where the DT is provided by
U-Boot), I won't oppose to retracting features described via DT if their
platform maintainers agree in a wide enough circle that the breakage is
manageable.

BTW, besides OpenWrt, what other software is deployed on these SoCs
typically?

Arınç ÜNAL Jan. 11, 2024, 10:22 a.m. UTC | #16

On 10.01.2024 21:23, Vladimir Oltean wrote:
> On Wed, Jan 10, 2024 at 10:26:54AM +0300, Arınç ÜNAL wrote:
>>> Are there existing systems that use PHY muxing? The possible problem I
>>> see is breaking those boards which have a phy-handle on gmac5, if the
>>> mt7530 driver is no longer going to modify its HWTRAP register.
>>
>> Ah see, for PHY muxing, the driver actually wants the phy-handle to be put
>> on the SoC MAC, and the PHY to be defined on the SoC ethernet's MDIO bus.
>> We don't even define gmac5 as a port on the switch dt-bindings.
> 
> I noticed that from the code already. Maybe I shouldn't have said
> "gmac5" when I meant "the GMAC attached to switch port 5, aka GMAC0".
> I was under the impression that you were also using this slightly
> incorrect terminology, to keep a numerical association between the CPU
> port number and its directly attached GMAC.
> 
>> While none of the DTs on the Linux repository utilise this, some of the
>> mt7621 DTs on OpenWrt do. The change in behaviour will only be that phy0/4
>> will be inaccessible from the SoC MAC's network interface. I de-facto
>> maintain the mt7621 device tree source files there. I intend to revert it
>> along with adding port 5 as a CPU port so that the conduit changing feature
>> becomes available.
> 
> If OpenWrt kernels are always shipped in tandem with updated device
> trees (i.e. no Arm SystemReady IR platforms, where the DT is provided by
> U-Boot), I won't oppose to retracting features described via DT if their
> platform maintainers agree in a wide enough circle that the breakage is
> manageable.

I will see to this when the time comes.

> 
> BTW, besides OpenWrt, what other software is deployed on these SoCs
> typically?

Other than OpenWrt which is widely used for these SoCs for its ease of
flashing and upgrading, compatibility with legacy U-boot versions that
usually come with any vendor making a product out of these SoCs, I can only
talk about what I deploy to run Linux. I use mainline U-Boot along with the
device trees from the Linux repository to boot mainline Linux kernels with
Buildroot as the filesystem.

Arınç

Vladimir Oltean Jan. 11, 2024, 10:31 a.m. UTC | #17

On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
> > BTW, besides OpenWrt, what other software is deployed on these SoCs
> > typically?
> 
> Other than OpenWrt which is widely used for these SoCs for its ease of
> flashing and upgrading, compatibility with legacy U-boot versions that
> usually come with any vendor making a product out of these SoCs, I can only
> talk about what I deploy to run Linux. I use mainline U-Boot along with the
> device trees from the Linux repository to boot mainline Linux kernels with
> Buildroot as the filesystem.
> 
> Arınç

I meant what other software (operating system) _instead_ of OpenWRT.
I'm trying to figure out what other users of PHY muxing there might be.

Frank Wunderlich Jan. 11, 2024, 10:35 a.m. UTC | #18

Am 11. Januar 2024 11:31:46 MEZ schrieb Vladimir Oltean <[e-mail]>:
>On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
>> > BTW, besides OpenWrt, what other software is deployed on these SoCs
>> > typically?
>> 
>> Other than OpenWrt which is widely used for these SoCs for its ease of
>> flashing and upgrading, compatibility with legacy U-boot versions that
>> usually come with any vendor making a product out of these SoCs, I can only
>> talk about what I deploy to run Linux. I use mainline U-Boot along with the
>> device trees from the Linux repository to boot mainline Linux kernels with
>> Buildroot as the filesystem.
>> 
>> Arınç
>
>I meant what other software (operating system) _instead_ of OpenWRT.
>I'm trying to figure out what other users of PHY muxing there might be.

Hi

I (and possibly others) use debian/ubuntu and there are also archlinux images for the bpi router boards.


regards Frank

Arınç ÜNAL Jan. 11, 2024, 10:59 a.m. UTC | #19

On 11.01.2024 13:31, Vladimir Oltean wrote:
> On Thu, Jan 11, 2024 at 01:22:12PM +0300, Arınç ÜNAL wrote:
>>> BTW, besides OpenWrt, what other software is deployed on these SoCs
>>> typically?
>>
>> Other than OpenWrt which is widely used for these SoCs for its ease of
>> flashing and upgrading, compatibility with legacy U-boot versions that
>> usually come with any vendor making a product out of these SoCs, I can only
>> talk about what I deploy to run Linux. I use mainline U-Boot along with the
>> device trees from the Linux repository to boot mainline Linux kernels with
>> Buildroot as the filesystem.
>>
>> Arınç
> 
> I meant what other software (operating system) _instead_ of OpenWRT.
> I'm trying to figure out what other users of PHY muxing there might be.

I'm not aware of any other projects that utilise PHY muxing of MT7530. It
was me spending a few months bringing the feature to OpenWrt in the first
place.

Arınç

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fc87ec817672..1aab4c3f28b0 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -942,9 +942,6 @@  static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
 		val &= ~MHWTRAP_P5_DIS;
 		break;
-	case P5_DISABLED:
-		interface = PHY_INTERFACE_MODE_NA;
-		break;
 	default:
 		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
 			priv->p5_intf_sel);
@@ -2313,8 +2310,6 @@  mt7530_setup(struct dsa_switch *ds)
 		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
 		 * is detected.
 		 */
-		interface = PHY_INTERFACE_MODE_NA;
-
 		for_each_child_of_node(dn, mac_np) {
 			if (!of_device_is_compatible(mac_np,
 						     "mediatek,eth-mac"))
@@ -2346,7 +2341,9 @@  mt7530_setup(struct dsa_switch *ds)
 			break;
 		}
 
-		mt7530_setup_port5(ds, interface);
+		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
+		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
+			mt7530_setup_port5(ds, interface);
 	}
 
 #ifdef CONFIG_GPIOLIB