Re: [PATCH v2 3/4] net: renesas: rswitch: add offloading for L2 switching

[Andrew Lunn]

> +void rswitch_update_l2_offload(struct rswitch_private *priv)
> +{
> +	bool learning_needed, forwarding_needed;
> +	unsigned int all_ports_mask, fwd_mask;
> +	struct rswitch_device *rdev;
> +
> +	/* calculate fwd_mask with zeroes in bits corresponding to ports that
> +	 * shall participate in hardware forwarding
> +	 */
> +	all_ports_mask = GENMASK(RSWITCH_NUM_AGENTS - 1, 0);
> +	fwd_mask = all_ports_mask;
> +
> +	rswitch_for_all_ports(priv, rdev) {
> +		if (rdev_for_l2_offload(rdev) && rdev->forwarding_requested)
> +			fwd_mask &= ~BIT(rdev->port);
> +	}
> +
> +	rswitch_for_all_ports(priv, rdev) {
> +		if (rdev_for_l2_offload(rdev)) {
> +			learning_needed = rdev->learning_requested;
> +			forwarding_needed = rdev->forwarding_requested;
> +		} else {
> +			learning_needed = false;
> +			forwarding_needed = false;
> +		}
> +
> +		if (!rdev->learning_offloaded && learning_needed) {
> +			rswitch_modify(priv->addr, FWPC0(rdev->port),
> +				       0,
> +				       FWPC0_MACSSA | FWPC0_MACHLA | FWPC0_MACHMA);
> +
> +			rdev->learning_offloaded = true;
> +			netdev_info(rdev->ndev, "starting hw learning\n");
> +		}
> +
> +		if (rdev->learning_offloaded && !learning_needed) {
> +			rswitch_modify(priv->addr, FWPC0(rdev->port),
> +				       FWPC0_MACSSA | FWPC0_MACHLA | FWPC0_MACHMA,
> +				       0);
> +
> +			rdev->learning_offloaded = false;
> +			netdev_info(rdev->ndev, "stopping hw learning\n");
> +		}
> +
> +		if (forwarding_needed) {
> +			/* Update allowed offload destinations even for ports
> +			 * with L2 offload enabled earlier.
> +			 *
> +			 * Do not allow L2 forwarding to self for hw port.
> +			 */
> +			iowrite32(FIELD_PREP(FWCP2_LTWFW_MASK, fwd_mask | BIT(rdev->port)),
> +				  priv->addr + FWPC2(rdev->port));
> +
> +			if (!rdev->forwarding_offloaded) {
> +				rswitch_modify(priv->addr, FWPC0(rdev->port),
> +					       0,
> +					       FWPC0_MACDSA);
> +
> +				rdev->forwarding_offloaded = true;
> +				netdev_info(rdev->ndev,
> +					    "starting hw forwarding\n");
> +			}
> +		} else if (rdev->forwarding_offloaded) {
> +			iowrite32(FIELD_PREP(FWCP2_LTWFW_MASK, fwd_mask | BIT(rdev->port)),
> +				  priv->addr + FWPC2(rdev->port));
> +
> +			rswitch_modify(priv->addr, FWPC0(rdev->port),
> +				       FWPC0_MACDSA,
> +				       0);
> +
> +			rdev->forwarding_offloaded = false;
> +			netdev_info(rdev->ndev, "stopping hw forwarding\n");
> +		}
> +	}
> +}

This function seems overly complicated.

Normally you can change a ports STP state without needing to consider
other ports. Can you separate STP from joining ports to a bridge? That
might help simplify this function, break it up into two functions.

> +static int rswitch_port_obj_add(struct net_device *ndev, const void *ctx,
> +				const struct switchdev_obj *obj,
> +				struct netlink_ext_ack *extack)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int rswitch_port_obj_del(struct net_device *ndev, const void *ctx,
> +				const struct switchdev_obj *obj)
> +{
> +	return -EOPNOTSUPP;
> +}

> +static int rswitch_switchdev_blocking_event(struct notifier_block *nb,
> +					    unsigned long event, void *ptr)
> +{
> +	struct net_device *ndev = switchdev_notifier_info_to_dev(ptr);
> +	int ret;
> +
> +	switch (event) {
> +	case SWITCHDEV_PORT_OBJ_ADD:
> +		ret = switchdev_handle_port_obj_add(ndev, ptr,
> +						    rswitch_port_check,
> +						    rswitch_port_obj_add);
> +		break;
> +	case SWITCHDEV_PORT_OBJ_DEL:
> +		ret = switchdev_handle_port_obj_del(ndev, ptr,
> +						    rswitch_port_check,
> +						    rswitch_port_obj_del);
> +		break;

Since these two are hard coded to return EOPNOTSUPP, it seems like you
could just return that here?

>  /* Forwarding engine block (MFWD) */
> -static void rswitch_fwd_init(struct rswitch_private *priv)
> +static int rswitch_fwd_init(struct rswitch_private *priv)
>  {
> +	/* Initialize MAC hash table */
> +	iowrite32(FWMACTIM_MACTIOG, priv->addr + FWMACTIM);
> +	ret = rswitch_reg_wait(priv->addr, FWMACTIM, FWMACTIM_MACTIOG, 0);
> +
> +	return ret;

Just

	return rswitch_reg_wait(priv->addr, FWMACTIM, FWMACTIM_MACTIOG, 0);

	Andrew