Re: [net-next RFC PATCH 00/13] net: hsr: Add PRP driver
From: Murali Karicheri
Date: Mon May 25 2020 - 12:49:52 EST
Hi Vinicius,
On 5/21/20 1:31 PM, Vinicius Costa Gomes wrote:
Murali Karicheri <m-karicheri2@xxxxxx> writes:
------------ Snip-------------
- prefix all common code with hsr_prp
- net/hsr -> renamed to net/hsr-prp
- All common struct types, constants, functions renamed with
hsr{HSR}_prp{PRP} prefix.
I don't really like these prefixes, I am thinking of when support for
IEEE 802.1CB is added, do we rename this to "hsr_prp_frer"?
And it gets even more complicated, and using 802.1CB you can configure
the tagging method and the stream identification function so a system
can interoperate in a HSR or PRP network.
So, I see this as different methods of achieving the same result, which
makes me think that the different "methods/types" (HSR and PRP in your
case) should be basically different implementations of a "struct
hsr_ops" interface. With this hsr_ops something like this:
struct hsr_ops {
int (*handle_frame)()
int (*add_port)()
int (*remove_port)()
int (*setup)()
void (*teardown)()
};
Thanks for your response!
I agree with you that the prefix renaming is ugly. However I wasn't
sure if it is okay to use a hsr prefixed code to handle PRP as
well as it may not be intuitive to anyone investigating the code. For
the same reason, handling 802.1CB specifc functions using the hsr_
prefixed code. If that is okay, then patch 1-6 are unnecessary. We could
also add some documentation at the top of the file to indicate that
both hsr and prp are implemented in the code or something like that.
BTW, I need to investigate more into 802.1CB and this was not known
when I developed this code few years ago.
Main difference between HSR and PRP is how they handle the protocol tag
or rct and create or handle the protocol specific part in the frame.
For that part, we should be able to define ops() like you have
suggested, instead of doing if check throughout the code. Hope that
is what you meant by hsr_ops() for this. Again shouldn't we use some
generic name like proto_ops or red_ops instead of hsr_ops() and assign
protocol specific implementaion to them? i.e hsr_ or prp_
or 802.1CB specific functions assigned to the function pointers. For
now I see handle_frame(), handle_sv_frame, create_frame(),
create_sv_frame() etc implemented differently (This is currently part of
patch 11 & 12). So something like
struct proto_ops {
int (*handle_frame)();
int (*create_frame)();
int (*handle_sv_frame)();
int (*create_sv_frame)();
};
and call dev->proto_ops->handle_frame() to process a frame from the
main hook. proto_ops gets initialized to of the set if implementation
at device or interface creation in hsr_dev_finalize().
Please review this and provide me feedback so that I can work to
incorporate them and send a formal patch series for this. As this
series impacts user space, I am not sure if this is the right
approach to introduce a new definitions and obsolete the old
API definitions for HSR. The current approach is choosen
to avoid redundant code in iproute2 and in the netlink driver
code (hsr_netlink.c). Other approach we discussed internally was
to Keep the HSR prefix in the user space and kernel code, but
live with the redundant code in the iproute2 and hsr netlink
code. Would like to hear from you what is the best way to add
this feature to networking core. If there is any other
alternative approach possible, I would like to hear about the
same.
Why redudant code is needed in the netlink parts and in iproute2 when
keeping the hsr prefix?
May be this is due to the specific implementation that I chose.
Currently I have separate netlink socket for HSR and PRP which may
be an overkill since bith are similar protocol.
Currently hsr inteface is created as
ip link add name hsr0 type hsr slave1 eth0 slave2 eth1 supervision 0
So I have implemented similar command for prp
ip link add name prp0 type prp slave1 eth0 slave2 eth1 supervision 0
In patch 7/13 I renamed existing HSR netlink socket attributes that
defines the hsr interface with the assumption that we can obsolete
the old definitions in favor of new common definitions with the
HSR_PRP prefix. Then I have separate code for creating prp
interface and related functions, even though they are similar.
So using common definitions, I re-use the code in netlink and
iproute2 (see patch 8 and 9 to re-use the code). PRP netlink
socket code in patch 10 which register prp_genl_family similar
to HSR.
+static struct genl_family prp_genl_family __ro_after_init = {
+ .hdrsize = 0,
+ .name = "PRP",
+ .version = 1,
+ .maxattr = HSR_PRP_A_MAX,
+ .policy = prp_genl_policy,
+ .module = THIS_MODULE,
+ .ops = prp_ops,
+ .n_ops = ARRAY_SIZE(prp_ops),
+ .mcgrps = prp_mcgrps,
+ .n_mcgrps = ARRAY_SIZE(prp_mcgrps),
+};
+
+int __init prp_netlink_init(void)
+{
+ int rc;
+
+ rc = rtnl_link_register(&prp_link_ops);
+ if (rc)
+ goto fail_rtnl_link_register;
+
+ rc = genl_register_family(&prp_genl_family);
+ if (rc)
+ goto fail_genl_register_family;
If we choose to re-use the existing HSR_ uapi defines, then should we
re-use the hsr netlink socket interface for PRP as well and
add additional attribute for differentiating the protocol specific
part?
i.e introduce protocol attribute to existing HSR uapi defines for
netlink socket to handle creation of prp interface.
enum {
HSR_A_UNSPEC,
HSR_A_NODE_ADDR,
HSR_A_IFINDEX,
HSR_A_IF1_AGE,
HSR_A_IF2_AGE,
HSR_A_NODE_ADDR_B,
HSR_A_IF1_SEQ,
HSR_A_IF2_SEQ,
HSR_A_IF1_IFINDEX,
HSR_A_IF2_IFINDEX,
HSR_A_ADDR_B_IFINDEX,
+ HSR_A_PROTOCOL <====if missing it is HSR (backward
compatibility)
defines HSR or PRP or 802.1CB in future.
__HSR_A_MAX,
};
So if ip link command is
ip link add name <if name> type <proto> slave1 eth0 slave2 eth1
supervision 0
Add HSR_A_PROTOCOL attribute with HSR/PRP specific value.
This way, the iprout2 code mostly remain the same as hsr, but will
change a bit to introduced this new attribute if user choose proto as
'prp' vs 'hsr'
BTW, I have posted the existing iproute2 code also to the mailing list
with title 'iproute2: Add PRP support'.
If re-using hsr code with existing prefix is fine for PRP or any future
protocol such as 801.1B, then I will drop patch 1-6 that are essentially
doing some renaming and re-use existing hsr netlink code for PRP with
added attribute to differentiate the protocol at the driver as described
above along with proto_ops and re-spin the series.
Let me know.
Regards,
Murali
The patch was tested using two TI AM57x IDK boards which are
connected back to back over two CPSW ports.
Script used for creating the hsr/prp interface is given below
and uses the ip link command. Also provided logs from the tests
I have executed for your reference.
iproute2 related patches will follow soon....
Murali Karicheri
Texas Instruments
============ setup.sh =================================================
#!/bin/sh
if [ $# -lt 4 ]
then
echo "setup-cpsw.sh <hsr/prp> <MAC-Address of slave-A>"
echo " <ip address for hsr/prp interface>"
echo " <if_name of hsr/prp interface>"
exit
fi
if [ "$1" != "hsr" ] && [ "$1" != "prp" ]
then
echo "use hsr or prp as first argument"
exit
fi
if_a=eth2
if_b=eth3
if_name=$4
ifconfig $if_a down
ifconfig $if_b down
ifconfig $if_a hw ether $2
ifconfig $if_b hw ether $2
ifconfig $if_a up
ifconfig $if_b up
echo "Setting up $if_name with MAC address $2 for slaves and IP address $3"
echo " using $if_a and $if_b"
if [ "$1" = "hsr" ]; then
options="version 1"
else
options=""
fi
ip link add name $if_name type $1 slave1 $if_a slave2 $if_b supervision 0 $options
ifconfig $if_name $3 up
==================================================================================
PRP Logs:
DUT-1 : https://pastebin.ubuntu.com/p/hhsRjTQpcr/
DUT-2 : https://pastebin.ubuntu.com/p/snPFKhnpk4/
HSR Logs:
DUT-1 : https://pastebin.ubuntu.com/p/FZPNc6Nwdm/
DUT-2 : https://pastebin.ubuntu.com/p/CtV4ZVS3Yd/
Murali Karicheri (13):
net: hsr: Re-use Kconfig option to support PRP
net: hsr: rename hsr directory to hsr-prp to introduce PRP
net: hsr: rename files to introduce PRP support
net: hsr: rename hsr variable inside struct hsr_port to priv
net: hsr: rename hsr_port_get_hsr() to hsr_prp_get_port()
net: hsr: some renaming to introduce PRP driver support
net: hsr: introduce common uapi include/definitions for HSR and PRP
net: hsr: migrate HSR netlink socket code to use new common API
net: hsr: move re-usable code for PRP to hsr_prp_netlink.c
net: hsr: add netlink socket interface for PRP
net: prp: add supervision frame generation and handling support
net: prp: add packet handling support
net: prp: enhance debugfs to display PRP specific info in node table
MAINTAINERS | 2 +-
include/uapi/linux/hsr_netlink.h | 3 +
include/uapi/linux/hsr_prp_netlink.h | 50 ++
include/uapi/linux/if_link.h | 19 +
net/Kconfig | 2 +-
net/Makefile | 2 +-
net/hsr-prp/Kconfig | 37 ++
net/hsr-prp/Makefile | 11 +
net/hsr-prp/hsr_netlink.c | 202 +++++++
net/{hsr => hsr-prp}/hsr_netlink.h | 15 +-
.../hsr_prp_debugfs.c} | 82 +--
net/hsr-prp/hsr_prp_device.c | 562 ++++++++++++++++++
net/hsr-prp/hsr_prp_device.h | 23 +
net/hsr-prp/hsr_prp_forward.c | 558 +++++++++++++++++
.../hsr_prp_forward.h} | 10 +-
.../hsr_prp_framereg.c} | 323 +++++-----
net/hsr-prp/hsr_prp_framereg.h | 68 +++
net/hsr-prp/hsr_prp_main.c | 194 ++++++
net/hsr-prp/hsr_prp_main.h | 289 +++++++++
net/hsr-prp/hsr_prp_netlink.c | 365 ++++++++++++
net/hsr-prp/hsr_prp_netlink.h | 28 +
net/hsr-prp/hsr_prp_slave.c | 222 +++++++
net/hsr-prp/hsr_prp_slave.h | 37 ++
net/hsr-prp/prp_netlink.c | 141 +++++
net/hsr-prp/prp_netlink.h | 27 +
net/hsr/Kconfig | 29 -
net/hsr/Makefile | 10 -
net/hsr/hsr_device.c | 509 ----------------
net/hsr/hsr_device.h | 22 -
net/hsr/hsr_forward.c | 379 ------------
net/hsr/hsr_framereg.h | 62 --
net/hsr/hsr_main.c | 154 -----
net/hsr/hsr_main.h | 188 ------
net/hsr/hsr_netlink.c | 514 ----------------
net/hsr/hsr_slave.c | 198 ------
net/hsr/hsr_slave.h | 33 -
36 files changed, 3084 insertions(+), 2286 deletions(-)
create mode 100644 include/uapi/linux/hsr_prp_netlink.h
create mode 100644 net/hsr-prp/Kconfig
create mode 100644 net/hsr-prp/Makefile
create mode 100644 net/hsr-prp/hsr_netlink.c
rename net/{hsr => hsr-prp}/hsr_netlink.h (58%)
rename net/{hsr/hsr_debugfs.c => hsr-prp/hsr_prp_debugfs.c} (52%)
create mode 100644 net/hsr-prp/hsr_prp_device.c
create mode 100644 net/hsr-prp/hsr_prp_device.h
create mode 100644 net/hsr-prp/hsr_prp_forward.c
rename net/{hsr/hsr_forward.h => hsr-prp/hsr_prp_forward.h} (50%)
rename net/{hsr/hsr_framereg.c => hsr-prp/hsr_prp_framereg.c} (56%)
create mode 100644 net/hsr-prp/hsr_prp_framereg.h
create mode 100644 net/hsr-prp/hsr_prp_main.c
create mode 100644 net/hsr-prp/hsr_prp_main.h
create mode 100644 net/hsr-prp/hsr_prp_netlink.c
create mode 100644 net/hsr-prp/hsr_prp_netlink.h
create mode 100644 net/hsr-prp/hsr_prp_slave.c
create mode 100644 net/hsr-prp/hsr_prp_slave.h
create mode 100644 net/hsr-prp/prp_netlink.c
create mode 100644 net/hsr-prp/prp_netlink.h
delete mode 100644 net/hsr/Kconfig
delete mode 100644 net/hsr/Makefile
delete mode 100644 net/hsr/hsr_device.c
delete mode 100644 net/hsr/hsr_device.h
delete mode 100644 net/hsr/hsr_forward.c
delete mode 100644 net/hsr/hsr_framereg.h
delete mode 100644 net/hsr/hsr_main.c
delete mode 100644 net/hsr/hsr_main.h
delete mode 100644 net/hsr/hsr_netlink.c
delete mode 100644 net/hsr/hsr_slave.c
delete mode 100644 net/hsr/hsr_slave.h
--
2.17.1
--
Murali Karicheri
Texas Instruments