Re: [PATCH net-next v2 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: dsa: lan9645x: add basic dsa driver for LAN9645X

This commit adds a basic DSA driver for the Microchip LAN9645X switch. It
includes initialization, port module setup for NPI, CPU, and front ports,
and phylink integration for MAC side configuration.

diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c
[ ... ]
> +static void disable_conduit_ipv6(struct lan9645x *lan9645x,
> +				 struct net_device *conduit)
> +{
> +	struct inet6_dev *dev_v6;
> +
> +	if (!conduit)
> +		return;
> +
> +	/* IPv6 on the conduit will generate frames bypassing our tag driver, so
> +	 * they lack an IFH. This will be garbage in garbage out and we want to
> +	 * avoid this.
> +	 */
> +	rtnl_lock();
> +	dev_v6 = __in6_dev_get(conduit);
> +	if (dev_v6) {
> +		WRITE_ONCE(dev_v6->cnf.disable_ipv6, 1);
> +		dev_warn(lan9645x->dev, "Disabled IPv6 on conduit device: %s\n",
> +			 netdev_name(conduit));
> +	}
> +	rtnl_lock();
> +}

Does directly modifying dev_v6->cnf.disable_ipv6 bypass the proper sysctl
teardown path (addrconf_disable_ipv6) and leave the IPv6 stack in an
inconsistent state on the conduit device by failing to clean up routes,
addresses, and MLD memberships?

Is it a layering violation for a switch driver to alter the networking
configuration of an external upper device, especially since this change
isn't reverted during driver unbind?

[ ... ]
> +void lan9645x_npi_port_init(struct lan9645x *lan9645x,
> +			    struct dsa_port *cpu_port)
> +{
> +	int port = cpu_port->index;
> +	struct lan9645x_port *p;
> +
> +	p = lan9645x_to_port(lan9645x, port);
> +	lan9645x->npi = port;

In a cascaded multi-switch DSA topology, cpu_port->index may belong to a
different switch and be greater than 8. If lan9645x_to_port returns NULL,
will this cause a panic when dereferencing p->chip_port below?

[ ... ]
> +	/* Configure IFH prefix mode for NPI port. */
> +	lan_rmw(SYS_PORT_MODE_INCL_XTR_HDR_SET(LAN9645X_TAG_PREFIX_LONG) |
> +		SYS_PORT_MODE_INCL_INJ_HDR_SET(LAN9645X_TAG_PREFIX_NONE),
> +		SYS_PORT_MODE_INCL_XTR_HDR |
> +		SYS_PORT_MODE_INCL_INJ_HDR,
> +		lan9645x,
> +		SYS_PORT_MODE(p->chip_port));

Setting LAN9645X_TAG_PREFIX_NONE means the hardware interprets the first
bytes of any frame received from the host as an IFH command structure.

Does this allow a local user to transmit crafted frames (for example, via
raw sockets or standard protocols) whose MAC address bytes map to malicious
IFH commands, thereby bypassing VLAN isolation or flooding the switch?

Should the hardware be configured to require a strict prefix, such as
LAN9645X_TAG_PREFIX_LONG, for injected frames to prevent this?

diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> +static int lan9645x_setup(struct dsa_switch *ds)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct device *dev = lan9645x->dev;
> +	u32 all_phys_ports;
> +	int err;
> +
> +	lan9645x->num_phys_ports = ds->num_ports;
> +	all_phys_ports = GENMASK(lan9645x->num_phys_ports - 1, 0);
> +
> +	err = lan9645x_reset_switch(lan9645x);
> +	if (err)
> +		return err;
> +
> +	lan9645x->ports = devm_kcalloc(lan9645x->dev, lan9645x->num_phys_ports,
> +				       sizeof(struct lan9645x_port *),
> +				       GFP_KERNEL);

Since the DSA .setup callback can be invoked multiple times during the
platform device's lifetime (for example, when the DSA master interface is
unbound and bound again), does using devm_kcalloc here cause a memory leak?
Should these allocations be moved to .probe or managed with kcalloc and
freed in .teardown?

[ ... ]
> +	/* Set all the entries to obey VLAN. */
> +	for (int i = 0; i < PGID_ENTRIES; ++i)
> +		lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
> +		       lan9645x, ANA_PGID_CFG(i));

PGID_ENTRIES is defined as 89, so this loop initializes indices 0 through
88. Since the CPU port is index 9, its source PGID is PGID_SRC + CPU_PORT
(80 + 9 = 89).

Is index 89 left uninitialized, breaking the OBEY_VLAN rule and allowing
CPU-injected frames to leak across VLAN boundaries?

[ ... ]
> +	/* Multicast to all front ports */
> +	lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_MC));
> +
> +	/* IP multicast to all front ports */
> +	lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_MCIPV4));
> +	lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_MCIPV6));
> +
> +	/* Unicast to all front ports */
> +	lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_UC));
> +
> +	/* Broadcast to all ports */
> +	lan_wr(BIT(CPU_PORT) | all_phys_ports, lan9645x, ANA_PGID(PGID_BC));

PGID_BC includes BIT(CPU_PORT) and all_phys_ports (which includes the NPI
port). Will this forward broadcast frames to both the CPU extraction queue
and the NPI port's normal egress queue, causing duplicate frames for the host?

Conversely, the multicast masks and PGID_UC exclude BIT(CPU_PORT). Does
this cause them to bypass the CPU extraction queue entirely, thereby
lacking the LONG extraction prefix and breaking the host's DSA tagger parsing?

diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
[ ... ]
> +static int lan9645x_phylink_mac_prepare(struct phylink_config *config,
> +					unsigned int mode,
> +					phy_interface_t iface)
> +{
> +	struct lan9645x_port *p = lan9645x_phylink_config_to_port(config);
> +	struct lan9645x *lan9645x = p->lan9645x;
> +	int port = p->chip_port;
> +	u32 mask;
> +
> +	if (port == 5 || port == 6 || port > 8)
> +		return -EINVAL;
> +
> +	mask = HSIO_HW_CFG_GMII_ENA_SET(BIT(port));
> +	lan_rmw(mask, mask, lan9645x, HSIO_HW_CFG);
> +
> +	if (port == 4 && phy_interface_mode_is_rgmii(iface))
> +		lan_rmw(HSIO_HW_CFG_RGMII_0_CFG_SET(1),
> +			HSIO_HW_CFG_RGMII_0_CFG,
> +			lan9645x, HSIO_HW_CFG);

The lan9645x_phylink_get_caps function advertises RGMII support for ports
4, 7, and 8. Does restricting the hardware multiplexer configuration here to
port 4 break connectivity for ports 7 and 8 when used in RGMII mode?

diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
[ ... ]
> +int lan9645x_port_set_maxlen(struct lan9645x *lan9645x, int port, size_t sdu)
> +{
> +	struct lan9645x_port *p = lan9645x_to_port(lan9645x, port);
> +	int maxlen = sdu + ETH_HLEN + ETH_FCS_LEN;

Does this maximum frame length calculation drop standard 1500-byte MTU
frames that are 802.1Q VLAN tagged (1522 bytes total), since it does not
account for VLAN_HLEN?

[ ... ]
> +int lan9645x_port_setup(struct dsa_switch *ds, int port)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct lan9645x_port *p;
> +
> +	p = lan9645x_to_port(lan9645x, port);
> +
> +	if (dp->dn) {
> +		p->rx_internal_delay =
> +			of_property_present(dp->dn, "rx-internal-delay-ps");
> +		p->tx_internal_delay =
> +			of_property_present(dp->dn, "tx-internal-delay-ps");
> +	}

These are standard integer properties specifying delays in picoseconds. If
a user explicitly disables the delay via devicetree using a value of 0,
will of_property_present evaluate to true and enable the hardware delay
anyway? Should of_property_read_u32 be used instead to check the value?