| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Sep 2020 09:36:59 +0000
From: "henrik.bjoernlund@...rochip.com" <henrik.bjoernlund@...rochip.com>
To: Nikolay Aleksandrov <nikolay@...dia.com>
CC: "bridge@...ts.linux-foundation.org"
<bridge@...ts.linux-foundation.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jiri@...lanox.com" <jiri@...lanox.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Roopa Prabhu <roopa@...dia.com>,
"idosch@...lanox.com" <idosch@...lanox.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
"horatiu.vultur@...rochip.com" <horatiu.vultur@...rochip.com>
Subject: Re: [PATCH RFC 4/7] bridge: cfm: Kernel space implementation of CFM.
Thanks for the review. Comments below.
The 09/08/2020 13:16, Nikolay Aleksandrov wrote:
>
> On Fri, 2020-09-04 at 09:15 +0000, Henrik Bjoernlund wrote:
> > This is the implementation of the CFM protocol according to
> > 802.1Q section 12.14.
> >
> > Connectivity Fault Management (CFM) comprises capabilities for
> > detecting, verifying, and isolating connectivity failures in
> > Virtual Bridged Networks. These capabilities can be used in
> > networks operated by multiple independent organizations, each
> > with restricted management access to each other’s equipment.
> >
> > CFM functions are partitioned as follows:
> > - Path discovery
> > - Fault detection
> > - Fault verification and isolation
> > - Fault notification
> > - Fault recovery
> >
> > Interface consists of these functions:
> > br_cfm_mep_create()
> > br_cfm_mep_delete()
> > br_cfm_mep_config_set()
> > br_cfm_mep_status_get()
> > br_cfm_mep_counters_get()
> > br_cfm_mep_counters_clear()
> > br_cfm_cc_config_set()
> > br_cfm_cc_peer_mep_add()
> > br_cfm_cc_peer_mep_remove()
> > br_cfm_cc_rdi_set()
> > br_cfm_cc_ccm_tx()
> > br_cfm_cc_status_get()
> > br_cfm_cc_counters_get()
> > br_cfm_cc_counters_clear()
> > br_cfm_cc_peer_status_get()
> >
> > A MEP instance is created by br_cfm_mep_create()
> > -It is the Maintenance association End Point
> > described in 802.1Q section 19.2.
> > -It is created on a specific level (1-7) and is assuring
> > that no CFM frames are passing through this MEP on lower levels.
> > -It initiates and validates CFM frames on its level.
> > -It can only exist on a port that is related to a bridge.
> > -Attributes given cannot be changed until the instance is
> > deleted.
> >
> > A MEP instance can be deleted by br_cfm_mep_delete().
> >
> > A created MEP instance has attributes that can be
> > configured by br_cfm_mep_config_set().
> >
> > A MEP contain status and counter information that can be
> > retrieved by br_cfm_mep_status_get() and
> > br_cfm_mep_counters_get().
> >
> > A MEP counters can be cleared by br_cfm_mep_counters_clear().
> >
> > A MEP Continuity Check feature can be configured by
> > br_cfm_cc_config_set()
> > The Continuity Check Receiver state machine can be
> > enabled and disabled.
> > According to 802.1Q section 19.2.8
> >
> > A MEP can have Peer MEPs added and removed by
> > br_cfm_cc_peer_mep_add() and br_cfm_cc_peer_mep_remove()
> > The Continuity Check feature can maintain connectivity
> > status on each added Peer MEP.
> >
> > A MEP can be configured to start or stop transmission of CCM frames by
> > br_cfm_cc_ccm_tx()
> > The CCM will be transmitted for a selected period in seconds.
> > Must call this function before timeout to keep transmission alive.
> >
> > A MEP transmitting CCM can be configured with inserted RDI in PDU by
> > br_cfm_cc_rdi_set()
> >
> > A MEP contain Continuity Check status and counter information
> > that can be retrieved by br_cfm_cc_status_get() and
> > br_cfm_cc_counters_get().
> >
> > A MEP Continuity Check counters can be cleared
> > by br_cfm_cc_counters_clear().
> >
> > A MEP contain Peer MEP Continuity Check status information that
> > can be retrieved by br_cfm_cc_peer_status_get().
> >
> > Signed-off-by: Henrik Bjoernlund <henrik.bjoernlund@...rochip.com>
> > ---
> > include/uapi/linux/cfm_bridge.h | 75 +++
> > net/bridge/Makefile | 2 +
> > net/bridge/br_cfm.c | 880 ++++++++++++++++++++++++++++++++
> > net/bridge/br_private.h | 16 +
> > net/bridge/br_private_cfm.h | 242 +++++++++
> > 5 files changed, 1215 insertions(+)
> > create mode 100644 include/uapi/linux/cfm_bridge.h
> > create mode 100644 net/bridge/br_cfm.c
> > create mode 100644 net/bridge/br_private_cfm.h
> >
>
> This is a large single patch, do you think it can be broken down into pieces?
> I'll review it like this now, but it would be much easier if it's in smaller
> logical pieces.
> In general are you sure there are no holes in the structs being assigned
> directly? Since you use memcmp() and such, you could end up surprised. :)
>
I will try and break this patch up into logical pieces.
I will assure that when called from br_cfm_netlink.c the struct is
memset to zero before members are set.
> > diff --git a/include/uapi/linux/cfm_bridge.h b/include/uapi/linux/cfm_bridge.h
> > new file mode 100644
> > index 000000000000..389ea1e1f68e
> > --- /dev/null
> > +++ b/include/uapi/linux/cfm_bridge.h
> > @@ -0,0 +1,75 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +
> > +#ifndef _UAPI_LINUX_CFM_BRIDGE_H_
> > +#define _UAPI_LINUX_CFM_BRIDGE_H_
> > +
> > +#include <linux/types.h>
> > +#include <linux/if_ether.h>
> > +
> > +#define ETHER_HEADER_LENGTH (6+6+4+2)
> > +#define CFM_MAID_LENGTH 48
> > +#define CFM_CCM_PDU_LENGTH 75
> > +#define CFM_PORT_STATUS_TLV_LENGTH 4
> > +#define CFM_IF_STATUS_TLV_LENGTH 4
> > +#define CFM_IF_STATUS_TLV_TYPE 4
> > +#define CFM_PORT_STATUS_TLV_TYPE 2
> > +#define CFM_ENDE_TLV_TYPE 0
> > +#define CFM_CCM_MAX_FRAME_LENGTH (ETHER_HEADER_LENGTH+\
> > + CFM_CCM_PDU_LENGTH+\
> > + CFM_PORT_STATUS_TLV_LENGTH+\
> > + CFM_IF_STATUS_TLV_LENGTH)
> > +#define CFM_FRAME_PRIO 7
> > +#define CFM_CCM_OPCODE 1
> > +#define CFM_CCM_TLV_OFFSET 70
> > +#define CFM_CCM_PDU_MAID_OFFSET 10
> > +#define CFM_CCM_PDU_MEPID_OFFSET 8
> > +#define CFM_CCM_PDU_SEQNR_OFFSET 4
> > +#define CFM_CCM_PDU_TLV_OFFSET 74
> > +#define CFM_CCM_ITU_RESERVED_SIZE 16
> > +
> > +struct br_cfm_common_hdr {
> > + __u8 mdlevel_version;
> > + __u8 opcode;
> > + __u8 flags;
> > + __u8 tlv_offset;
> > +};
> > +
> > +struct br_cfm_status_tlv {
> > + __u8 type;
> > + __be16 length;
> > + __u8 value;
> > +};
> > +
> > +enum br_cfm_opcodes {
> > + BR_CFM_OPCODE_CCM = 0x1,
> > + BR_CFM_OPCODE_LBR = 0x2,
> > + BR_CFM_OPCODE_LBM = 0x3,
> > + BR_CFM_OPCODE_LTR = 0x4,
> > + BR_CFM_OPCODE_LTM = 0x5,
> > +};
> > +
> > +/* MEP domain */
> > +enum br_cfm_domain {
> > + BR_CFM_PORT,
> > + BR_CFM_VLAN,
> > +};
> > +
> > +/* MEP direction */
> > +enum br_cfm_mep_direction {
> > + BR_CFM_MEP_DIRECTION_DOWN,
> > + BR_CFM_MEP_DIRECTION_UP,
> > +};
> > +
> > +/* CCM interval supported. */
> > +enum br_cfm_ccm_interval {
> > + BR_CFM_CCM_INTERVAL_NONE,
> > + BR_CFM_CCM_INTERVAL_3_3_MS,
> > + BR_CFM_CCM_INTERVAL_10_MS,
> > + BR_CFM_CCM_INTERVAL_100_MS,
> > + BR_CFM_CCM_INTERVAL_1_SEC,
> > + BR_CFM_CCM_INTERVAL_10_SEC,
> > + BR_CFM_CCM_INTERVAL_1_MIN,
> > + BR_CFM_CCM_INTERVAL_10_MIN,
> > +};
> > +
> > +#endif
> > diff --git a/net/bridge/Makefile b/net/bridge/Makefile
> > index ccb394236fbd..ddc0a9192348 100644
> > --- a/net/bridge/Makefile
> > +++ b/net/bridge/Makefile
> > @@ -27,3 +27,5 @@ bridge-$(CONFIG_NET_SWITCHDEV) += br_switchdev.o
> > obj-$(CONFIG_NETFILTER) += netfilter/
> >
> > bridge-$(CONFIG_BRIDGE_MRP) += br_mrp_switchdev.o br_mrp.o br_mrp_netlink.o
> > +
> > +bridge-$(CONFIG_BRIDGE_CFM) += br_cfm.o
> > diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
> > new file mode 100644
> > index 000000000000..e38cc3e8f262
> > --- /dev/null
> > +++ b/net/bridge/br_cfm.c
> > @@ -0,0 +1,880 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/cfm_bridge.h>
> > +#include <uapi/linux/cfm_bridge.h>
> > +#include "br_private_cfm.h"
> > +
> > +static struct br_cfm_mep *br_mep_find(struct net_bridge *br, u32 instance)
> > +{
> > + struct br_cfm_mep *res = NULL;
> > + struct br_cfm_mep *mep;
> > +
> > + list_for_each_entry_rcu(mep, &br->mep_list, head,
> > + lockdep_rtnl_is_held()) {
> > + if (mep->instance == instance) {
> > + res = mep;
> > + break;
> > + }
> > + }
> > +
> > + return res;
> > +}
>
> It seems br_mep_find() is called only from functions which rely on rtnl being
> held so you can just use normal list traversing functions instead of the rcu
> flavor.
>
I will change that as suggested.
> > +
> > +static struct br_cfm_mep *br_mep_find_ifindex(struct net_bridge *br,
> > + u32 ifindex)
> > +{
> > + struct br_cfm_mep *res = NULL;
> > + struct br_cfm_mep *mep;
> > +
> > + list_for_each_entry_rcu(mep, &br->mep_list, head,
> > + lockdep_rtnl_is_held()) {
> > + if (mep->create.ifindex == ifindex) {
> > + res = mep;
>
> No need for "res", just return "mep" when you find it and NULL below.
>
I will change that as suggested.
> > + break;
> > + }
> > + }
>
> Then you can delete these brackets and the break.
>
I will change that as suggested.
> > +
> > + return res;
> > +}
> > +
> > +static struct br_cfm_peer_mep *br_peer_mep_find(struct br_cfm_mep *mep,
> > + u32 mepid)
> > +{
> > + struct br_cfm_peer_mep *res = NULL;
> > + struct br_cfm_peer_mep *peer_mep;
> > +
> > + list_for_each_entry_rcu(peer_mep, &mep->peer_mep_list, head,
> > + lockdep_rtnl_is_held()) {
> > + if (peer_mep->mepid == mepid) {
> > + res = peer_mep;
>
> No need for "res", just return "peer_mep" when you find it and NULL below.
>
I will change that as suggested.
> > + break;
> > + }
> > + }
>
> Then you can delete these brackets and the break.
>
I will change that as suggested.
> > +
> > + return res;
> > +}
> > +
> > +static struct net_bridge_port *br_mep_get_port(struct net_bridge *br,
> > + u32 ifindex)
> > +{
> > + struct net_bridge_port *res = NULL;
> > + struct net_bridge_port *port;
> > +
> > + list_for_each_entry(port, &br->port_list, list) {
> > + if (port->dev->ifindex == ifindex) {
> > + res = port;
>
> No need for "res", just return "port" when you find it and NULL below.
>
I will change that as suggested.
> > + break;
> > + }
> > + }
>
> Then you can delete these brackets and the break. :)
>
I will change that as suggested.
> > +
> > + return res;
> > +}
> > +
> > +/* Calculate the CCM interval in us. */
> > +static u32 interval_to_us(enum br_cfm_ccm_interval interval)
> > +{
> > + switch (interval) {
> > + case BR_CFM_CCM_INTERVAL_NONE:
> > + return 0;
> > + case BR_CFM_CCM_INTERVAL_3_3_MS:
> > + return 3300;
> > + case BR_CFM_CCM_INTERVAL_10_MS:
> > + return 10 * 1000;
> > + case BR_CFM_CCM_INTERVAL_100_MS:
> > + return 100 * 1000;
> > + case BR_CFM_CCM_INTERVAL_1_SEC:
> > + return 1000 * 1000;
> > + case BR_CFM_CCM_INTERVAL_10_SEC:
> > + return 10 * 1000 * 1000;
> > + case BR_CFM_CCM_INTERVAL_1_MIN:
> > + return 60 * 1000 * 1000;
> > + case BR_CFM_CCM_INTERVAL_10_MIN:
> > + return 10 * 60 * 1000 * 1000;
> > + }
> > + return 0;
> > +}
> > +
> > +/* Convert the interface interval to CCM PDU value. */
> > +static u32 interval_to_pdu(enum br_cfm_ccm_interval interval)
> > +{
> > + switch (interval) {
> > + case BR_CFM_CCM_INTERVAL_NONE:
> > + return 0;
> > + case BR_CFM_CCM_INTERVAL_3_3_MS:
> > + return 1;
> > + case BR_CFM_CCM_INTERVAL_10_MS:
> > + return 2;
> > + case BR_CFM_CCM_INTERVAL_100_MS:
> > + return 3;
> > + case BR_CFM_CCM_INTERVAL_1_SEC:
> > + return 4;
> > + case BR_CFM_CCM_INTERVAL_10_SEC:
> > + return 5;
> > + case BR_CFM_CCM_INTERVAL_1_MIN:
> > + return 6;
> > + case BR_CFM_CCM_INTERVAL_10_MIN:
> > + return 7;
> > + }
> > + return 0;
> > +}
> > +
> > +/* Convert the CCM PDU value to interval on interface. */
> > +static u32 pdu_to_interval(u32 value)
> > +{
> > + switch (value) {
> > + case 0:
> > + return BR_CFM_CCM_INTERVAL_NONE;
> > + case 1:
> > + return BR_CFM_CCM_INTERVAL_3_3_MS;
> > + case 2:
> > + return BR_CFM_CCM_INTERVAL_10_MS;
> > + case 3:
> > + return BR_CFM_CCM_INTERVAL_100_MS;
> > + case 4:
> > + return BR_CFM_CCM_INTERVAL_1_SEC;
> > + case 5:
> > + return BR_CFM_CCM_INTERVAL_10_SEC;
> > + case 6:
> > + return BR_CFM_CCM_INTERVAL_1_MIN;
> > + case 7:
> > + return BR_CFM_CCM_INTERVAL_10_MIN;
> > + }
> > + return BR_CFM_CCM_INTERVAL_NONE;
> > +}
> > +
> > +static void ccm_rx_timer_start(struct br_cfm_peer_mep *peer_mep)
> > +{
> > + u32 interval_us;
> > +
> > + interval_us = interval_to_us(peer_mep->mep->cc_config.exp_interval);
> > + /* Function ccm_rx_dwork must be called with 1/4
> > + * of the configured CC 'expected_interval'
> > + * in order to detect CCM defect after 3.25 interval.
> > + */
> > + queue_delayed_work(system_wq, &peer_mep->ccm_rx_dwork,
> > + usecs_to_jiffies(interval_us / 4));
> > +}
> > +
> > +static void cc_peer_enable(struct br_cfm_peer_mep *peer_mep)
> > +{
> > + memset(&peer_mep->cc_status, 0, sizeof(peer_mep->cc_status));
> > + peer_mep->ccm_rx_count_miss = 0;
> > +
> > + ccm_rx_timer_start(peer_mep);
> > +}
> > +
> > +static void cc_peer_disable(struct br_cfm_peer_mep *peer_mep)
> > +{
> > + cancel_delayed_work_sync(&peer_mep->ccm_rx_dwork);
> > +}
> > +
> > +static struct sk_buff *ccm_frame_build(struct br_cfm_mep *mep,
> > + const struct br_cfm_cc_ccm_tx_info *const tx_info)
> > +
> > +{
> > + struct br_cfm_common_hdr *common_hdr;
> > + struct net_bridge_port *b_port;
> > + struct br_cfm_maid *maid;
> > + u8 *itu_reserved, *e_tlv;
> > + struct ethhdr *eth_hdr;
> > + struct sk_buff *skb;
> > + __be32 *status_tlv;
> > + __be32 *snumber;
> > + __be16 *mepid;
> > +
> > + rcu_read_lock();
> > + b_port = rcu_dereference(mep->b_port);
> > + if (!b_port)
>
> You need to rcu_read_unlock() here, how was this tested?
>
I will assure that rcu_read_unlock is called before return.
> > + return NULL;
> > + skb = dev_alloc_skb(CFM_CCM_MAX_FRAME_LENGTH);
> > + if (!skb)
> > + return NULL;
> > +
> > + skb->dev = b_port->dev;
> > + rcu_read_unlock();
>
> You'll have to explain why the device will still exist past this point.
> I get it, but it'd be nice to have it in the commit msg.
>
I have added a comment in the code explaining.
You want that in the commit message too?
> > + skb->protocol = htons(ETH_P_CFM);
> > + skb->priority = CFM_FRAME_PRIO;
> > +
> > + /* Ethernet header */unnecessary comment
> > + eth_hdr = skb_put(skb, sizeof(*eth_hdr));
> > + ether_addr_copy(eth_hdr->h_dest, tx_info->dmac.addr);
> > + ether_addr_copy(eth_hdr->h_source, mep->config.unicast_mac.addr);
> > + eth_hdr->h_proto = htons(ETH_P_CFM);
> > +
> > + /* Common CFM Header */
> > + common_hdr = skb_put(skb, sizeof(*common_hdr));
> > + common_hdr->mdlevel_version = mep->config.mdlevel << 5;
> > + common_hdr->opcode = CFM_CCM_OPCODE;
> > + common_hdr->flags = (mep->rdi << 7) |
> > + interval_to_pdu(mep->cc_config.exp_interval);
> > + common_hdr->tlv_offset = CFM_CCM_TLV_OFFSET;
> > +
> > + /* Sequence number */
> > + snumber = skb_put(skb, sizeof(*snumber));
> > + if (tx_info->seq_no_update) {
> > + *snumber = cpu_to_be32(mep->ccm_tx_snumber);
> > + mep->ccm_tx_snumber += 1;
> > + } else {
> > + *snumber = 0;
> > + }
> > +
> > + /* MEP ID */
> unnecessary comment
I will change that as suggested.
> > + mepid = skb_put(skb, sizeof(*mepid));
> > + *mepid = cpu_to_be16((u16)mep->config.mepid);
> > +
> > + /* MA ID */
> unnecessary comment
I will change that as suggested.
> > + maid = skb_put(skb, sizeof(*maid));
> > + memcpy(maid->data, mep->cc_config.exp_maid.data, sizeof(maid->data));
> > +
> > + /* ITU reserved (CFM_CCM_ITU_RESERVED_SIZE octets) */
> > + itu_reserved = skb_put(skb, CFM_CCM_ITU_RESERVED_SIZE);
> > + memset(itu_reserved, 0, CFM_CCM_ITU_RESERVED_SIZE);
> > +
> > + /* Generel CFM TLV format:
> > + * TLV type: one byte
> > + * TLV value length: two bytes
> > + * TLV value: 'TLV value length' bytes
> > + */
> > +
> > + /* Port status TLV. The value length is 1. Total of 4 bytes. */
> > + if (tx_info->port_tlv) {
> > + status_tlv = skb_put(skb, sizeof(*status_tlv));
> > + *status_tlv = cpu_to_be32((CFM_PORT_STATUS_TLV_TYPE << 24) |
> > + (1 << 8) | /* Value length */
> > + (tx_info->port_tlv_value & 0xFF));
> > + }
> > +
> > + /* Interface status TLV. The value length is 1. Total of 4 bytes. */
> > + if (tx_info->if_tlv) {
> > + status_tlv = skb_put(skb, sizeof(*status_tlv));
> > + *status_tlv = cpu_to_be32((CFM_IF_STATUS_TLV_TYPE << 24) |
> > + (1 << 8) | /* Value length */
> > + (tx_info->if_tlv_value & 0xFF));
> > + }
> > +
> > + /* End TLV */
> > + e_tlv = skb_put(skb, sizeof(*e_tlv));
> > + *e_tlv = CFM_ENDE_TLV_TYPE;
> > +
> > + return skb;
> > +}
> > +
> > +static void ccm_frame_tx(struct sk_buff *skb)
> > +{
> > + skb_reset_network_header(skb);
> > + dev_queue_xmit(skb);
> > +}
> > +
> > +/* This function is called with the configured CC 'expected_interval'
> > + * in order to drive CCM transmission when enabled.
> > + */
> > +static void ccm_tx_work_expired(struct work_struct *work)
> > +{
> > + struct delayed_work *del_work;
> > + struct br_cfm_mep *mep;
> > + struct sk_buff *skb;
> > + u32 interval_us;
> > +
> > + del_work = to_delayed_work(work);
> > + mep = container_of(del_work, struct br_cfm_mep, ccm_tx_dwork);
> > +
> > + if (time_before_eq(mep->ccm_tx_end, jiffies)) {
> > + /* Transmission period has ended */
> > + mep->cc_ccm_tx_info.period = 0;
> > + return;
> > + }
> > +
> > + skb = ccm_frame_build(mep, &mep->cc_ccm_tx_info);
> > + if (skb)
> > + ccm_frame_tx(skb);
> > +
> > + interval_us = interval_to_us(mep->cc_config.exp_interval);
> > + queue_delayed_work(system_wq, &mep->ccm_tx_dwork,
> > + usecs_to_jiffies(interval_us));
> > +}
> > +
> > +/* This function is called with 1/4 of the configured CC 'expected_interval'
> > + * in order to detect CCM defect after 3.25 interval.
> > + */
> > +static void ccm_rx_work_expired(struct work_struct *work)
> > +{
> > + struct br_cfm_peer_mep *peer_mep;
> > + struct delayed_work *del_work;
> > +
> > + del_work = to_delayed_work(work);
> > + peer_mep = container_of(del_work, struct br_cfm_peer_mep, ccm_rx_dwork);
> > +
> > + /* After 13 counts (4 * 3,25) then 3.25 intervals are expired */
> > + if (peer_mep->ccm_rx_count_miss < 13) {
> > + /* 3.25 intervals are NOT expired without CCM reception */
> > + peer_mep->ccm_rx_count_miss++;
> > +
> > + /* Start timer again */
> > + ccm_rx_timer_start(peer_mep);
> > + } else {
> > + /* 3.25 intervals are expired without CCM reception.
> > + * CCM defect detected
> > + */
> > + peer_mep->cc_status.ccm_defect = true;
> > +
> > + /* Change in CCM defect status - notify */
> > + }
> > +}
> > +
> > +static u32 ccm_tlv_extract(struct sk_buff *skb, u32 index,
> > + struct br_cfm_peer_mep *peer_mep)
> > +{
> > + __be32 *s_tlv;
> > + __be32 _s_tlv;
> > + u32 h_s_tlv;
> > + u8 *e_tlv;
> > + u8 _e_tlv;
> > +
> > + e_tlv = skb_header_pointer(skb, index, sizeof(_e_tlv), &_e_tlv);
> > + if (!e_tlv)
> > + return 0;
> > +
> > + /* TLV is present - get the status TLV */
> > + s_tlv = skb_header_pointer(skb,
> > + index,
> > + sizeof(_s_tlv), &_s_tlv);
> > + if (!s_tlv)
> > + return 0;
> > +
> > + h_s_tlv = ntohl(*s_tlv);
> > + if ((h_s_tlv >> 24) == CFM_IF_STATUS_TLV_TYPE) {
> > + /* Interface status TLV */
> > + peer_mep->cc_status.tlv_seen = true;
> > + peer_mep->cc_status.if_tlv_value = (h_s_tlv & 0xFF);
> > + }
> > +
> > + if ((h_s_tlv >> 24) == CFM_PORT_STATUS_TLV_TYPE) {
> > + /* Port status TLV */
> > + peer_mep->cc_status.tlv_seen = true;
> > + peer_mep->cc_status.port_tlv_value = (h_s_tlv & 0xFF);
> > + }
> > +
> > + /* The Sender ID TLV is not handled */
> > + /* The Organization-Specific TLV is not handled */
> > +
> > + /* Return the length of this tlv.
> > + * This is the length of the value field plus 3 bytes for size of type
> > + * field and length field
> > + */
> > + return ((h_s_tlv >> 8) & 0xFFFF) + 3;
> > +}
> > +
> > +/* note: already called with rcu_read_lock */
> > +static int br_cfm_frame_rx(struct net_bridge_port *port, struct sk_buff *skb)
> > +{
> > + u32 mdlevel, interval, size, index, max;
> > + const struct br_cfm_common_hdr *hdr;
> > + struct br_cfm_peer_mep *peer_mep;
> > + const struct br_cfm_maid *maid;
> > + struct br_cfm_common_hdr _hdr;
> > + struct br_cfm_maid _maid;
> > + struct br_cfm_mep *mep;
> > + struct net_bridge *br;
> > + __be32 *snumber;
> > + __be32 _snumber;
> > + __be16 *mepid;
> > + __be16 _mepid;
> > +
> > + /* If port is disabled don't accept any frames */
>
> Unnecessary comment, obvious from the code.
>
I will change that as suggested.
> > + if (port->state == BR_STATE_DISABLED)
> > + return 0;
> > +
> > + hdr = skb_header_pointer(skb, 0, sizeof(_hdr), &_hdr);
> > + if (!hdr)
> > + return 1;
> > +
> > + br = port->br;
> > + mep = br_mep_find_ifindex(br, port->dev->ifindex);
> > + if (unlikely(!mep))
> > + /* No MEP on this port - must be forwarded */
> > + return 0;
> > +
> > + mdlevel = hdr->mdlevel_version >> 5;
> > + if (mdlevel > mep->config.mdlevel)
> > + /* The level is above this MEP level - must be forwarded */
> > + return 0;
> > +
> > + if ((hdr->mdlevel_version & 0x1F) != 0) {
> > + /* Invalid version */
> > + mep->status.version_unexp_seen = true;
> > + return 1;
> > + }
> > +
> > + if (mdlevel < mep->config.mdlevel) {
> > + /* The level is below this MEP level */
> > + mep->status.rx_level_low_seen = true;
> > + return 1;
> > + }
> > +
> > + if (hdr->opcode == BR_CFM_OPCODE_CCM) {
> > + /* CCM PDU received. */
> > + /* MA ID is after common header + sequence number + MEP ID */
> > + maid = skb_header_pointer(skb,
> > + CFM_CCM_PDU_MAID_OFFSET,
> > + sizeof(_maid), &_maid);
> > + if (!maid)
> > + return 1;
> > + if (memcmp(maid->data, mep->cc_config.exp_maid.data,
> > + sizeof(maid->data)))
> > + /* MA ID not as expected */
> > + return 1;
> > +
> > + /* MEP ID is after common header + sequence number */
> > + mepid = skb_header_pointer(skb,
> > + CFM_CCM_PDU_MEPID_OFFSET,
> > + sizeof(_mepid), &_mepid);
> > + if (!mepid)
> > + return 1;
> > + peer_mep = br_peer_mep_find(mep, (u32)ntohs(*mepid));
> > + if (!peer_mep)
> > + return 1;
> > +
> > + /* Interval is in common header flags */
> > + interval = hdr->flags & 0x07;
> > + if (mep->cc_config.exp_interval != pdu_to_interval(interval))
> > + /* Interval not as expected */
> > + return 1;
> > +
> > + /* A valid CCM frame is received */
> > + if (peer_mep->cc_status.ccm_defect) {
> > + peer_mep->cc_status.ccm_defect = false;
> > +
> > + /* Change in CCM defect status - notify */
> > +
> > + /* Start CCM RX timer */
> > + ccm_rx_timer_start(peer_mep);
> > + }
> > +
> > + peer_mep->cc_status.seen = true;
> > + peer_mep->ccm_rx_count_miss = 0;
> > +
> > + /* RDI is in common header flags */
> > + peer_mep->cc_status.rdi = (hdr->flags & 0x80) ? true : false;
> > +
> > + /* Sequence number is after common header */
> > + snumber = skb_header_pointer(skb,
> > + CFM_CCM_PDU_SEQNR_OFFSET,
> > + sizeof(_snumber), &_snumber);
> > + if (!snumber)
> > + return 1;
> > + if (ntohl(*snumber) != (mep->ccm_rx_snumber + 1))
> > + /* Unexpected sequence number */
> > + peer_mep->cc_status.seq_unexp_seen = true;
> > +
> > + mep->ccm_rx_snumber = ntohl(*snumber);
> > +
> > + /* TLV end is after common header + sequence number + MEP ID +
> > + * MA ID + ITU reserved
> > + */
> > + index = CFM_CCM_PDU_TLV_OFFSET;
> > + max = 0;
> > + do { /* Handle all TLVs */
> > + size = ccm_tlv_extract(skb, index, peer_mep);
> > + index += size;
> > + max += 1;
> > + } while (size != 0 && max < 4); /* Max four TLVs possible */
> > +
> > + return 1;
> > + }
> > +
> > + mep->status.opcode_unexp_seen = true;
> > +
> > + return 1;
> > +}
> > +
> > +static struct br_frame_type cfm_frame_type __read_mostly = {
> > + .type = cpu_to_be16(ETH_P_CFM),
> > + .func = br_cfm_frame_rx,
> > +};
> > +
> > +/* note: already called with rtnl_lock */
>
> Instead of these note comments you can just add ASSERT_RTNL()
> in the beginning of
> these functions if you want to show they expect
> rtnl to be held.
>
I will change that as suggested.
> > +int br_cfm_mep_create(struct net_bridge *br,
> > + const u32 instance,
> > + struct br_cfm_mep_create *const create,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct net_bridge_port *p;
> > + struct br_cfm_mep *mep;
> > +
> > + if (create->domain == BR_CFM_VLAN) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "VLAN domain not supported");
> > + return -EINVAL;
> > + }
> > + if (create->domain != BR_CFM_PORT) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Invalid domain value");
> > + return -EINVAL;
> > + }
> > + if (create->direction == BR_CFM_MEP_DIRECTION_UP) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Up-MEP not supported");
> > + return -EINVAL;
> > + }
> > + if (create->direction != BR_CFM_MEP_DIRECTION_DOWN) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Invalid direction value");
> > + return -EINVAL;
> > + }
> > + if (!br_mep_get_port(br, create->ifindex))
>
> You can set extack here, too.
>
I will change that as suggested.
> > + return -EINVAL;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance already created");
> > + return -EINVAL;
> > + }
> > +
> > + /* In PORT domain only one instance can be created per port */
> > + if (create->domain == BR_CFM_PORT) {
> > + mep = br_mep_find_ifindex(br, create->ifindex);
> > + if (mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Only one Port MEP on a port allowed");
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + mep = kzalloc(sizeof(*mep), GFP_KERNEL);
> > + if (!mep)
> > + return -ENOMEM;
> > +
> > + /* Save create parameters */
> > + mep->create = *create;
> > + mep->instance = instance;
> > + p = br_mep_get_port(br, mep->create.ifindex);
>
> Please do this only once in the beginning and use the local variable here.
>
I will change that as suggested.
> > + rcu_assign_pointer(mep->b_port, p);
>
> Can b_port be already assigned here?
>
Maybe I do not understand your comment. As the mep instance has just
been allocated I cannot see how the pointer can be already assigned.
> > +
> > + INIT_LIST_HEAD(&mep->peer_mep_list);
> > + INIT_DELAYED_WORK(&mep->ccm_tx_dwork, ccm_tx_work_expired);
> > +
> > + if (list_empty(&br->mep_list))
> > + br_add_frame(br, &cfm_frame_type);
> > +
> > + /* Add instance to list */
> unnecessary comment
I will change that as suggested.
> > + list_add_tail_rcu(&mep->head, &br->mep_list);
> > +
> > + return 0;
> > +}
> > +
> > +static void mep_delete_implementation(struct net_bridge *br,
> > + struct br_cfm_mep *mep)
> > +{
> > + struct br_cfm_peer_mep *peer_mep;
> > +
> > + /* Empty and free peer MEP list */
> > + list_for_each_entry_rcu(peer_mep, &mep->peer_mep_list, head,
> > + lockdep_rtnl_is_held()) {
>
> You can use normal list traversal
I will change that as suggested.
> > + cancel_delayed_work_sync(&peer_mep->ccm_rx_dwork);
> > + list_del_rcu(&peer_mep->head);
> > + kfree_rcu(peer_mep, rcu);
> > + }
> > +
> > + /* Stop transmitting */
>
> unnecessary comment
>
I will change that as suggested.
> > + cancel_delayed_work_sync(&mep->ccm_tx_dwork);
> > +
> > + /* Free the MEP instance */
>
> unnecessary comment, code is pretty clear
>
I will change that as suggested.
> > + rcu_assign_pointer(mep->b_port, NULL);
>
> RCU_INIT_POINTER
>
I will change that as suggested.
> > + list_del_rcu(&mep->head);
> > + kfree_rcu(mep, rcu);
> > +
> > + if (list_empty(&br->mep_list))
> > + br_del_frame(br, &cfm_frame_type);
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_mep_delete(struct net_bridge *br,
> > + const u32 instance,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_mep *mep;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
> > +
> > + mep_delete_implementation(br, mep);
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_mep_config_set(struct net_bridge *br,
> > + const u32 instance,
> > + const struct br_cfm_mep_config *const config,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_mep *mep;
> > +
> > + if (config->mdlevel > 7) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "mdlevel is invalid");
> > + return -EINVAL;
> > + }
> > + if (config->mepid > 0x1FFF) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "mepid is invalid");
>
> MEP-ID ?
>
I have added a comment explaining what the MEP-ID is
> > + return -EINVAL;
> > + }
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
> > +
> > + /* Save config parameters */
>
> unnecessary comment
>
I will change that as suggested.
> > + mep->config = *config;
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_mep_counters_clear(struct net_bridge *br,
> > + const u32 instance,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_mep *mep;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
> > +
> > + memset(&mep->counters, 0, sizeof(mep->counters));
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_cc_config_set(struct net_bridge *br,
> > + const u32 instance,
> > + const struct br_cfm_cc_config *const config,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_peer_mep *peer_mep;
> > + struct br_cfm_mep *mep;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
>
> "does not exist" somehow seems more fitting, but up to you. :)
>
I will change that as suggested.
> > + return -EINVAL;
> > + }
> > +
> > + /* Check for no change in configuration */
> > + if (memcmp(config, &mep->cc_config, sizeof(*config)) == 0)
> > + return 0;
> > +
> > + if (config->enable && !mep->cc_config.enable)
> > + /* CC is enabled */
> > + list_for_each_entry_rcu(peer_mep, &mep->peer_mep_list, head,
> > + lockdep_rtnl_is_held())
> > + cc_peer_enable(peer_mep);
>
> Use normal list traversal when rtnl is being held.
>
I will change that as suggested.
> > +
> > + if (!config->enable && mep->cc_config.enable)
> > + /* CC is disabled */
> > + list_for_each_entry_rcu(peer_mep, &mep->peer_mep_list, head,
> > + lockdep_rtnl_is_held())
>
> Same here.
>
I will change that as suggested.
> > + cc_peer_disable(peer_mep);
> > +
> > + /* Save new cc_config parameters */
>
> unnecessary comment
>
I will change that as suggested.
> > + mep->cc_config = *config;
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_cc_peer_mep_add(struct net_bridge *br, const u32 instance,
> > + u32 mepid,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_peer_mep *peer_mep;
> > + struct br_cfm_mep *mep;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
> > + if (mepid > 0x1FFF) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "mepid is invalid");
>
> Shouldn't this be MEP-ID as it's written out in other places?
>
I have aligned the error messeges
> > + return -EINVAL;
> > + }
> > +
> > + peer_mep = br_peer_mep_find(mep, mepid);
> > + if (peer_mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Peer MEP-ID already added");
>
> perhaps "already exists" ?
>
I will change that as suggested.
> > + return -EINVAL;
>
> EEXIST ?
>
I will change that as suggested.
> > + }
> > +
> > + peer_mep = kzalloc(sizeof(*peer_mep), GFP_KERNEL);
> > + if (!peer_mep)
> > + return -ENOMEM;
> > +
> > + peer_mep->mepid = mepid;
> > + peer_mep->mep = mep;
> > + INIT_DELAYED_WORK(&peer_mep->ccm_rx_dwork, ccm_rx_work_expired);
> > +
> > + if (mep->cc_config.enable)
> > + /* CC is enabled */
>
> Unnecessary comment, the function name is self-descriptive.
>
I will change that as suggested.
> > + cc_peer_enable(peer_mep);
> > +
> > + /* Add Peer MEP to list */
>
> This comment is unnecessary, the code is seems pretty clear.
>
I will change that as suggested.
> > + list_add_tail_rcu(&peer_mep->head, &mep->peer_mep_list);
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_cc_peer_mep_remove(struct net_bridge *br, const u32 instance,
> > + u32 mepid,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_peer_mep *peer_mep;
> > + struct br_cfm_mep *mep;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
> > +
> > + peer_mep = br_peer_mep_find(mep, mepid);
> > + if (!peer_mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Peer MEP-ID already deleted");
>
> Why already deleted ? Maybe it didn't exist at all ? :) "does not exist" sounds
> more appropriate.
>
I will change that as suggested.
> > + return -EINVAL;
>
> in that spirit ENOENT here ?
>
I will change that as suggested.
> > + }
> > +
> > + cc_peer_disable(peer_mep);
> > +
> > + list_del_rcu(&peer_mep->head);
> > + kfree_rcu(peer_mep, rcu);
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_cc_rdi_set(struct net_bridge *br, const u32 instance,
> > + const bool rdi, struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_mep *mep;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
> > +
> > + /* Save rdi parameters */
>
> unnecessary comment
>
I will change that as suggested.
> > + mep->rdi = rdi;
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_cc_ccm_tx(struct net_bridge *br, const u32 instance,
> > + const struct br_cfm_cc_ccm_tx_info *const tx_info,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_mep *mep;
> > + struct sk_buff *skb;
> > + u32 interval_us;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
>
> All of these seem to start with finding the instance and checking if it exists.
> Perhaps this can be factored out in the netlink handling code?
>
I agree that it could be done in br_cfm_netlink.c but I prfer to keep it
like this to make code in this file more self contained regarding mep
instances.
If you think it must be changed I will ofc do it.
> > +
> > + if (memcmp(tx_info, &mep->cc_ccm_tx_info, sizeof(*tx_info)) == 0) {
> > + /* No change in tx_info. */
> > + if (mep->cc_ccm_tx_info.period == 0)
> > + /* Transmission is not enabled - just return */
> > + return 0;
> > +
> > + /* Transmission is ongoing, the end time is recalculated */
> > + mep->ccm_tx_end = jiffies +
> > + usecs_to_jiffies(tx_info->period * 1000000);
> > + return 0;
> > + }
> > +
> > + if (tx_info->period == 0 && mep->cc_ccm_tx_info.period == 0)
> > + /* Some change in info and transmission is not ongoing */
> > + goto save;
> > +
> > + if (tx_info->period != 0 && mep->cc_ccm_tx_info.period != 0) {
> > + /* Some change in info and transmission is ongoing */
> > + /* The end time is recalculated */
>
> Multi-line comments format is:
> /* comment1
> * comment2
> */
>
I will change that as suggested.
> > + mep->ccm_tx_end = jiffies +
> > + usecs_to_jiffies(tx_info->period * 1000000);
> > +
> > + goto save;
> > + }
> > +
> > + if (tx_info->period == 0 && mep->cc_ccm_tx_info.period != 0) {
> > + /* CCM transmission stop */
>
> This comment seems unnecessary, the code is pretty clear.
>
I will change that as suggested.
> > + cancel_delayed_work_sync(&mep->ccm_tx_dwork);
> > + goto save;
> > + }
> > +
> > + /* Start delayed work to transmit CCM frames */
>
> This comment seems unnecessary, the code is pretty clear.
>
I will change that as suggested.
> > + mep->ccm_tx_end = jiffies + usecs_to_jiffies(tx_info->period * 1000000);
> > + interval_us = interval_to_us(mep->cc_config.exp_interval);
> > + queue_delayed_work(system_wq, &mep->ccm_tx_dwork,
> > + usecs_to_jiffies(interval_us));
> > +
> > + /* Send first CCM frame now */
> > + mep->ccm_tx_snumber = 0;
> > + skb = ccm_frame_build(mep, tx_info);
> > + if (skb)
> > + ccm_frame_tx(skb);
> > +
> > +save:
> > + /* Save tx_info parameters */
> > + mep->cc_ccm_tx_info = *tx_info;
> > +
> > + return 0;
> > +}
> > +
> > +/* note: already called with rtnl_lock */
> > +int br_cfm_cc_counters_clear(struct net_bridge *br, const u32 instance,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct br_cfm_mep *mep;
> > +
> > + mep = br_mep_find(br, instance);
> > + if (!mep) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "MEP instance not created");
> > + return -EINVAL;
> > + }
> > +
> > + memset(&mep->cc_counters, 0, sizeof(mep->cc_counters));
> > +
> > + return 0;
> > +}
> > +
> > +bool br_cfm_created(struct net_bridge *br)
> > +{
> > + return !list_empty(&br->mep_list);
> > +}
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index 6294a3e51a33..385a6a6aa17e 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -1363,6 +1363,22 @@ static inline int br_mrp_fill_info(struct sk_buff *skb, struct net_bridge *br)
> >
> > #endif
> >
> > +/* br_cfm.c */
> > +#if IS_ENABLED(CONFIG_BRIDGE_CFM)
> > +int br_cfm_rx_frame_process(struct net_bridge_port *p, struct sk_buff *skb);
> > +bool br_cfm_created(struct net_bridge *br);
> > +#else
> > +static inline int br_cfm_rx_frame_process(struct net_bridge_port *p, struct sk_buff *skb)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline bool br_cfm_created(struct net_bridge *br)
> > +{
> > + return false;
> > +}
> > +#endif
> > +
> > /* br_netlink.c */
> > extern struct rtnl_link_ops br_link_ops;
> > int br_netlink_init(void);
> > diff --git a/net/bridge/br_private_cfm.h b/net/bridge/br_private_cfm.h
> > new file mode 100644
> > index 000000000000..379334a42667
> > --- /dev/null
> > +++ b/net/bridge/br_private_cfm.h
> > @@ -0,0 +1,242 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef _BR_PRIVATE_CFM_H_
> > +#define _BR_PRIVATE_CFM_H_
> > +
> > +#include "br_private.h"
> > +#include <uapi/linux/cfm_bridge.h>
> > +
> > +/* MEP create parameters */
> > +struct br_cfm_mep_create {
> > + enum br_cfm_domain domain; /* Domain for this MEP */
> > + enum br_cfm_mep_direction direction; /* Up or Down MEP direction */
> > + u32 ifindex; /* Residence port */
> > + u16 vid; /* Classified VID in VLAN domain */
> > +};
> > +
> > +/* Create/Delete a MEP. */
>
> This is clear.
>
I will change that as suggested.
> > +int br_cfm_mep_create(struct net_bridge *br,
> > + const u32 instance,
> > + struct br_cfm_mep_create *const create,
> > + struct netlink_ext_ack *extack);
> > +int br_cfm_mep_delete(struct net_bridge *br,
> > + const u32 instance,
> > + struct netlink_ext_ack *extack);
> > +
> > +/* MEP configuration parameters */
> > +struct br_cfm_mep_config {
> > + u32 mdlevel;
> > + u32 mepid; /* MEPID for this MEP */
> > + struct mac_addr unicast_mac; /* The MEP unicast MAC */
> > + /* Added tag VID in case of tagged PORT domain. Untagged if zero. */
> > + u16 vid;
> > +};
> > +
> > +/* Set general configuration of MEP */
>
> The function name is self-descriptive.
>
I will change that as suggested.
> > +int br_cfm_mep_config_set(struct net_bridge *br,
> > + const u32 instance,
> > + const struct br_cfm_mep_config *const config,
> > + struct netlink_ext_ack *extack);
> > +
> > +/* MEP status. */
> > +struct br_cfm_mep_status {
> > + /* Indications that an OAM PDU has been seen. */
> > + /* Are cleared during call to br_cfm_status_get(). */
> > + bool opcode_unexp_seen; /* RX of OAM PDU with unexpected opcode */
> > + bool dmac_unexp_seen; /* RX of OAM Frame with unexpected DMAC */
> > + bool tx_level_low_seen; /* TX of OAM PDU with level low blocked */
> > + bool version_unexp_seen; /* RX of OAM PDU with unexpected version */
> > + bool rx_level_low_seen; /* Rx of OAM PDU with level low */
> > +};
> > +
> > +/* MEP status get. */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_mep_status_get(const u32 instance, struct br_cfm_mep_status *const status);
> > +
> > +/* MEP counters */
>
> unnecessary comment
>
I will change that as suggested.
> > +struct br_cfm_mep_counters {
> > + /* All the following counters are cleared during call to */
> > + /* br_cfm_counters_clear(). */
> > +
> > + /* OAM PDU Rx and Tx counters. */
> > + u64 rx_counter;
> > + u64 tx_counter;
> > +
> > + /* Rx/Tx PDUs that are discarded due to filtering */
> > + u64 rx_discard_counter; /* Check of MEL or DMAC or Version or CCM */
> > + u64 tx_discard_counter; /* Check of MEL */
> > +};
> > +
> > +/* Get MEP counters. */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_mep_counters_get(const u32 instance, struct br_cfm_mep_counters *counters);
> > +
> > +/* MEP counter clear. */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_mep_counters_clear(struct net_bridge *br,
> > + const u32 instance,
> > + struct netlink_ext_ack *extack);
> > +
> > +struct br_cfm_maid {
> > + u8 data[CFM_MAID_LENGTH];
> > +};
> > +
> > +struct br_cfm_cc_config {
> > + /* Expected received CCM PDU MAID. */
> > + struct br_cfm_maid exp_maid;
> > +
> > + /* Expected received CCM PDU interval. */
> > + /* Transmitting CCM PDU interval when CCM tx is enabled. */
> > + enum br_cfm_ccm_interval exp_interval;
> > +
> > + /* Expected received CCM PDU Priority */
> > + u8 exp_priority;
> > +
> > + bool enable; /* Enable/disable CCM PDU handling */
> > +};
> > +
> > +/* Set CC configuration of a MEP */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_cc_config_set(struct net_bridge *br,
> > + const u32 instance,
> > + const struct br_cfm_cc_config *const config,
> > + struct netlink_ext_ack *extack);
> > +
> > +/* Set CC Peer MEP ID add/remove */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_cc_peer_mep_add(struct net_bridge *br, const u32 instance,
> > + u32 peer_mep_id,
> > + struct netlink_ext_ack *extack);
> > +int br_cfm_cc_peer_mep_remove(struct net_bridge *br, const u32 instance,
> > + u32 peer_mep_id,
> > + struct netlink_ext_ack *extack);
> > +
> > +/* Transmitted CCM Remote Defect Indication status set.
> > + * This RDI is inserted in transmitted CCM PDUs if CCM transmission is enabled.
> > + * See br_cfm_cc_ccm_tx() with interval != BR_CFM_CCM_INTERVAL_NONE
> > + */
> > +int br_cfm_cc_rdi_set(struct net_bridge *br, const u32 instance,
> > + const bool rdi, struct netlink_ext_ack *extack);
> > +
> > +/* OAM PDU Tx information */
> > +struct br_cfm_cc_ccm_tx_info {
> > + struct mac_addr dmac;
> > + u8 priority;
> > + bool dei;
> > + /* The CCM will be transmitted for this period in seconds.
> > + * Call br_cfm_cc_ccm_tx before timeout to keep transmission alive.
> > + * When period is zero any ongoing transmission will be stopped.
> > + */
> > + u32 period;
> > +
> > + bool seq_no_update; /* Update Tx CCM sequence number */
> > + bool if_tlv; /* Insert Interface Status TLV */
> > + u8 if_tlv_value; /* Interface Status TLV value */
> > + bool port_tlv; /* Insert Port Status TLV */
> > + u8 port_tlv_value; /* Port Status TLV value */
> > + /* Sender ID TLV ??
> > + * Organization-Specific TLV ??
> > + */
> > +};
> > +
> > +/* Transmit CCM PDU */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_cc_ccm_tx(struct net_bridge *br, const u32 instance,
> > + const struct br_cfm_cc_ccm_tx_info *const tx_info,
> > + struct netlink_ext_ack *extack);
> > +
> > +struct br_cfm_cc_status {
> > + /* (interval == 0) for last received CCM PDU */
> > + bool zero_interval;
> > +
> > + /* Unexpected MD Level received. Cleared after 3.5 interval */
> > + bool mdlevel_unexp;
> > +
> > + /* Unexpected MAID received. Cleared after 3.5 interval */
> > + bool maid_unexp;
> > +
> > + /* Unexpected MEPID received. Cleared after 3.5 interval */
> > + bool mepid_unexp;
> > +};
> > +
> > +/* Get CC status. */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_cc_status_get(const u32 instance, struct br_cfm_cc_status *const status);
> > +
> > +struct br_cfm_cc_counters {
> > + /* CCM PDU RX and TX counters
> > + * Are cleared during call to br_cfm_cc_counters_clear()
> > + */
> > + u64 rx_valid_counter;
> > + u64 rx_invalid_counter;
> > + u64 rx_oo_counter; /* Out of Order sequence numbers counter */
> > + u64 tx_counter;
> > +};
> > +
> > +/* Get CC counters. */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_cc_counters_get(const u32 instance, struct br_cfm_cc_counters *counters);
> > +
> > +/* CC counter clear. */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_cc_counters_clear(struct net_bridge *br, const u32 instance,
> > + struct netlink_ext_ack *extack);
> > +
> > +struct br_cfm_cc_peer_status {
> > + struct mac_addr unicast_mac; /* The Peer MEP unicast MAC */
> > +
> > + /* Unexpected Interval. Cleared after 3.5 interval */
> > + bool interval_unexp;
> > +
> > + /* Unexpected Priority received. Cleared after 3.5 interval */
> > + bool priority_unexp;
> > +
> > + /* This CCM related status is based on the latest received CCM PDU. */
> > + u32 rx_ifindex; /* The ingress port */
> > + u8 port_tlv_value; /* Port Status TLV value */
> > + u8 if_tlv_value; /* Interface Status TLV value */
> > +
> > + /* CCM has not been received for 3.25 intervals */
> > + bool ccm_defect;
> > +
> > + /* (RDI == 1) for last received CCM PDU */
> > + bool rdi;
> > +
> > + /* Indications that a CCM PDU has been seen.
> > + * Are cleared during call to br_cfm_cc_status_get()
> > + */
> > + bool seen; /* CCM PDU received */
> > + bool tlv_seen; /* CCM PDU with TLV received */
> > + /* CCM PDU with unexpected sequence number received */
> > + bool seq_unexp_seen;
> > +};
> > +
> > +/* Get CC peer MEP status. */
> unnecessary comment
I will change that as suggested.
> > +int br_cfm_cc_peer_status_get(const u32 instance, const u32 mepid,
> > + struct br_cfm_cc_peer_status *const status);
> > +
> > +struct br_cfm_mep {
> > + /* list header of MEP instances */
> > + struct list_head head;
> > + u32 instance;
> > + struct br_cfm_mep_create create;
> > + struct br_cfm_mep_config config;
> > + struct br_cfm_cc_config cc_config;
> > + struct br_cfm_cc_ccm_tx_info cc_ccm_tx_info;
> > + struct br_cfm_mep_counters counters;
> > + struct br_cfm_cc_counters cc_counters;
> > + /* List of multiple peer MEPs */
> > + struct list_head peer_mep_list;
> > + struct rcu_head rcu;
>
> Please move the rcu at the end, it's used only on deletion.
>
I have moved it but there are consequences regarding memory holes in the
structs. According to pahole this create a hole of 6 bytes in br_cfm_mep
and 4 bytes in br_cfm_peer_mep
> > + struct net_bridge_port __rcu *b_port;
> > + unsigned long ccm_tx_end;
> > + struct delayed_work ccm_tx_dwork;
> > + struct br_cfm_cc_status cc_status;
> > + u32 ccm_tx_snumber;
> > + u32 ccm_rx_snumber;
> > + struct br_cfm_mep_status status;
> > + bool rdi;
> > +};
> > +
> > +struct br_cfm_peer_mep {
> > + struct list_head head;
> > + struct br_cfm_mep *mep;
> > + struct rcu_head rcu;
> > + struct delayed_work ccm_rx_dwork;
> > + u32 mepid;
> > + struct br_cfm_cc_peer_status cc_status;
> > + u32 ccm_rx_count_miss;
> > +};
> > +
> > +#endif /* _BR_PRIVATE_CFM_H_ */
>
--
/Henrik
Powered by blists - more mailing lists