Re: [PATCH net-next 01/15] net: dsa: mt7530: always trap frames to active CPU port on MT7530

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

On 18.11.2023 17:34, Russell King (Oracle) wrote:
> On Sat, Nov 18, 2023 at 03:31:51PM +0300, Arınç ÜNAL wrote:
>> +	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
>> +	 * forwarded to the numerically smallest CPU port which the DSA conduit
>> +	 * interface its affine to is up.
>> +	 */
>> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
>> +		return;
>> +
>> +	if (operational)
>> +		priv->active_cpu_ports |= BIT(cpu_dp->index);
>> +	else
>> +		priv->active_cpu_ports &= ~BIT(cpu_dp->index);
>> +
>> +	if (priv->active_cpu_ports)
>> +		mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, CPU_EN |
>> +			   CPU_PORT(__ffs(priv->active_cpu_ports)));
> 
> I would be tempted to write this as:
> 
> 	mask = BIT(cpu_dp->index);
> 
> 	if (operational)
> 		priv->active_cpu_ports |= mask;
> 	else
> 		priv->active_cpu_ports &= ~mask;
> 
> Now, what happens when active_cpu_ports is zero? Doesn't that mean there
> is no active CPU port? In which case, wouldn't disabling the CPU port
> direction be appropriate, such as:
> 
> 	if (priv->active_cpu_ports)
> 		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
> 	else
> 		val = 0;
> 
> 	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);	
> 
> ?

In practice, it doesn't seem to matter. The CPU_EN bit enables the CPU port
defined on CPU_PORT_MASK which is used for trapping frames. No active CPU
ports would mean that all the DSA conduits are down. In that case, all the
user ports will be down also. So there won't be any traffic. But disabling
it is of course more appropriate here.

> 
>>   struct mt7530_priv {
>>   	struct device		*dev;
>> @@ -786,6 +787,7 @@ struct mt7530_priv {
>>   	struct irq_domain *irq_domain;
>>   	u32 irq_enable;
>>   	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
>> +	unsigned long active_cpu_ports;
> 
> So this will be 32 or 64 bit in size. Presumably you know how many CPU
> ports there can be, which looking at this code must be less than 8 as
> CPU_PORT_MASK is only 3 bits in size. So, maybe use a u8, and check
> that cpu_dp->index <= 7 ?

Aren't there other mechanisms to check that cpu_dp->index is a valid port?
At least with phylink_get_caps(), only ports lower than 7 will have proper
interface modes allowed.

Here's the code after you and Vladimir's review:

static void
mt753x_conduit_state_change(struct dsa_switch *ds,
			    const struct net_device *conduit,
			    bool operational)
{
	struct dsa_port *cpu_dp = conduit->dsa_ptr;
	struct mt7530_priv *priv = ds->priv;
	u8 mask;
	int val;

	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
	 * forwarded to the numerically smallest CPU port which the DSA conduit
	 * interface its affine to is up.
	 */
	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
		return;

	mask = BIT(cpu_dp->index);

	if (operational)
		priv->active_cpu_ports |= mask;
	else
		priv->active_cpu_ports &= ~mask;

	if (priv->active_cpu_ports)
		val = CPU_EN | CPU_PORT(__ffs(priv->active_cpu_ports));
	else
		val = 0;

	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
}

struct mt7530_priv {
	[...]
	u8 active_cpu_ports;
};

> 
> I would also suggest moving irq_enable after create_sgmii, to avoid
> holes in the struct.

Sorry, I've got no idea about this. Could you explain why would there
possibly be holes in the struct with the current ordering of the members of
the mt7530_priv structure?

Arınç