Re: [PATCH net] net: amd-xgbe: support receiving packets with bad FCS

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "If AI is right about other error frame types it may be
better to program them all in one commit to avoid a behavior change
if someone needs the other bad frames later"
---
net: amd-xgbe: support receiving packets with bad FCS

The driver unconditionally set MAC_RCR.DCRCC when enabling RX, so frames
with a bad FCS were always accepted without any user control.  This patch
advertises NETIF_F_RXALL, leaves it off by default, and programs DCRCC
from the feature state in xgbe_set_features() and xgbe_enable_rx().

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> index 2de974213090c..cbae3d7ef9836 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> @@ -3400,7 +3400,8 @@ static void xgbe_enable_rx(struct xgbe_prv_data *pdata)
>  	XGMAC_IOWRITE(pdata, MAC_RQC0R, reg_val);
>  
>  	/* Enable MAC Rx */
> -	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, DCRCC, 1);
> +	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, DCRCC,
> +			   !!(pdata->netdev->features & NETIF_F_RXALL));
>  	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, CST, 1);
>  	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, ACS, 1);
>  	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, RE, 1);

[Severity: Medium]
Can the DCRCC state programmed here disagree with the one programmed by
xgbe_set_features()?  Two different state sources are now used for the
same bit: xgbe_enable_rx() reads pdata->netdev->features, while
xgbe_set_features() uses its own features argument and records the result
in pdata->netdev_features.

The core only commits the new feature word after ndo_set_features()
returns:

net/core/dev.c:__netdev_update_features() {
	...
	if (dev->netdev_ops->ndo_set_features)
		err = dev->netdev_ops->ndo_set_features(dev, features);
	...
	dev->features = features;
	...
}

and xgbe_enable_rx()/xgbe_disable_rx() are also reached without RTNL from
the service work:

xgbe_service()
  phy_if.phy_status()
    xgbe_phy_link_status()
      xgbe_phy_start_data_path()
        hw_if->enable_rx()   /* xgbe_enable_rx() */

So if a link transition is processed while ethtool -K ethX rx-all on is
running, xgbe_enable_rx() reads the old netdev->features and writes DCRCC
back to the previous value.  Since the next request is compared against
pdata->netdev_features:

	if ((features & NETIF_F_RXALL) != rxall)

wouldn't the mismatch persist, with ethtool reporting one state and the
MAC doing the other?  Would keeping the requested DCRCC state in
pdata (updated under the same serialization as the register write) avoid
the two sources of truth?

Also, both paths perform unlocked read-modify-write cycles on MAC_RCR
through XGMAC_IOWRITE_BITS().  Can the write-back from xgbe_set_features()
clear the RE/CST/ACS bits that the service work just set in
xgbe_enable_rx(), leaving reception off until the next link event?

> @@ -3411,7 +3412,6 @@ static void xgbe_disable_rx(struct xgbe_prv_data *pdata)
>  	unsigned int i;
>  
>  	/* Disable MAC Rx */
> -	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, DCRCC, 0);
>  	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, CST, 0);
>  	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, ACS, 0);
>  	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, RE, 0);
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> index 2d6d00e3689ba..cea49a1153ac5 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> @@ -1980,13 +1980,14 @@ static int xgbe_set_features(struct net_device *netdev,
>  {
>  	struct xgbe_prv_data *pdata = netdev_priv(netdev);
>  	struct xgbe_hw_if *hw_if = &pdata->hw_if;
> -	netdev_features_t rxhash, rxcsum, rxvlan, rxvlan_filter;
> +	netdev_features_t rxhash, rxcsum, rxvlan, rxvlan_filter, rxall;
>  	int ret = 0;
>  
>  	rxhash = pdata->netdev_features & NETIF_F_RXHASH;

[ ... ]

> +	rxall = pdata->netdev_features & NETIF_F_RXALL;

[ ... ]

> @@ -2017,6 +2018,10 @@ static int xgbe_set_features(struct net_device *netdev,
>  	else if (!(features & NETIF_F_HW_VLAN_CTAG_FILTER) && rxvlan_filter)
>  		hw_if->disable_rx_vlan_filtering(pdata);
>  
> +	if ((features & NETIF_F_RXALL) != rxall)
> +		XGMAC_IOWRITE_BITS(pdata, MAC_RCR, DCRCC,
> +				   !!(features & NETIF_F_RXALL));
> +
>  	pdata->netdev_features = features;
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-main.c b/drivers/net/ethernet/amd/xgbe/xgbe-main.c
> index 7d45ea22a02e2..6e30ef84239f9 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-main.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-main.c
> @@ -241,7 +241,8 @@ int xgbe_config_netdev(struct xgbe_prv_data *pdata)
>  			      NETIF_F_GRO |
>  			      NETIF_F_HW_VLAN_CTAG_RX |
>  			      NETIF_F_HW_VLAN_CTAG_TX |
> -			      NETIF_F_HW_VLAN_CTAG_FILTER;
> +			      NETIF_F_HW_VLAN_CTAG_FILTER |
> +			      NETIF_F_RXALL;

[Severity: Medium]
Does advertising NETIF_F_RXALL here promise more than the DCRCC-only
implementation delivers?

Documentation/networking/netdev-features.rst describes rx-all as:

	This requests that the NIC receive all possible frames, including
	errored frames (such as bad FCS, etc).

With only MAC_RCR.DCRCC wired up, bad-FCS frames get through, but other
errored frames still look unreachable.  MTL_Q_RQOMR.FEP/FUP are never
programmed, and xgbe_init() still carries the original note:

drivers/net/ethernet/amd/xgbe/xgbe-dev.c:xgbe_init() {
	...
	/*TODO: Error Packet and undersized good Packet forwarding enable
		(FEP and FUP)
	 */
	...
}

On top of that, xgbe_dev_read() turns any descriptor error summary into
RX_PACKET_ERRORS.FRAME, and xgbe_rx_poll() then drops the frame with no
NETIF_F_RXALL exemption:

drivers/net/ethernet/amd/xgbe/xgbe-drv.c:xgbe_rx_poll() {
	...
		if (error || packet->errors) {
			dev_kfree_skb(skb);
			goto next_packet;
		}
	...
}

Over-MTU frames are dropped unconditionally in the same function.
igb and ixgbe gate exactly these drops on

	!(netdev->features & NETIF_F_RXALL)

Would it make sense to gate the error and length drops on NETIF_F_RXALL
too, or to state the limitation explicitly?  The commit message says:

	Advertise NETIF_F_RXALL and disable it by default.

which reads as full rx-all support, while runt, giant, alignment/dribble
and receive-error frames still appear to be discarded.
-- 
pw-bot: cr