Re: [PATCH net-next 09/10] net: renesas: rswitch: add simple l3 routing

[Andrew Lunn <andrew@lunn.ch>]

> +void rswitch_update_l3_offload(struct rswitch_private *priv)
> +{
> +	u32 all_ports_mask = GENMASK(RSWITCH_NUM_AGENTS - 1, 0);
> +	struct rswitch_device *rdev;
> +	bool l3_offload_enable_cond;
> +	u32 l3_rdev_count;
> +	u32 l3_ports_mask;
> +
> +	l3_ports_mask = all_ports_mask;
> +
> +	l3_rdev_count = 0;
> +	rswitch_for_all_ports(priv, rdev) {
> +		if (rdev_for_l3_offload(rdev)) {
> +			l3_rdev_count++;
> +			l3_ports_mask &= ~BIT(rdev->port);
> +		}
> +	}
> +
> +	l3_offload_enable_cond = (l3_rdev_count >= 2);
> +
> +#define FWPC0_L3_MASK (FWPC0_LTHTA | FWPC0_IP4UE | FWPC0_IP4TE | FWPC0_IP4OE)
> +	rswitch_for_all_ports(priv, rdev) {
> +		if (rdev_for_l3_offload(rdev) && l3_offload_enable_cond) {
> +			/* Update allowed offload destinations even for ports
> +			 * with l3 offload enabled earlier.
> +			 *
> +			 * Allow offload routing to self for hw port.
> +			 */
> +			rswitch_modify(priv->addr, FWPC1(rdev->port),
> +				       FWPC1_LTHFW_MASK,
> +				       FIELD_PREP(FWPC1_LTHFW_MASK, l3_ports_mask));
> +			if (!rdev->l3_offload_enabled) {
> +				rswitch_modify(priv->addr, FWPC0(rdev->port),
> +					       0,
> +					       FWPC0_L3_MASK);
> +				rdev->l3_offload_enabled = 1;
> +				netdev_info(rdev->ndev, "starting l3 offload\n");

This, and the other netdev_info calls should probably be debug.

> +static bool rswitch_l23update_hw_op(struct rswitch_private *priv,
> +				    struct rswitch_l23update *update,
> +				    bool install)
> +{
> +	u8 *dst_mac = update->spec.dst_mac;
> +	u32 val;
> +	int ret;
> +
> +	val = FIELD_PREP(FWL23URL0_RN, update->index) |
> +	      FIELD_PREP(FWL23URL0_PV,
> +			 install ? GENMASK(RSWITCH_NUM_AGENTS - 1, 0) : 0);
> +	iowrite32(val, priv->addr + FWL23URL0);
> +
> +	val = FWL23URL1_TTLU |
> +	      FWL23URL1_MSAU |
> +	      FWL23URL1_MDAU |
> +	      (dst_mac[0] << 8) | (dst_mac[1] << 0);
> +	iowrite32(val, priv->addr + FWL23URL1);
> +
> +	val = (dst_mac[2] << 24) | (dst_mac[3] << 16) |
> +	      (dst_mac[4] << 8)  | (dst_mac[5] << 0);
> +	iowrite32(val, priv->addr + FWL23URL2);
> +
> +	iowrite32(0, priv->addr + FWL23URL3);
> +
> +	/* Rule write starts after writing to FWL23URL3 */
> +
> +	ret = rswitch_reg_wait(priv->addr, FWL23URLR, FWL23URLR_L, 0);
> +	if (ret) {
> +		dev_err(&priv->pdev->dev, "timeout writing l23_update\n");
> +		return false;

Why not make this an int function and return -ETIMEDOUT?

> +static bool rmon_ipv4_dst_offload_hw_op(struct rswitch_route_monitor *rmon,
> +					struct rmon_ipv4_dst_offload *offload,
> +					u8 frame_type, bool install)

Why all this bool functions? Especially when you have calls returning
error codes you are throwing away.

> +static struct rswitch_l23update *rswitch_get_l23update(struct rswitch_private *priv,
> +						       struct rswitch_l23update_spec *spec)
> +{
> +	struct rswitch_l23update *update;
> +
> +	spin_lock(&priv->l3_lock);
> +
> +	list_for_each_entry(update, &priv->l23_update_list, list) {
> +		if (rswitch_l23update_matches_spec(update, spec)) {
> +			update->use_count++;
> +			goto out;
> +		}
> +	}
> +
> +	update = kzalloc(sizeof(*update), GFP_ATOMIC);
> +	if (!update)
> +		goto out;
> +
> +	update->use_count = 1;
> +	update->spec = *spec;
> +	update->index = find_first_zero_bit(priv->l23_update_bitmap,
> +					    RSWITCH_MAX_NUM_RRULE);
> +	if (update->index == RSWITCH_MAX_NUM_RRULE) {
> +		dev_err_ratelimited(&priv->pdev->dev,
> +				    "out of l23_update entries\n");
> +		/* FIXME: trigger expire? */
> +		goto no_free_bit;
> +	}
> +	set_bit(update->index, priv->l23_update_bitmap);
> +
> +	if (!rswitch_l23update_hw_op(priv, update, true))
> +		goto hw_op_failed;
> +
> +	list_add(&update->list, &priv->l23_update_list);
> +out:
> +	spin_unlock(&priv->l3_lock);
> +
> +	return update;
> +
> +hw_op_failed:
> +	clear_bit(update->index, priv->l23_update_bitmap);
> +no_free_bit:
> +	kfree(update);
> +	update = NULL;
> +	goto out;

It is pretty unusual to have a backwards goto, especially in error
handling. This is one case where a scoped_guard() might make sense.

	Andrew