回复：回复：[PATCH v1 1/1] net/nbl: add VLAN offload set interface

[Dimon]

Hi Stephen,
Thank you for your feedback. 
We understand your technical concern regarding the VLAN offload implementation.
Our implementation choice is driven by practical customer requirements.
In real-world deployment scenarios, there is a strong expectation that basic VLAN offload capabilities are available and functional by default.
Numerous customer applications, automation scripts, and network configurations rely on these standard VLAN operations "just working,"
irrespective of whether the underlying mechanism is hardware-accelerated or software-based.
Our current approach ensures that:
Compatibility is maintained: Existing deployment tools and scripts function without any required modifications.
Customer expectations are met: The fundamental VLAN functionality is present and operational.
The alternative—returning -ENOTSUP—would indeed break a significant number of existing customer deployments that
operate under the reasonable assumption that basic VLAN features are available.
Thank you.
------------------------------------------------------------------
发件人：Stephen Hemminger <stephen at networkplumber.org>
发送时间：2025年11月25日(周二) 23:28
收件人：Dimon<dimon.zhao at nebula-matrix.com>
抄　送：dev<dev at dpdk.org>; Kyo Liu<kyo.liu at nebula-matrix.com>; Leon<leon.yu at nebula-matrix.com>; Sam<sam.chen at nebula-matrix.com>
主　题：Re: 回复：[PATCH v1 1/1] net/nbl: add VLAN offload set interface
On Tue, 25 Nov 2025 10:28:38 +0800
"Dimon" <dimon.zhao at nebula-matrix.com> wrote:
> Hi Stephen,
> Thank you for your review.
> http://patches.dpdk.org/project/dpdk/patch/20251107073459.3532524-3-dimon.zhao@nebula-matrix.com/ <http://patches.dpdk.org/project/dpdk/patch/20251107073459.3532524-3-dimon.zhao@nebula-matrix.com/ > <http://patches.dpdk.org/project/dpdk/patch/20251107073459.3532524-3-dimon.zhao@nebula-matrix.com/ <http://patches.dpdk.org/project/dpdk/patch/20251107073459.3532524-3-dimon.zhao@nebula-matrix.com/ > >
> In this previous commit, the driver is advertising RTE_ETH_RX_OFFLOAD_VLAN_STRIP
> in the offload flags. Because we do not support QINQ and FILTER,
> we are not advertising QINQ or FILTER flags.
> dev_info->tx_offload_capa |= RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
> dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_VLAN_STRIP;
> In this previous commit, we simulate support for Tx and Rx VLAN offload,
> while in reality we handle Tx VLAN insertion and Rx VLAN stripping in software.
> For Rx VLAN stripping, the driver uses rte_vlan_strip to achieve this functionality.
> It does not have the capability to do this in hardware.
> So when enabling or disabling Rx VLAN stripping, the driver and hardware do not need to do anything.
> Why vlan_offload_set is Required?
> The rte_eth_dev_set_vlan_offload function has this flow:
> int rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask)
> {
> // ...
> if (dev->dev_ops->vlan_offload_set == NULL)
> return -ENOTSUP; // This causes the error!
> // ...
> ret = dev->dev_ops->vlan_offload_set(dev, mask);
> }
> Without implementing vlan_offload_set, users get -ENOTSUP error when running:
> testpmd> vlan set strip on 0 
> Since hardware doesn't need configuration changes, our implementation is minimal:
> int nbl_vlan_offload_set(__rte_unused struct rte_eth_dev *dev, 
> __rte_unused int mask)
> {
> // No hardware configuration needed - everything handled in software
> return 0;
> }
> ------------------------------------------------------------------
> 发件人：Stephen Hemminger <stephen at networkplumber.org>
> 发送时间：2025年11月25日(周二) 07:35
> 收件人：Dimon<dimon.zhao at nebula-matrix.com>
> 抄 送：dev<dev at dpdk.org>; Kyo Liu<kyo.liu at nebula-matrix.com>; Leon<leon.yu at nebula-matrix.com>; Sam<sam.chen at nebula-matrix.com>
> 主 题：Re: [PATCH v1 1/1] net/nbl: add VLAN offload set interface
> On Sun, 23 Nov 2025 19:40:26 -0800
> Dimon Zhao <dimon.zhao at nebula-matrix.com> wrote:
> > The rte_eth_dev_set_vlan_offload function internally calls
> > the vlan_offload_set interface, so we must implement this function.
> > Otherwise, an error will occur when
> > executing the vlan set strip on command.
> > 
> > Fixes: 9d7757dce874 ("net/nbl: simulate VLAN offload")
> > 
> > Signed-off-by: Dimon Zhao <dimon.zhao at nebula-matrix.com>
> > ---
> > drivers/net/nbl/nbl_dev/nbl_dev.c | 5 +++++
> > drivers/net/nbl/nbl_dev/nbl_dev.h | 1 +
> > drivers/net/nbl/nbl_ethdev.c | 1 +
> > 3 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/net/nbl/nbl_dev/nbl_dev.c b/drivers/net/nbl/nbl_dev/nbl_dev.c
> > index 58eb1c6231..923de2e9d0 100644
> > --- a/drivers/net/nbl/nbl_dev/nbl_dev.c
> > +++ b/drivers/net/nbl/nbl_dev/nbl_dev.c
> > @@ -758,6 +758,11 @@ int nbl_promiscuous_disable(struct rte_eth_dev *eth_dev)
> > return 0;
> > }
> > 
> > +int nbl_vlan_offload_set(__rte_unused struct rte_eth_dev *dev, __rte_unused int mask)
> > +{
> > + return 0;
> > +} 
> This seems broken in handling VLAN.
> The intention is that the driver changes how vlans handled based on the mask
> in the API call. 
> The driver is not advertising RTE_ETH_VLAN_STRIP_OFFLOAD in the offload flags.
> Same for QINQ or FILTER flags.
> What is the intention? How is the hardware handling VLAN tags? Does it
> have the capability to do this in hardware? Can it be enabled and disabled?
If device does not support VLAN offload then it is correct for any
attempts to change the mask to return -ENOTSUP. This is what happens
now with all of these kind of devices.
You could make an argument that at the ethdev shim layer, requests
to disable any feature that a device could not support could be reported
as allowed. But that is such a odd case, that it is more logical to
argue that: "Any change to offload X should be reported as non supported
unless the device reports it can do feature X".
If you have some customer or internal script expecting testpmd
to always allow vlan strip 0 then that is the real issue.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/dev/attachments/20251126/1b74a315/attachment-0001.htm>