Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

[Florian Fainelli <f.fainelli@gmail.com>]

On 1/13/21 6:59 AM, George McCollister wrote:
> Add a driver with initial support for the Arrow SpeedChips XRS7000
> series of gigabit Ethernet switch chips which are typically used in
> critical networking applications.
> 
> The switches have up to three RGMII ports and one RMII port.
> Management to the switches can be performed over i2c or mdio.
> 
> Support for advanced features such as PTP and
> HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
> may be added at a later date.
> 
> Signed-off-by: George McCollister <george.mccollister@gmail.com>
> ---
[snip]

This looks ready to me, just a few nits and suggestions.


> +/* Add an inbound policy filter which matches the BPDU destination MAC
> + * and forwards to the CPU port. Leave the policy disabled, it will be
> + * enabled as needed.
> + */
> +static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port)
> +{
> +	struct xrs700x *priv = ds->priv;
> +	unsigned int val = 0;
> +	int i = 0;
> +	int ret;
> +
> +	/* Compare all 48 bits of the destination MAC address. */
> +	ret = regmap_write(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 48 << 2);
> +	if (ret)
> +		return ret;
> +
> +	/* match BPDU destination 01:80:c2:00:00:00 */
> +	ret = regmap_write(priv->regmap, XRS_ETH_ADDR_0(port, 0), 0x8001);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, XRS_ETH_ADDR_1(port, 0), 0xc2);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, XRS_ETH_ADDR_2(port, 0), 0x0);
> +	if (ret)
> +		return ret;

Not that this is likely to change, but you could write this as a for
loop and use eth_stp_addr from include/linux/etherdevice.h.

[snip]

> +static int xrs700x_port_setup(struct dsa_switch *ds, int port)
> +{
> +	bool cpu_port = dsa_is_cpu_port(ds, port);
> +	struct xrs700x *priv = ds->priv;
> +	unsigned int val = 0;
> +	int ret, i;
> +
> +	xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED);

It looks like we should be standardizing at some point on having switch
drivers do just the global configuration in the ->setup() callback and
have the core call the ->port_disable() for each port except the CPU/DSA
ports, and finally let the actual port configuration bet done in
->port_enable(). What you have is fine for now and easy to change in the
future.

> +int xrs700x_switch_register(struct xrs700x *dev)
> +{
> +	int ret;
> +	int i;
> +
> +	ret = xrs700x_detect(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = xrs700x_setup_regmap_range(dev);
> +	if (ret)
> +		return ret;
> +
> +	dev->ports = devm_kzalloc(dev->dev,
> +				  sizeof(*dev->ports) * dev->ds->num_ports,
> +				  GFP_KERNEL);
> +	if (!dev->ports)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < dev->ds->num_ports; i++) {
> +		ret = xrs700x_alloc_port_mib(dev, i);
> +		if (ret)
> +			return ret;

Nothing frees up the successfully allocated p->mib_data[] in case of
errors so you would be leaking here.

[snip]

> +
> +	/* Main DSA driver may not be started yet. */
> +	if (ret)
> +		return ret;
> +
> +	i2c_set_clientdata(i2c, dev);

I would be tempted to move this before "publishing" the device, probably
does not harm though, likewise for the MDIO stub.

[snip]

> +/* Switch Configuration Registers - VLAN */
> +#define XRS_VLAN(v)			(XRS_SWITCH_VLAN_BASE + 0x2 * (v))
> +
> +#define MAX_VLAN			4095

Can you use VLAN_N_VID - 1 here from include/linux/if_vlan.h?
-- 
Florian