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

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

[James]

From: James Nugraha <aslan.jnn@gmail.com>

The driver currently sets the MAC_RCR.DCRCC bit whenever RX is enabled.
This disables hardware FCS validation, causing packets with a bad FCS to
be accepted unconditionally. Users cannot control this behavior because
the driver does not advertise NETIF_F_RXALL.

Advertise NETIF_F_RXALL and disable it by default. Update DCRCC when the
RXALL feature is enabled or disabled, and preserve the selected state
across RX and link stop/start cycles.

Fixes: c5aa9e3b8156 ("amd-xgbe: Initial AMD 10GbE platform driver")
Signed-off-by: James Nugraha <aslan.jnn@gmail.com>
---
Tests:
- Verified invalid-FCS packets are dropped with RXALL disabled.
- Verified invalid-FCS packets are received with RXALL enabled.
- Verified invalid-FCS packets are dropped again after RXALL is disabled.
- Verified the RXALL setting survives RX stop/start.
- Verified the RXALL setting survives link down/up.
- Verified RXALL is disabled by default.

 drivers/net/ethernet/amd/xgbe/xgbe-dev.c  |  3 +--
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c  |  7 ++++++-
 drivers/net/ethernet/amd/xgbe/xgbe-main.c |  7 +++++--
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
index 1f350d3bd..ad2030517 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
@@ -3455,7 +3455,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);
@@ -3466,7 +3467,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 3a79fd054..5fb81bea1 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -2247,13 +2247,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;
 	rxcsum = pdata->netdev_features & NETIF_F_RXCSUM;
 	rxvlan = pdata->netdev_features & NETIF_F_HW_VLAN_CTAG_RX;
 	rxvlan_filter = pdata->netdev_features & NETIF_F_HW_VLAN_CTAG_FILTER;
+	rxall = pdata->netdev_features & NETIF_F_RXALL;
 
 	if ((features & NETIF_F_RXHASH) && !rxhash)
 		ret = hw_if->enable_rss(pdata);
@@ -2284,6 +2285,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;
 
 	DBGPR("<--xgbe_set_features\n");
 
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-main.c b/drivers/net/ethernet/amd/xgbe/xgbe-main.c
index 0e8698928..898146b67 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-main.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-main.c
@@ -351,7 +351,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;
 
 	if (pdata->hw_feat.rss)
 		netdev->hw_features |= NETIF_F_RXHASH;
@@ -382,2 +382,4 @@ int xgbe_config_netdev(struct xgbe_prv_data *pdata)
 	netdev->features |= netdev->hw_features;
+	/* disable RXALL by default */
+	netdev->features &= ~NETIF_F_RXALL;
 	pdata->netdev_features = netdev->features;

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

[Simon Horman]

On Wed, Aug 12, 2026 at 09:16:16AM +0000, James wrote:
> From: James Nugraha <aslan.jnn@gmail.com>
> 
> The driver currently sets the MAC_RCR.DCRCC bit whenever RX is enabled.
> This disables hardware FCS validation, causing packets with a bad FCS to
> be accepted unconditionally. Users cannot control this behavior because
> the driver does not advertise NETIF_F_RXALL.
> 
> Advertise NETIF_F_RXALL and disable it by default. Update DCRCC when the
> RXALL feature is enabled or disabled, and preserve the selected state
> across RX and link stop/start cycles.
> 
> Fixes: c5aa9e3b8156 ("amd-xgbe: Initial AMD 10GbE platform driver")

FWIW, I lean towards this being an enhancement for net-next without
a Fixes tag. Rather than a fix.

> Signed-off-by: James Nugraha <aslan.jnn@gmail.com>
> ---
> Tests:
> - Verified invalid-FCS packets are dropped with RXALL disabled.
> - Verified invalid-FCS packets are received with RXALL enabled.
> - Verified invalid-FCS packets are dropped again after RXALL is disabled.
> - Verified the RXALL setting survives RX stop/start.
> - Verified the RXALL setting survives link down/up.
> - Verified RXALL is disabled by default.

Thanks for noting the testing.
I might have even included that in the patch description.

The above not withstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>


...

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

[Jakub Kicinski]

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

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

[James]

On Fri, Aug 14, 2026 at 11:09:16AM +0100, Simon Horman <horms@kernel.org> wrote:
> FWIW, I lean towards this being an enhancement for net-next without
> a Fixes tag. Rather than a fix.

Hi Simon, thank you for the review! Apologise for the late reply as I was
unwell last week.

My rationale for targeting `net` was that the driver currently accepts bad
FCS frames by default, which I believe should be an incorrect existing behavior.
The `NETIF_F_RXALL` I added is intended to preserve access to that behavior
only when a user explicitly requests it.

With that said, I understand that exposing the `rx-all` control may make
this better suited for `net-next`. Do you prefer that I respin this as a
`net-next` patch (when it opens later this month), or do you think the
default-behavior correction justifies keeping it in `net`?

Regards,
James

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

[James]

On Mon, Aug 17, 2026 at 4:32:43PM -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> 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"
> ...

Hi Jakub, thanks a lot for your review!

Thank you for catching the possible race between the current feature update
path with the service-work RX re-enable path: `ndo_set_features()` programs
DCRCC before it commits `netdev_features`, while `xgbe_enable_rx()` reads
`netdev_features`. The non-atomic read-modify-write accesses to `MAC_RCR`
also make the final register value unreliable when those paths overlap.
Apologise that I've missed this.

Also taking Simon's earlier input into account, I think the best approach
for now is to set the default DCRCC value to 0, so that this network
controller can become more consistent with most other network controllers
and also with respect to user expectations. In particular, I would not
expect users to receive packets with bad FCS by default.

I will therefore drop the RXALL mechanism altogether for now (the updated,
trimmed down patch, will follow later). We can revisit this later and
implement proper RXALL support (under `net-next`, as Simon mentioned),
potentially including additional features beyond allowing packets with bad
FCS to come in.

Regards,
James