Currently the DSA framework will HW offload any bridge port if there is
a driver available to support HW offloading. This may not always be the
preferred case. In cases where it is preferred that all traffic still
hit the CPU, do software bridging instead.

To prevent HW bridging (and potential CPU bypass), make the DSA
framework aware of the devlink port function attr, bridge_offload, and
add a matching field to the port struct. Add get/set functions to
configure the field, and use this field to condition HW config for
offloading a bridge port.

Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
---
 include/net/dsa.h |  1 +
 net/dsa/devlink.c | 27 ++++++++++++++++++++++++++-
 net/dsa/dsa.c     |  1 +
 net/dsa/port.c    |  3 ++-
 4 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index a0a9481c52c2..9ee2d7ccfff8 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -291,6 +291,7 @@ struct dsa_port {
 
 	struct device_node	*dn;
 	unsigned int		ageing_time;
+	bool bridge_offloading;
 
 	struct dsa_bridge	*bridge;
 	struct devlink_port	devlink_port;
diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
index f41f9fc2194e..dc98e5311921 100644
--- a/net/dsa/devlink.c
+++ b/net/dsa/devlink.c
@@ -298,6 +298,31 @@ void dsa_devlink_region_destroy(struct devlink_region *region)
 }
 EXPORT_SYMBOL_GPL(dsa_devlink_region_destroy);
 
+static void
+dsa_devlink_port_bridge_offload_get(struct devlink_port *dlp,
+				    bool *is_enable)
+{
+	struct dsa_port *dp = dsa_to_port(dsa_devlink_port_to_ds(dlp),
+					  dsa_devlink_port_to_port(dlp));
+
+	*is_enable = dp->bridge_offloading;
+}
+
+static void
+dsa_devlink_port_bridge_offload_set(struct devlink_port *dlp,
+				    bool enable)
+{
+	struct dsa_port *dp = dsa_to_port(dsa_devlink_port_to_ds(dlp),
+					  dsa_devlink_port_to_port(dlp));
+
+	dp->bridge_offloading = enable;
+}
+
+static const struct devlink_port_ops dsa_devlink_port_ops = {
+	.port_fn_bridge_offload_get = dsa_devlink_port_bridge_offload_get,
+	.port_fn_bridge_offload_set = dsa_devlink_port_bridge_offload_set,
+};
+
 int dsa_port_devlink_setup(struct dsa_port *dp)
 {
 	struct devlink_port *dlp = &dp->devlink_port;
@@ -341,7 +366,7 @@ int dsa_port_devlink_setup(struct dsa_port *dp)
 	}
 
 	devlink_port_attrs_set(dlp, &attrs);
-	err = devlink_port_register(dl, dlp, dp->index);
+	err = devlink_port_register_with_ops(dl, dlp, dp->index, &dsa_devlink_port_ops);
 	if (err) {
 		if (ds->ops->port_teardown)
 			ds->ops->port_teardown(ds, dp->index);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5a7c0e565a89..20cf55bd42db 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -529,6 +529,7 @@ static int dsa_port_setup(struct dsa_port *dp)
 		return err;
 	}
 
+	dp->bridge_offloading = true;
 	dp->setup = true;
 
 	return 0;
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 5c9d1798e830..de176f7993d0 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -493,7 +493,8 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
 	struct net_device *brport_dev;
 	int err;
 
-	if (br_mst_enabled(br) && !dsa_port_supports_mst(dp))
+	if ((br_mst_enabled(br) && !dsa_port_supports_mst(dp)) ||
+	    !dp->bridge_offloading)
 		return -EOPNOTSUPP;
 
 	/* Here the interface is already bridged. Reflect the current
-- 
2.47.1

On Mon, Jan 20, 2025 at 01:49:12PM +1300, Aryan Srivastava wrote:
> Currently the DSA framework will HW offload any bridge port if there is
> a driver available to support HW offloading. This may not always be the
> preferred case. In cases where it is preferred that all traffic still
> hit the CPU, do software bridging instead.
> 
> To prevent HW bridging (and potential CPU bypass), make the DSA
> framework aware of the devlink port function attr, bridge_offload, and
> add a matching field to the port struct. Add get/set functions to
> configure the field, and use this field to condition HW config for
> offloading a bridge port.

This is not a very convincing description. What is your real use case
for not offloading?

> 
> Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
> ---
>  include/net/dsa.h |  1 +
>  net/dsa/devlink.c | 27 ++++++++++++++++++++++++++-
>  net/dsa/dsa.c     |  1 +
>  net/dsa/port.c    |  3 ++-
>  4 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index a0a9481c52c2..9ee2d7ccfff8 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -291,6 +291,7 @@ struct dsa_port {
>  
>  	struct device_node	*dn;
>  	unsigned int		ageing_time;
> +	bool bridge_offloading;

Indentation is not consistent here.

net-next is closed for the merge window. 

    Andrew

---
pw-bot: cr

On Mon, 2025-01-20 at 03:39 +0100, Andrew Lunn wrote:
> On Mon, Jan 20, 2025 at 01:49:12PM +1300, Aryan Srivastava wrote:
> > Currently the DSA framework will HW offload any bridge port if
> > there is
> > a driver available to support HW offloading. This may not always be
> > the
> > preferred case. In cases where it is preferred that all traffic
> > still
> > hit the CPU, do software bridging instead.
> > 
> > To prevent HW bridging (and potential CPU bypass), make the DSA
> > framework aware of the devlink port function attr, bridge_offload,
> > and
> > add a matching field to the port struct. Add get/set functions to
> > configure the field, and use this field to condition HW config for
> > offloading a bridge port.
> 
> This is not a very convincing description. What is your real use case
> for not offloading?
> 
The real use case for us is packet inspection. Due to the bridge ports
being offloaded in hardware, we can no longer inspect the traffic on
them, as the packets never hit the CPU.
> > 
> > Signed-off-by: Aryan Srivastava
> > <aryan.srivastava@alliedtelesis.co.nz>
> > ---
> >  include/net/dsa.h |  1 +
> >  net/dsa/devlink.c | 27 ++++++++++++++++++++++++++-
> >  net/dsa/dsa.c     |  1 +
> >  net/dsa/port.c    |  3 ++-
> >  4 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index a0a9481c52c2..9ee2d7ccfff8 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -291,6 +291,7 @@ struct dsa_port {
> >  
> >         struct device_node      *dn;
> >         unsigned int            ageing_time;
> > +       bool bridge_offloading;
> 
> Indentation is not consistent here.
Will fix.
> 
> net-next is closed for the merge window.
I was unsure about uploading this right now (as you said net-next is
closed), but the netdev docs page states that RFC patches are welcome
anytime, please let me know if this is not case, and if so I apologize
for my erroneous submission.
>  
> 
>     Andrew
> 
> ---
> pw-bot: cr

	Aryan

> > This is not a very convincing description. What is your real use case
> > for not offloading?
> > 
> The real use case for us is packet inspection. Due to the bridge ports
> being offloaded in hardware, we can no longer inspect the traffic on
> them, as the packets never hit the CPU.

So are you using libpcap to bring them into user space? Or are you
using eBPF?

What i'm thinking about is, should this actually be a devlink option,
or should it happen on its own because you have attached something
which cannot be offloaded to the hardware, so hardware offload should
be disabled by the kernel?

> > net-next is closed for the merge window.

> I was unsure about uploading this right now (as you said net-next is
> closed), but the netdev docs page states that RFC patches are welcome
> anytime, please let me know if this is not case, and if so I apologize
> for my erroneous submission.

It would be good to say what you are requesting comments about.

   Andrew

On Mon, 2025-01-20 at 04:12 +0100, Andrew Lunn wrote:
> > > This is not a very convincing description. What is your real use
> > > case
> > > for not offloading?
> > > 
> > The real use case for us is packet inspection. Due to the bridge
> > ports
> > being offloaded in hardware, we can no longer inspect the traffic
> > on
> > them, as the packets never hit the CPU.
> 
> So are you using libpcap to bring them into user space? Or are you
> using eBPF?
> 
> What i'm thinking about is, should this actually be a devlink option,
> or should it happen on its own because you have attached something
> which cannot be offloaded to the hardware, so hardware offload should
> be disabled by the kernel?
Yes, so we use various in-kernel (eBPF) and some out of tree methods
(netmap). To service both, I thought that a devlink config would be
best.

Also aside from specific kernel feature use-cases the actual port we
are trying to service is meant to be a dedicated NIC, whose
implementation detail is that it is on a switch chip. By offloading
aspects of its config, we end up with inconsistent behaviour across NIC
ports on our devices. As the port is not meant to behave like a
switchport, the fact it happens to be on the end of a switch should not
force it to HW offload bridging. The HW bridging results in CPU bypass,
where as SW bridging will be a more desirable method of bridging for
this port. Therefore it is desirable to have a method to configure
certain ports for SW bridging only, rather than the driver
automatically choosing to do HW offload if capable.
> 
> > > net-next is closed for the merge window.
> 
> > I was unsure about uploading this right now (as you said net-next
> > is
> > closed), but the netdev docs page states that RFC patches are
> > welcome
> > anytime, please let me know if this is not case, and if so I
> > apologize
> > for my erroneous submission.
> 
> It would be good to say what you are requesting comments about.
> 
Ah yes, I understand. I will add the request in the cover letter next
time, but essentially I just wanted comments about the best way to go
about this. Specifically if there was already a method in kernel to
prevent HW offload of certain features or if there are alternative
methods to devlink to add specific switch config options on a per port
basis.

	Aryan

On Mon, Jan 20, 2025 at 09:36:57PM +0000, Aryan Srivastava wrote:
> On Mon, 2025-01-20 at 04:12 +0100, Andrew Lunn wrote:
> > > > This is not a very convincing description. What is your real use
> > > > case
> > > > for not offloading?
> > > > 
> > > The real use case for us is packet inspection. Due to the bridge
> > > ports
> > > being offloaded in hardware, we can no longer inspect the traffic
> > > on
> > > them, as the packets never hit the CPU.
> > 
> > So are you using libpcap to bring them into user space? Or are you
> > using eBPF?
> > 
> > What i'm thinking about is, should this actually be a devlink option,
> > or should it happen on its own because you have attached something
> > which cannot be offloaded to the hardware, so hardware offload should
> > be disabled by the kernel?
> Yes, so we use various in-kernel (eBPF) and some out of tree methods
> (netmap). To service both, I thought that a devlink config would be
> best.
> 
> Also aside from specific kernel feature use-cases the actual port we
> are trying to service is meant to be a dedicated NIC, whose
> implementation detail is that it is on a switch chip. By offloading
> aspects of its config, we end up with inconsistent behaviour across NIC
> ports on our devices. As the port is not meant to behave like a
> switchport, the fact it happens to be on the end of a switch should not
> force it to HW offload bridging. The HW bridging results in CPU bypass,
> where as SW bridging will be a more desirable method of bridging for
> this port. Therefore it is desirable to have a method to configure
> certain ports for SW bridging only, rather than the driver
> automatically choosing to do HW offload if capable.
> > 
> > > > net-next is closed for the merge window.
> > 
> > > I was unsure about uploading this right now (as you said net-next
> > > is
> > > closed), but the netdev docs page states that RFC patches are
> > > welcome
> > > anytime, please let me know if this is not case, and if so I
> > > apologize
> > > for my erroneous submission.
> > 
> > It would be good to say what you are requesting comments about.
> > 
> Ah yes, I understand. I will add the request in the cover letter next
> time, but essentially I just wanted comments about the best way to go
> about this. Specifically if there was already a method in kernel to
> prevent HW offload of certain features or if there are alternative
> methods to devlink to add specific switch config options on a per port
> basis.

For debug purpouse I am turning off HW bridge offload by setting aging to 0.
Don't know if it is appropriate in your case.

Thanks
Michal

> 
> 	Aryan
> 
> 
>