Re: [net-next v5 2/3] net: ethernet: adi: Add ADIN1110 support

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 19 Aug 2022 17:19:40 +0300 andrei.tachici@stud.acs.upb.ro wrote:
> +static int adin1110_ndo_set_mac_address(struct net_device *netdev, void *addr)
> +{
> +	struct sockaddr *sa = addr;
> +
> +	if (netif_running(netdev))
> +		return -EBUSY;

Please use eth_prepare_mac_addr_change() instead.

> +	return adin1110_set_mac_address(netdev, sa->sa_data);
> +}

> +static int adin1110_net_stop(struct net_device *net_dev)
> +{
> +	struct adin1110_port_priv *port_priv = netdev_priv(net_dev);
> +
> +	netif_stop_queue(port_priv->netdev);
> +	flush_work(&port_priv->tx_work);

What prevents the IRQ from firing right after this point and waking 
the queue again?

> +	phy_stop(port_priv->phydev);
> +
> +	return 0;
> +}
> +
> +static void adin1110_tx_work(struct work_struct *work)
> +{
> +	struct adin1110_port_priv *port_priv = container_of(work, struct adin1110_port_priv, tx_work);
> +	struct adin1110_priv *priv = port_priv->priv;
> +	struct sk_buff *txb;
> +	bool last;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	last = skb_queue_empty(&port_priv->txq);
> +
> +	while (!last) {
> +		txb = skb_dequeue(&port_priv->txq);
> +		last = skb_queue_empty(&port_priv->txq);

while ((txb = skb_dequeue(&port_priv->txq)))

> +		if (txb) {
> +			ret = adin1110_write_fifo(port_priv, txb);
> +			if (ret < 0)
> +				netdev_err(port_priv->netdev, "Frame write error: %d\n", ret);

This needs rate limiting.

> +			dev_kfree_skb(txb);
> +		}
> +	}
> +
> +	mutex_unlock(&priv->lock);
> +}
> +
> +static netdev_tx_t adin1110_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct adin1110_port_priv *port_priv = netdev_priv(dev);
> +	struct adin1110_priv *priv = port_priv->priv;
> +	netdev_tx_t netdev_ret = NETDEV_TX_OK;
> +	u32 tx_space_needed;
> +
> +	spin_lock(&priv->state_lock);
> +
> +	tx_space_needed = skb->len + ADIN1110_FRAME_HEADER_LEN + ADIN1110_INTERNAL_SIZE_HEADER_LEN;
> +	if (tx_space_needed > priv->tx_space) {
> +		netif_stop_queue(dev);
> +		netdev_ret = NETDEV_TX_BUSY;
> +	} else {
> +		priv->tx_space -= tx_space_needed;
> +		skb_queue_tail(&port_priv->txq, skb);
> +	}
> +
> +	spin_unlock(&priv->state_lock);

What is this lock protecting? There's already a lock around Tx.

> +	schedule_work(&port_priv->tx_work);
> +
> +	return netdev_ret;
> +}
> +

> +static int adin1110_net_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct adin1110_port_priv *port_priv = netdev_priv(dev);
> +	struct nlattr *br_spec;
> +	struct nlattr *attr;
> +	int rem;
> +
> +	br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
> +	if (!br_spec)
> +		return -EINVAL;
> +
> +	nla_for_each_nested(attr, br_spec, rem) {
> +		u16 mode;
> +
> +		if (nla_type(attr) != IFLA_BRIDGE_MODE)
> +			continue;
> +
> +		if (nla_len(attr) < sizeof(mode))
> +			return -EINVAL;
> +
> +		port_priv->priv->br_mode = nla_get_u16(attr);
> +		adin1110_set_rx_mode(dev);
> +		break;
> +	}
> +
> +	return 0;
> +}

I thought this is a callback for legacy SR-IOV NICs. What are you using
it for in a HW device over SPI? :S

> +static int adin1110_port_set_forwarding_state(struct adin1110_port_priv *port_priv)
> +{
> +	struct adin1110_priv *priv = port_priv->priv;
> +	int ret;
> +
> +	port_priv->state = BR_STATE_FORWARDING;
> +
> +	if (adin1110_can_offload_forwarding(priv)) {
> +		ret = adin1110_hw_forwarding(priv, true);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	mutex_lock(&priv->lock);
> +	ret = adin1110_set_mac_address(port_priv->netdev, port_priv->netdev->dev_addr);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = adin1110_setup_rx_mode(port_priv);
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}

The bridge support looks quite incomplete here, no?
There's no access to the FDB of the switch.
You forward to the host based on the MAC addr of the port not 
the bridge.