Re: [net-next RFC PATCH 00/13] net: hsr: Add PRP driver
From: Murali Karicheri
Date: Tue May 26 2020 - 17:51:48 EST
Hi Vinicius,
On 5/26/20 2:56 PM, Vinicius Costa Gomes wrote:
Murali Karicheri <m-karicheri2@xxxxxx> writes:
Hi Vinicius,
On 5/21/20 1:31 PM, Vinicius Costa Gomes wrote:
Murali Karicheri <m-karicheri2@xxxxxx> writes:
------------ Snip-------------
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.
I think for now it's better to make it clear how similar PRP and HSR
are.
As for the renaming, I am afraid that this boat has sailed, as the
netlink API already uses HSR_ and it's better to reuse that than create
a new family for, at least conceptually, the same thing (PRP and
802.1CB). And this is important bit, the userspace API.
And even for 802.1CB using name "High-availability Seamless Redudancy"
is as good as any, if very pompous.
I have reviewed the 802.1CB at a high level. The idea of 802.1CB is
also high availability and redundancy similar to HSR and PRP but at
stream level. So now I feel more comfortable to re-use the hsr prefix
until we find a better name. I can document this in all file headers to
make this explicit when I spin the formal patch for this. I will wait
for a couple of weeks before start the work on a formal patch
series so that others have a chance to respond as well.
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)();
};
That's it. That was the idea I was trying to communicate :-)
Ok
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.
Deprecating an userspace API is hard and takes a long time. So let's
avoid that if it makes sense.
Ok, make sense.
+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?
Yes, that seems the way to go.
Ok.
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'
Sounds good, I think.
Ok. If we want to add 802.1CB later, specific value used can be
extended to use 802.1CB.
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.
If I forget that HSR is also the name of a protocol, what the acronym
means makes sense for 802.1CB, so it's not too bad, I think.
Agree.
Let me know.
Regards,
Murali
Cheers,
--
Murali Karicheri
Texas Instruments