Re: [PATCH net-next v4 16/16] net: airoha: Introduce PPE debugfs support

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

On Feb 17, Simon Horman wrote:
> On Thu, Feb 13, 2025 at 04:34:35PM +0100, Lorenzo Bianconi wrote:
> > Similar to PPE support for Mediatek devices, introduce PPE debugfs
> > in order to dump binded and unbinded flows.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c b/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c
> 
> ...
> 
> > +static int airoha_ppe_debugfs_foe_show(struct seq_file *m, void *private,
> > +				       bool bind)
> > +{
> > +	static const char *const ppe_type_str[] = {
> > +		[PPE_PKT_TYPE_IPV4_HNAPT] = "IPv4 5T",
> > +		[PPE_PKT_TYPE_IPV4_ROUTE] = "IPv4 3T",
> > +		[PPE_PKT_TYPE_BRIDGE] = "L2B",
> > +		[PPE_PKT_TYPE_IPV4_DSLITE] = "DS-LITE",
> > +		[PPE_PKT_TYPE_IPV6_ROUTE_3T] = "IPv6 3T",
> > +		[PPE_PKT_TYPE_IPV6_ROUTE_5T] = "IPv6 5T",
> > +		[PPE_PKT_TYPE_IPV6_6RD] = "6RD",
> > +	};
> > +	static const char *const ppe_state_str[] = {
> > +		[AIROHA_FOE_STATE_INVALID] = "INV",
> > +		[AIROHA_FOE_STATE_UNBIND] = "UNB",
> > +		[AIROHA_FOE_STATE_BIND] = "BND",
> > +		[AIROHA_FOE_STATE_FIN] = "FIN",
> > +	};
> > +	struct airoha_ppe *ppe = m->private;
> > +	int i;
> > +
> > +	for (i = 0; i < PPE_NUM_ENTRIES; i++) {
> > +		const char *state_str, *type_str = "UNKNOWN";
> > +		u16 *src_port = NULL, *dest_port = NULL;
> > +		struct airoha_foe_mac_info_common *l2;
> > +		unsigned char h_source[ETH_ALEN] = {};
> > +		unsigned char h_dest[ETH_ALEN];
> > +		struct airoha_foe_entry *hwe;
> > +		u32 type, state, ib2, data;
> > +		void *src_addr, *dest_addr;
> > +		bool ipv6 = false;
> > +
> > +		hwe = airoha_ppe_foe_get_entry(ppe, i);
> > +		if (!hwe)
> > +			continue;
> > +
> > +		state = FIELD_GET(AIROHA_FOE_IB1_STATE, hwe->ib1);
> > +		if (!state)
> > +			continue;
> > +
> > +		if (bind && state != AIROHA_FOE_STATE_BIND)
> > +			continue;
> > +
> > +		state_str = ppe_state_str[state % ARRAY_SIZE(ppe_state_str)];
> > +		type = FIELD_GET(AIROHA_FOE_IB1_PACKET_TYPE, hwe->ib1);
> > +		if (type < ARRAY_SIZE(ppe_type_str) && ppe_type_str[type])
> > +			type_str = ppe_type_str[type];
> > +
> > +		seq_printf(m, "%05x %s %7s", i, state_str, type_str);
> > +
> > +		switch (type) {
> > +		case PPE_PKT_TYPE_IPV4_HNAPT:
> > +		case PPE_PKT_TYPE_IPV4_DSLITE:
> > +			src_port = &hwe->ipv4.orig_tuple.src_port;
> > +			dest_port = &hwe->ipv4.orig_tuple.dest_port;
> > +			fallthrough;
> > +		case PPE_PKT_TYPE_IPV4_ROUTE:
> > +			src_addr = &hwe->ipv4.orig_tuple.src_ip;
> > +			dest_addr = &hwe->ipv4.orig_tuple.dest_ip;
> > +			break;
> > +		case PPE_PKT_TYPE_IPV6_ROUTE_5T:
> > +			src_port = &hwe->ipv6.src_port;
> > +			dest_port = &hwe->ipv6.dest_port;
> > +			fallthrough;
> > +		case PPE_PKT_TYPE_IPV6_ROUTE_3T:
> > +		case PPE_PKT_TYPE_IPV6_6RD:
> > +			src_addr = &hwe->ipv6.src_ip;
> > +			dest_addr = &hwe->ipv6.dest_ip;
> > +			ipv6 = true;
> > +			break;
> > +		}
> 
> Hi Lorenzo,

Hi Simon,

> 
> Perhaps it can't happen, but if type is not one of the cases handled
> by the switch statement above then src_addr and dest_addr will
> be used while uninitialised by the call to airoha_debugfs_ppe_print_tuple()
> below.

ack, right. I will fix it in v6.

Regards,
Lorenzo

> 
> Flagged by Smatch.
> 
> > +
> > +		seq_puts(m, " orig=");
> > +		airoha_debugfs_ppe_print_tuple(m, src_addr, dest_addr,
> > +					       src_port, dest_port, ipv6);
> > +
> > +		switch (type) {
> > +		case PPE_PKT_TYPE_IPV4_HNAPT:
> > +		case PPE_PKT_TYPE_IPV4_DSLITE:
> > +			src_port = &hwe->ipv4.new_tuple.src_port;
> > +			dest_port = &hwe->ipv4.new_tuple.dest_port;
> > +			fallthrough;
> > +		case PPE_PKT_TYPE_IPV4_ROUTE:
> > +			src_addr = &hwe->ipv4.new_tuple.src_ip;
> > +			dest_addr = &hwe->ipv4.new_tuple.dest_ip;
> > +			seq_puts(m, " new=");
> > +			airoha_debugfs_ppe_print_tuple(m, src_addr, dest_addr,
> > +						       src_port, dest_port,
> > +						       ipv6);
> > +			break;
> > +		}
> > +
> > +		if (type >= PPE_PKT_TYPE_IPV6_ROUTE_3T) {
> > +			data = hwe->ipv6.data;
> > +			ib2 = hwe->ipv6.ib2;
> > +			l2 = &hwe->ipv6.l2;
> > +		} else {
> > +			data = hwe->ipv4.data;
> > +			ib2 = hwe->ipv4.ib2;
> > +			l2 = &hwe->ipv4.l2.common;
> > +			*((__be16 *)&h_source[4]) =
> > +				cpu_to_be16(hwe->ipv4.l2.src_mac_lo);
> > +		}
> > +
> > +		*((__be32 *)h_dest) = cpu_to_be32(l2->dest_mac_hi);
> > +		*((__be16 *)&h_dest[4]) = cpu_to_be16(l2->dest_mac_lo);
> > +		*((__be32 *)h_source) = cpu_to_be32(l2->src_mac_hi);
> > +
> > +		seq_printf(m, " eth=%pM->%pM etype=%04x data=%08x"
> > +			      " vlan=%d,%d ib1=%08x ib2=%08x\n",
> > +			   h_source, h_dest, l2->etype, data,
> > +			   l2->vlan1, l2->vlan2, hwe->ib1, ib2);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]