Messages in this thread | | | Subject | Re: [net-next RFC PATCH 00/13] net: hsr: Add PRP driver | From | Murali Karicheri <> | Date | Tue, 26 May 2020 17:51:40 -0400 |
| |
Hi Vinicius,
On 5/26/20 2:56 PM, Vinicius Costa Gomes wrote: > Murali Karicheri <m-karicheri2@ti.com> writes: > >> Hi Vinicius, >> >> On 5/21/20 1:31 PM, Vinicius Costa Gomes wrote: >>> Murali Karicheri <m-karicheri2@ti.com> 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
| |