Reset ring role to disable in case MRP is destroyed #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Kernel callback `port_mrp_del_ring_role` is only called if ring_role is set to `BR_MRP_RING_ROLE_DISABLED`. Role is never said by userspace application so it might by the best place here. Signed-off-by: Andre Werner <[email protected]>
|
Hi, Thanks for the PR. I have one question about this one. I don't have a setup to actually test the DSA part of the kernel, so I have tried your change with a switchdev driver. But I have seen that with this change the ring role will be set twice:
Here is the link to the source code which should change the role to disable when the MRP instance is deleted: BTW: I have few patches for sparx5 regarding MRP that are not yet in the mainline kernel. Or are you using some configuration without switchdev support? Thanks, |
|
Dear Horatiu,
I start using it for an embedded device that is running Linux kernel
version 5.15 with a KSZ8567 switch attached. I adapted the driver ksz9477.c
and add the callbacks for MRP. The callback to set the ring role (`port_mrp_add_ring_role`)
adapt the static address lookup table for forwarding MRP related
Ethernet frames with the dedicated MAC addresses of MRP. When I detach the MRP server
from a used port I did not see the callback `port_mrp_del_ring_role` or
`port_mrp_del` that was used to reset the static mac address entries.
I took a look into the call chain and found this place in `net/bridge/br_mrp_switchdev.c`.
```
/* If the driver can't configure to run completely the protocol in HW,
* then try again to configure the HW so the SW can run the protocol.
*/
mrp_role.sw_backup = true;
if (role != BR_MRP_RING_ROLE_DISABLED)
err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
else
err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
```
So I used the place in the patch to trigger the dedicated callback.
I have already started porting my KSZ dedicated switch driver adaptions
to the mainline Linux kernel, but I have no results for tests yet.
I can share the patch for the ksz9477.c switch with you that is used in
Kernel version 5.15 if you interested. Maybe that helps for better
understanding. I saw that it changes a lot since Kernel version 5.15. So
it may be unnecessary now.
Regards,
André
…On Fri, 24 Jan 2025, HoratiuVultur wrote:
Hi,
Thanks for the PR. I have one question about this one. I don't have a setup to actually test the DSA part of the kernel, so I have tried your change with a switchdev driver. But I have seen that with this change the ring role will be set twice:
1. One time when you call `mrp_netlink_set_ring_role(mrp, BR_MRP_RING_ROLE_DISABLED);`
2. One time when the function `mrp_netlink_del(mrp)`. This is the stack trace for this call.
[ 18.722294] WARNING: CPU: 0 PID: 173 at drivers/net/ethernet/microchip/sparx5/sparx5_mrp.c:331 sparx5_handle_mrp_ring_role_del+0x88/0x100
[ 18.734619] Modules linked in:
[ 18.737660] CPU: 0 PID: 173 Comm: mrp_server Tainted: G W 6.6.51-00291-g438b23b478e1-dirty #1342
[ 18.747813] Hardware name: lan969x ev23x71a (pcb8398) (DT)
[ 18.753282] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 18.760225] pc : sparx5_handle_mrp_ring_role_del+0x88/0x100
[ 18.765780] lr : sparx5_handle_mrp_ring_role_del+0x88/0x100
[ 18.771335] sp : ffff80008184b2a0
[ 18.774634] x29: ffff80008184b2a0 x28: ffff000096033900 x27: 0000000000000000
[ 18.781751] x26: ffff80008184b458 x25: ffff80008184b907 x24: ffff800080d21488
[ 18.788869] x23: ffff80008184b458 x22: ffff800080b14748 x21: ffff000063b95000
[ 18.795986] x20: ffff000063876800 x19: ffff000063876000 x18: 0000000000000000
[ 18.803104] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[ 18.810222] x14: 0000b4466fdf828a x13: 02d120539d67838a x12: 00000000fa83b2da
[ 18.817339] x11: 00000000000001bb x10: 00000000000009d0 x9 : ffff80008184b050
[ 18.824457] x8 : ffff000096034330 x7 : 00000000000003f8 x6 : 000000000000b83e
[ 18.831574] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
[ 18.838692] x2 : 0000000000000000 x1 : ffff000096033900 x0 : 0000000000000026
[ 18.845810] Call trace:
[ 18.848241] sparx5_handle_mrp_ring_role_del+0x88/0x100
[ 18.853448] sparx5_switchdev_blocking_event+0x378/0x9bc
[ 18.858743] blocking_notifier_call_chain+0x6c/0xa0
[ 18.863604] switchdev_port_obj_notify+0x48/0xa8
[ 18.868204] switchdev_port_obj_del+0x3c/0xb8
[ 18.872544] br_mrp_switchdev_set_ring_role+0xcc/0x104 <----- this is one will set the port role to disable
[ 18.877665] br_mrp_del_impl+0x74/0x214
[ 18.881484] br_mrp_del+0x34/0x4c
[ 18.884783] br_mrp_instance_parse+0xd4/0x174
[ 18.889123] br_mrp_parse+0xb0/0x424
[ 18.892682] br_afspec+0x28c/0x300
[ 18.896067] br_dellink+0x78/0xf8
[ 18.899365] rtnl_bridge_dellink+0xdc/0x1fc
[ 18.903532] rtnetlink_rcv_msg+0x11c/0x354
[ 18.907611] netlink_rcv_skb+0x58/0x124
[ 18.911431] rtnetlink_rcv+0x18/0x24
[ 18.914990] netlink_unicast+0x1c0/0x29c
[ 18.918895] netlink_sendmsg+0x190/0x3f8
[ 18.922801] ____sys_sendmsg+0x21c/0x250
[ 18.926707] ___sys_sendmsg+0xac/0xf4
[ 18.930353] __sys_sendmsg+0x78/0xc4
[ 18.933912] __arm64_sys_sendmsg+0x24/0x30
[ 18.937991] invoke_syscall.constprop.0+0x50/0xec
[ 18.942679] do_el0_svc+0x40/0xc8
[ 18.945977] el0_svc+0x48/0x148
[ 18.949102] el0t_64_sync_handler+0x120/0x12c
[ 18.953442] el0t_64_sync+0x190/0x194
[ 18.957088] ---[ end trace 0000000000000000 ]---
Here is the link to the source code which should change the role to disable when the MRP instance is deleted:
https://elixir.bootlin.com/linux/v6.6.21/source/net/bridge/br_mrp.c#L442
BTW: I have few patches for sparx5 regarding MRP that are not yet in the mainline kernel.
Or are you using some configuration without switchdev support?
Thanks,
/Horatiu
--
Reply to this email directly or view it on GitHub:
#1 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
|
Hi André, Can you please share the userspace command that you run when "When I detach the MRP server from a used port" As mention before, I have not managed to test the MRP on DSA driver as I didn't have a setup for testing this (Also now I don't have a setup to try it). So it might that you have found an issue. Thanks, |
|
Dear /Horatiu,
please find attached the driver patch and in the following the usage of
the mrp server and client application.
Start server:
$ mrp_server -m -l 7 &
Add MRP on bridge:
mrp addmrp bridge br0 pport eth4 sport eth5 ring_nr 1 ring_role mrm
This will call port_mrp_add to register the ring_nr and ring_role for
the dedicated ports. Afterwards port_mrp_add_ring_role is called that
adds static forward lookup entries for the switch if the role is MRC.
For my understanding their is no need to forward packages to the CPU
connected to the switch because in this role packets only need to be
forwarded in this case.
[ 114.969315] ksz9477-switch spi2.0 eth5: Set MRP ring ID: 1
[ 114.969365] ksz9477-switch spi2.0 eth4: Set MRP ring ID: 1
[ 114.973427] ksz9477-switch spi2.0: Add port eth4 forwarding entry for MRP
[ 114.974979] ksz9477-switch spi2.0: Add port eth5 forwarding entry for MRP
Delete MRP on bridge:
mrp delmrp bridge br0 ring_nr 1
This will call port_del_mrp and port_mrp_del_ring_role and should delete
the entries in the static forward lookup table.
[ 176.015416] ksz9477-switch spi2.0: Delete port eth4 forwarding entry for MRP
[ 176.022530] ksz9477-switch spi2.0: Delete port eth5 forwarding entry for MRP
[ 176.022585] ksz9477-switch spi2.0 eth5: Delete MRP ring ID: 1
[ 176.022611] ksz9477-switch spi2.0 eth4: Delete MRP ring ID: 1
Before I add this line I never saw a call to the del[ete] functions in
my kernel log. Thus, my entries are never reverted in my understanding.
Regards,
André
On Mon, 27 Jan 2025, HoratiuVultur wrote:
Hi André,
Can you please share the userspace command that you run when "When I detach the MRP server from a used port"
It would be also nice if you share the patch with me.
As mention before, I have not managed to test the MRP on DSA driver as I didn't have a setup for testing this (Also now I don't have a setup to try it). So it might that you have found an issue.
Thanks,
/Horatiu
--
Reply to this email directly or view it on GitHub:
#1 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
From d293f207c62f52d53d72a8ef2acabbf12183fc91 Mon Sep 17 00:00:00 2001
From: Andre Werner ***@***.***>
Date: Mon, 20 Jan 2025 15:09:48 +0100
Subject: [PATCH] net: dsa: microchip: ksz9477: Add support for MRP
MRP needs client and server support from userspace.
Please refer to https://github.com/microchip-ung/mrp.
The driver implements callbacks to store the ring ID and role of a port
if using MRP. Additionally it adds rules to static lookup table to
forward MRP dedicated Ethernet frames only on ports that are used by the
MRP userspace server.
Signed-off-by: Andre Werner ***@***.***>
---
Usage:
Start server:
$ mrp_server -m -l 7 &
Add MRP on bridge:
mrp addmrp bridge br0 pport eth4 sport eth5 ring_nr 1 ring_role mrm
This will call port_mrp_add to register the ring_nr and ring_role for
the dedicated ports. Afterwards port_mrp_add_ring_role is called that
adds static forward lookup entries for the switch if the role is MRC.
For my understanding their is no need to forward packages to the CPU
connected to the switch because in this role packets only need to be
forwarded in this case.
[ 114.969315] ksz9477-switch spi2.0 eth5: Set MRP ring ID: 1
[ 114.969365] ksz9477-switch spi2.0 eth4: Set MRP ring ID: 1
[ 114.973427] ksz9477-switch spi2.0: Add port eth4 forwarding entry for MRP
[ 114.974979] ksz9477-switch spi2.0: Add port eth5 forwarding entry for MRP
Delete MRP on bridge
mrp delmrp bridge br0 ring_nr 1
This will call port_del_mrp and port_mrp_del_ring_role and should delete
the entries in the static forward lookup table.
[ 176.015416] ksz9477-switch spi2.0: Delete port eth4 forwarding entry for MRP
[ 176.022530] ksz9477-switch spi2.0: Delete port eth5 forwarding entry for MRP
[ 176.022585] ksz9477-switch spi2.0 eth5: Delete MRP ring ID: 1
[ 176.022611] ksz9477-switch spi2.0 eth4: Delete MRP ring ID: 1
Before I add this line I never saw a call to the del[ete] functions in
my kernel log. Thus, my entries are never reverted in my understanding.
---
---
drivers/net/dsa/microchip/ksz9477.c | 148 +++++++++++++++++++++++++
drivers/net/dsa/microchip/ksz_common.h | 3 +
2 files changed, 151 insertions(+)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index cee694169..3979e0707 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -7,6 +7,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/mrp_bridge.h>
#include <linux/iopoll.h>
#include <linux/platform_data/microchip-ksz.h>
#include <linux/phy.h>
@@ -1798,6 +1799,148 @@ int ksz9477_mac_link_state(struct dsa_switch *ds, int port,
return 0;
}
+static int ksz9477_port_mrp_add(struct dsa_switch *ds, int port,
+const struct switchdev_obj_mrp *mrp)
+{
+ struct ksz_device *dev = ds->priv;
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct net_device *ndev;
+ struct ksz_port *p;
+
+ if ((port < 0) || (port > dev->port_cnt) || (dsa_is_cpu_port(ds, port)))
+ return -EINVAL;
+
+ if (dp == NULL)
+ return -ENODEV;
+
+ ndev = dp->slave;
+ p = &dev->ports[port];
+
+ if (!((mrp->p_port == dp->slave) || (mrp->s_port == dp->slave)))
+ return 0;
+
+ if (p->mrp_ring_id)
+ return -EBUSY;
+
+ p->mrp_ring_id = mrp->ring_id;
+ p->mrp_role = BR_MRP_RING_ROLE_DISABLED;
+
+ netdev_info(dp->slave, "Set MRP ring ID: %d\n", mrp->ring_id);
+
+ return 0;
+}
+
+static const unsigned char mrp_test_dmac[] = { 0x01, 0x15, 0x4e, 0x00, 0x00, 0x01 };
+static const unsigned char mrp_control_dmac[] = { 0x01, 0x15, 0x4e, 0x00, 0x00, 0x02 };
+
+static int ksz9477_port_mrp_del(struct dsa_switch *ds, int port,
+const struct switchdev_obj_mrp *mrp)
+{
+ struct ksz_device *dev = ds->priv;
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct ksz_port *p;
+ int ret = 0;
+
+ if ((port < 0) || (port > dev->port_cnt) || (dsa_is_cpu_port(ds, port)))
+ return -EINVAL;
+
+ if (dp == NULL)
+ return -ENODEV;
+
+ p = &dev->ports[port];
+ if (p->mrp_ring_id != mrp->ring_id)
+ return 0;
+
+
+ if(p->mrp_role == BR_MRP_RING_ROLE_MRC) {
+ ret = ds->ops->port_fdb_del(ds, port, mrp_test_dmac, 0);
+ ret += ds->ops->port_fdb_del(ds, port, mrp_control_dmac, 0);
+ }
+
+ if(!ret) {
+ p->mrp_ring_id = 0;
+ p->mrp_role = BR_MRP_RING_ROLE_DISABLED;
+ netdev_info(dp->slave, "Delete MRP ring ID: %d\n", mrp->ring_id);
+ }
+
+ return ret;
+}
+
+static int ksz9477_port_mrp_add_ring_role(struct dsa_switch *ds, int port,
+const struct switchdev_obj_ring_role_mrp *mrp)
+{
+ struct ksz_device *dev = ds->priv;
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct ksz_port *p;
+ int ret = 0;
+
+ if ((port < 0) || (port > dev->port_cnt) || (dsa_is_cpu_port(ds, port)))
+ return -EINVAL;
+
+ if (dp == NULL)
+ return -ENODEV;
+
+ p = &dev->ports[port];
+
+ if (p->mrp_ring_id != mrp->ring_id)
+ return 0;
+
+ /* If there is no role change do nothing */
+ if (mrp->ring_role == p->mrp_role)
+ return 0;
+
+ /* Configure forwarding on partner port for control and test messages */
+ if (mrp->ring_role == BR_MRP_RING_ROLE_MRC) {
+ ret = ds->ops->port_fdb_add(ds, port, mrp_test_dmac, 0);
+ ret += ds->ops->port_fdb_add(ds, port, mrp_control_dmac, 0);
+ }
+ else {
+ ret = ds->ops->port_fdb_del(ds, port, mrp_test_dmac, 0);
+ ret += ds->ops->port_fdb_del(ds, port, mrp_control_dmac, 0);
+ }
+
+ if(!ret) {
+ p->mrp_role = mrp->ring_role;
+ dev_info(dev->dev, "Add port %s forwarding entry for MRP\n", netdev_name(dp->slave));
+ }
+
+ return ret;
+}
+static int ksz9477_port_mrp_del_ring_role(struct dsa_switch *ds, int port,
+const struct switchdev_obj_ring_role_mrp *mrp)
+{
+ struct ksz_device *dev = ds->priv;
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct ksz_port *p;
+ int ret = 0;
+
+ if ((port < 0) || (port > dev->port_cnt) || (dsa_is_cpu_port(ds, port)))
+ return -EINVAL;
+
+ if (dp == NULL)
+ return -ENODEV;
+
+ p = &dev->ports[port];
+ if (p->mrp_ring_id != mrp->ring_id)
+ return 0;
+
+ if (mrp->ring_role != BR_MRP_RING_ROLE_DISABLED)
+ return -EINVAL;
+
+ /* Configure forwarding on partner port for control and test messages */
+ ret = ds->ops->port_fdb_del(ds, port, mrp_test_dmac, 0);
+ ret += ds->ops->port_fdb_del(ds, port, mrp_control_dmac, 0);
+
+
+ if(!ret) {
+ p->mrp_role = mrp->ring_role;
+ dev_info(dev->dev, "Delete port %s forwarding entry for MRP\n", netdev_name(dp->slave));
+ }
+
+
+ return ret;
+}
+
static const struct dsa_switch_ops ksz9477_switch_ops = {
.get_tag_protocol = ksz9477_get_tag_protocol,
.setup = ksz9477_setup,
@@ -1824,6 +1967,11 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
.port_mdb_del = ksz9477_port_mdb_del,
.port_mirror_add = ksz9477_port_mirror_add,
.port_mirror_del = ksz9477_port_mirror_del,
+ /* MRP dedicated Operations */
+ .port_mrp_add = ksz9477_port_mrp_add,
+ .port_mrp_del = ksz9477_port_mrp_del,
+ .port_mrp_add_ring_role = ksz9477_port_mrp_add_ring_role,
+ .port_mrp_del_ring_role = ksz9477_port_mrp_del_ring_role,
};
static u32 ksz9477_get_port_addr(int port, int offset)
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 05e5a3b9e..860d077a9 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -9,6 +9,7 @@
#include <linux/etherdevice.h>
#include <linux/kernel.h>
+#include <linux/mrp_bridge.h>
#include <linux/mutex.h>
#include <linux/phy.h>
#include <linux/regmap.h>
@@ -31,6 +32,8 @@ struct ksz_port {
u8 link_val;
bool remove_tag; /* Remove Tag flag set, for ksz8795 only */
int stp_state;
+ enum br_mrp_ring_role_type mrp_role;
+ u32 mrp_ring_id;
struct phy_device phydev;
struct phy_device *actual_phydev;
…--
2.48.1
|
|
Hi André, Thanks for the commands and the patch. I don't fully understand why you don't get those callbacks if you don't explicitly set the role to BR_MRP_RING_ROLE_DISABLED, even if the delete will do that. I am looking here: Just few more comments about your patches:
Thanks, |
|
Thanks for your advises. I will implement your advises and give feedback
afterwards.
Regards.
André
…On Tue, 28 Jan 2025, HoratiuVultur wrote:
Hi André,
Thanks for the commands and the patch. I don't fully understand why you don't get those callbacks if you don't explicitly set the role to BR_MRP_RING_ROLE_DISABLED, even if the delete will do that. I am looking here:
https://elixir.bootlin.com/linux/v5.15.177/source/net/bridge/br_mrp.c#L442
When you delete the instance does the callback reach here?
Just few more comments about your patches:
1. I think your function 'ksz9477_port_mrp_add/del' looks good.
2. In the function 'ksz9477_port_mrp_add_ring_role' you will need to check also the flag 'mrp->sw_backup' this means that if the role is MRM and sw_backup is false, then it means that the HW will generate the test frames and it would figure out when it stop receiving the test frames. So my suggestion is to return -EOPNOTSUPP because you will be call back again with mrp->sw_backup set to false and in this case the SW will generate the test frame and figure out when it stop receiving the test frames.
3. Also you need to make sure that you trap to the CPU and allow the CPU to forward control frames
4. If you want to be MRC with offload support then you need to know which is the partner port on which you want to forward the frame, because it is possible to have multiple ports under the bridge and you don't want to forward these MRP test frames to all the other ports.
5. I suggest to have a look at this file: https://elixir.bootlin.com/linux/v5.15.177/source/drivers/net/ethernet/mscc/ocelot_mrp.c as I think this will match what you need to do for ksz switch.
Thanks,
/Horatiu
--
Reply to this email directly or view it on GitHub:
#1 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
This is clear to me and I added this as you suggested.
This is something I do not fully understand as the bridge runs on the Linux CPU and the applications as well. So all necessary packages arrive there automatically, I guess?
I thought this was done by adding static lookup table entries for the MRP frame MAC addresses to the switch. So if a package arrives on any of these configured ports, they are forwarded to the ports mapped via static lookup table entry. Or do you mean I may have several rings on different ports of one switch? So I need to link the same MAC address of topics to dedicated port pairs for each ring? This is out of the scope of our use case but then you are right, I guess.
Thanks. I try to understand your vcap feature. This is what is necessary for my answer above, right?
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Kernel callback
port_mrp_del_ring_roleis only called if ring_role is set toBR_MRP_RING_ROLE_DISABLED. Role is never said by userspace application so it might by the best place here.