Re: [PATCH net-next 5/6] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

On 10/13/21 11:55 AM, DENG Qingfang wrote:
> On Tue, Oct 12, 2021 at 02:35:54PM +0200, Alvin Šipraga wrote:
>> +/* Port mapping macros
>> + *
>> + * PORT_NUM_x2y: map a port number from domain x to domain y
>> + * PORT_MASK_x2y: map a port mask from domain x to domain y
>> + *
>> + * L = logical port domain, i.e. dsa_port.index
>> + * P = physical port domain, used by the Realtek ASIC for port indexing;
>> + *     for ports with internal PHYs, this is also the PHY index
>> + * E = extension port domain, used by the Realtek ASIC for managing EXT ports
>> + *
>> + * The terminology is borrowed from the vendor driver. The extension port domain
>> + * is mostly used to navigate the labyrinthine layout of EXT port configuration
>> + * registers and is not considered intuitive by the author.
>> + *
>> + * Unless a function is accessing chip registers, it should be using the logical
>> + * port domain. Moreover, function arguments for port numbers and port masks
>> + * must always be in the logical domain. The conversion must be done as close as
>> + * possible to the register access to avoid chaos.
>> + *
>> + * The mappings vary between chips in the family supported by this driver. Here
>> + * is an example of the mapping for the RTL8365MB-VC:
>> + *
>> + *    L | P | E | remark
>> + *   ---+---+---+--------
>> + *    0 | 0 |   | user port
>> + *    1 | 1 |   | user port
>> + *    2 | 2 |   | user port
>> + *    3 | 3 |   | user port
>> + *    4 | 6 | 1 | extension (CPU) port
>> + *
>> + * NOTE: Currently this is hardcoded for the RTL8365MB-VC. This will probably
>> + * require a rework when adding support for other chips.
>> + */
>> +#define CPU_PORT_LOGICAL_NUM	4
>> +#define CPU_PORT_LOGICAL_MASK	BIT(CPU_PORT_LOGICAL_NUM)
>> +#define CPU_PORT_PHYSICAL_NUM	6
>> +#define CPU_PORT_PHYSICAL_MASK	BIT(CPU_PORT_PHYSICAL_NUM)
>> +#define CPU_PORT_EXTENSION_NUM	1
>> +
>> +static u32 rtl8365mb_port_num_l2p(u32 port)
>> +{
>> +	return port == CPU_PORT_LOGICAL_NUM ? CPU_PORT_PHYSICAL_NUM : port;
>> +}
>> +
>> +static u32 rtl8365mb_port_mask_l2p(u32 mask)
>> +{
>> +	u32 phys_mask = mask & ~CPU_PORT_LOGICAL_MASK;
>> +
>> +	if (mask & CPU_PORT_LOGICAL_MASK)
>> +		phys_mask |= CPU_PORT_PHYSICAL_MASK;
>> +
>> +	return phys_mask;
>> +}
>> +
>> +static u32 rtl8365mb_port_mask_p2l(u32 phys_mask)
>> +{
>> +	u32 mask = phys_mask & ~CPU_PORT_PHYSICAL_MASK;
>> +
>> +	if (phys_mask & CPU_PORT_PHYSICAL_MASK)
>> +		mask |= CPU_PORT_LOGICAL_MASK;
>> +
>> +	return mask;
>> +}
>> +
>> +#define PORT_NUM_L2P(_p) (rtl8365mb_port_num_l2p(_p))
>> +#define PORT_NUM_L2E(_p) (CPU_PORT_EXTENSION_NUM)
>> +#define PORT_MASK_L2P(_m) (rtl8365mb_port_mask_l2p(_m))
>> +#define PORT_MASK_P2L(_m) (rtl8365mb_port_mask_p2l(_m))
> 
> The whole port mapping thing can be avoided if you just use port 6 as the CPU
> port.

Andrew also suggested this, but the discontinuity in port IDs seems to 
be an invitation for trouble. Here is an example of a series of 
functions from dsa.h:

static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
{
	struct dsa_switch_tree *dst = ds->dst;
	struct dsa_port *dp;

	list_for_each_entry(dp, &dst->ports, list)
		if (dp->ds == ds && dp->index == p)
			return dp;

	return NULL;
}

static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
{
	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
}

static inline u32 dsa_user_ports(struct dsa_switch *ds)
{
	u32 mask = 0;
	int p;

	for (p = 0; p < ds->num_ports; p++)
		if (dsa_is_user_port(ds, p))
			mask |= BIT(p);

	return mask;
}

My reading of dsa_user_ports() is that the port IDs run from 0 to 
(ds->num_ports - 1). If num_ports is 5 (4 user ports and 1 CPU port, as 
in my case), but the CPU is port 6, will we not dereference NULL when 
calling dsa_is_user_port(ds, 4)?

> 
>> +
>> +/* Chip-specific data and limits */