[lldp-devel] [PATCH 2/8] introduce support for multiple agents per port

Jens Osterkamp jens at linux.vnet.ibm.com
Mon Aug 15 12:57:58 UTC 2011


On Wednesday 10 August 2011, John Fastabend wrote:
> On 8/5/2011 8:35 AM, Jens Osterkamp wrote:
> > This patch introduces support for multiple agents per port.
> > Each agent sends and receives frames to and from different group mac
> > addresses. Right now these are nearest bridge, nearest customer bridge and
> > nearest non-twoport mac-relaying bridge.
> > 
> > It splits out most of the agent specific variables from the port structure
> > to an agent structure. This requires changes in the rx and tx state
> > machines.
> > 
> > With the changes for the multi-agent support, adminStatus has moved from the
> > port to the agent. As port->adminStatus has been used to control the VDP
> > state machines as well, a new mechanism was needed here.
> > The VDP module does not have agent support, so port->adminStatus is replaced
> > by the vdp->enableTx flag. If this is set to yes by lldptool, VDP will be
> > active, and will be inactive if set to no.
> > 
> > Signed-off-by: Jens Osterkamp <jens at linux.vnet.ibm.com>
> 
> Some minor nits and one null check needed see comments below.

Hi John,

thank you for your thorough review !
Please see my comments below.

> 
> > ---
> >  config.c               |  115 ++++++++++--
> >  dcb_protocol.c         |    9 +-
> >  ecp/ecp.h              |    6 +-
> >  ecp/ecp_rx.c           |   15 +-
> >  ecp/ecp_tx.c           |   14 +-
> >  event_iface.c          |   45 +++--
> >  include/clif_msgs.h    |    2 +
> >  include/config.h       |    8 +-
> >  include/lldp.h         |    2 +-
> >  include/lldp_8021qaz.h |   11 +-
> >  include/lldp_8023.h    |    7 +-
> >  include/lldp_basman.h  |    7 +-
> >  include/lldp_dcbx.h    |   10 +-
> >  include/lldp_evb.h     |    9 +-
> >  include/lldp_mand.h    |    7 +-
> >  include/lldp_med.h     |    7 +-
> >  include/lldp_mod.h     |   12 +-
> >  include/lldp_vdp.h     |    1 +
> >  include/tlv_dcbx.h     |   14 +-
> >  lldp/agent.c           |  159 ++++++++++++++--
> >  lldp/agent.h           |  143 ++++++++++++++-
> >  lldp/l2_packet.h       |    9 +-
> >  lldp/l2_packet_linux.c |   16 +-
> >  lldp/ports.c           |  285 +++++++++++++---------------
> >  lldp/ports.h           |  108 ++---------
> >  lldp/rx.c              |  496 ++++++++++++++++++++++++------------------------
> >  lldp/states.h          |   57 +++---
> >  lldp/tx.c              |  327 +++++++++++++++++---------------
> >  lldp_8021qaz.c         |  134 ++++++++-----
> >  lldp_8021qaz_cmds.c    |   16 +-
> >  lldp_8023.c            |   22 ++-
> >  lldp_8023_cmds.c       |    2 +-
> >  lldp_basman.c          |   19 +-
> >  lldp_basman_cmds.c     |    6 +-
> >  lldp_dcbx.c            |   82 ++++----
> >  lldp_dcbx_cmds.c       |   16 +-
> >  lldp_evb.c             |   36 ++--
> >  lldp_evb_cmds.c        |   26 ++--
> >  lldp_mand.c            |   33 ++--
> >  lldp_mand_cmds.c       |   19 ++-
> >  lldp_med.c             |   19 +-
> >  lldp_med_cmds.c        |    4 +-
> >  lldp_tlv.c             |   17 +-
> >  lldp_vdp.c             |   43 ++--
> >  lldpad.c               |   11 +-
> >  tlv_dcbx.c             |  133 ++++++++------
> >  46 files changed, 1455 insertions(+), 1084 deletions(-)
> > 
> 
> [...]
> 
> 
> > 
> > @@ -226,6 +233,7 @@ static void event_if_decode_nlmsg(int route_type, void *data, int len)
> >         const struct lldp_mod_ops *ops;
> >         struct rtattr *rta;
> >         char device_name[IFNAMSIZ];
> > +       struct lldp_agent *agent;
> >         int attrlen;
> >         int valid;
> >         int link_status = IF_OPER_UNKNOWN;
> > @@ -268,14 +276,21 @@ static void event_if_decode_nlmsg(int route_type, void *data, int len)
> >                         if (!valid)
> >                                 break;
> > 
> > -                       LIST_FOREACH(np, &lldp_head, lldp) {
> > -                               ops = np->ops;
> > -                               if (ops->lldp_mod_ifdown)
> > -                                       ops->lldp_mod_ifdown(device_name);
> > +                       struct port *port = port_find_by_name(device_name);
> 				      ^^^^^
> Need to check port is not null here?

Yes, will be in my next version, thanks !

> 
> > +                       LIST_FOREACH(agent, &port->agent_head, entry) {
> > +                               LLDPAD_DBG("%s: calling ifdown for agent %p.\n",
> > +                                          __func__, agent);
> > +                               LIST_FOREACH(np, &lldp_head, lldp) {
> > +                                       ops = np->ops;
> > +                                       if (ops->lldp_mod_ifdown)
> > +                                               ops->lldp_mod_ifdown(device_name,
> > +                                                                    agent);
> > +                               }
> >                         }
> > 
> >                         /* Disable Port */
> > -                       set_lldp_port_enable_state(device_name, 0);
> > +                       set_lldp_port_enable(device_name, 0);
> > 
> >                         if (route_type == RTM_DELLINK) {
> >                                 LLDPAD_INFO("%s: %s: device removed!\n",
> 
> [...]
> 
> 
> >  #endif /* _LLDP_H */
> > diff --git a/include/lldp_8021qaz.h b/include/lldp_8021qaz.h
> > index 8f5cf56..0cf2ac1 100644
> > --- a/include/lldp_8021qaz.h
> > +++ b/include/lldp_8021qaz.h
> > @@ -228,11 +228,12 @@ int ieee8021qaz_check_active(const char *ifname);
> > 
> >  struct lldp_module *ieee8021qaz_register(void);
> >  void ieee8021qaz_unregister(struct lldp_module *mod);
> > -struct packed_tlv *ieee8021qaz_gettlv(struct port *port);
> > -int ieee8021qaz_rchange(struct port *port, struct unpacked_tlv *tlv);
> > -void ieee8021qaz_ifup(char *ifname);
> > -void ieee8021qaz_ifdown(char *ifname);
> > -u8 ieee8021qaz_mibDeleteObject(struct port *port);
> > +struct packed_tlv *ieee8021qaz_gettlv(struct port *port, struct lldp_agent *);
> > +int ieee8021qaz_rchange(struct port *port, struct lldp_agent *,
> > +                       struct unpacked_tlv *tlv);
> > +void ieee8021qaz_ifup(char *ifname, struct lldp_agent *);
> > +void ieee8021qaz_ifdown(char *ifname, struct lldp_agent *);
> 
> These should all be 'const char' I believe, not a fault of this patch though.
> I'll roll a follow up patch to constify things correctly.

I left these untouched for correction in a later patch.

> 
> [...]
> 
> 
> > diff --git a/include/lldp_mod.h b/include/lldp_mod.h
> > index 208bdd4..009734d 100644
> > --- a/include/lldp_mod.h
> > +++ b/include/lldp_mod.h
> > @@ -55,13 +55,14 @@
> >  struct lldp_mod_ops {
> >         struct lldp_module *    (* lldp_mod_register)(void);
> >         void                    (* lldp_mod_unregister)(struct lldp_module *);
> > -       struct packed_tlv *     (* lldp_mod_gettlv)(struct port *);
> > +       struct packed_tlv *     (* lldp_mod_gettlv)(struct port *, struct lldp_agent *);
> >         int                     (* lldp_mod_rchange)(struct port *,
> > +                                                    struct lldp_agent *,
> >                                                     struct unpacked_tlv *);
> >         void                    (* lldp_mod_utlv)(struct port *);
> > -       void                    (* lldp_mod_ifup)(char *);
> > -       void                    (* lldp_mod_ifdown)(char *);
> > -       u8                      (* lldp_mod_mibdelete)(struct port *port);
> > +       void                    (* lldp_mod_ifup)(char *, struct lldp_agent *);
> > +       void                    (* lldp_mod_ifdown)(char *, struct lldp_agent *);
> > +       u8                      (* lldp_mod_mibdelete)(struct port *port, struct lldp_agent *);
> >         u32                     (* client_register)(void);
> >         int                     (* client_cmd)(void *data,
> >                                               struct sockaddr_un *from,
> > @@ -70,7 +71,7 @@ struct lldp_mod_ops {
> >         int                     (* print_tlv)(u32, u16, char *);
> >         u32                     (* lookup_tlv_name)(char *);
> 
> Do print_tlv and lookup_tlv_name need to be per agent as well? Maybe in a follow up patch.

I did not find a reason in my testing yet, but this could also be easily introduced in a later patch.

> 
> >         int                     (* print_help)();
> > -       int                     (* timer)();
> > +       int                     (* timer)(struct port *, struct lldp_agent *);
> >         struct arg_handlers *   (* get_arg_handler)(void);
> >  };
> > 
> > @@ -95,7 +96,6 @@ struct lldp_module {
> >  LIST_HEAD(lldp_head, lldp_module);
> >  struct lldp_head lldp_head;
> > 
> > -
> >  static inline struct lldp_module *find_module_by_id(struct lldp_head *head, int id)
> >  {
> >         struct lldp_module *mod;
> > diff --git a/include/lldp_vdp.h b/include/lldp_vdp.h
> > index 2f6adf8..f9cdee2 100644
> 
> [...]
> 
> > diff --git a/include/tlv_dcbx.h b/include/tlv_dcbx.h
> > index d64d7b4..eb9ba38 100644
> > --- a/include/tlv_dcbx.h
> > +++ b/include/tlv_dcbx.h
> > @@ -241,13 +241,13 @@ struct unpacked_tlv *bld_dcbx2_app_tlv(struct dcbx_tlvs *, u32 sub_type,
> >  struct unpacked_tlv *bld_dcbx_llink_tlv(struct dcbx_tlvs *, u32 sub_type,
> >                                         bool *success);
> > 
> > -bool   unpack_dcbx1_tlvs(struct port *, struct unpacked_tlv *);
> > -bool   unpack_dcbx2_tlvs(struct port *, struct unpacked_tlv *);
> > -bool   process_dcbx_ctrl_tlv(struct port *);
> > -bool   process_dcbx_pg_tlv(struct port *);
> > -bool   process_dcbx_pfc_tlv(struct port *);
> > -bool   process_dcbx_app_tlv(struct port *, int);
> > -bool   process_dcbx_llink_tlv(struct port *);
> > +bool   unpack_dcbx1_tlvs(struct port *, struct lldp_agent *, struct unpacked_tlv *);
> > +bool   unpack_dcbx2_tlvs(struct port *, struct lldp_agent *, struct unpacked_tlv *);
> > +bool   process_dcbx_ctrl_tlv(struct port *, struct lldp_agent *);
> > +bool   process_dcbx_pg_tlv(struct port *, struct lldp_agent *);
> > +bool   process_dcbx_pfc_tlv(struct port *, struct lldp_agent *);
> > +bool   process_dcbx_app_tlv(struct port *, struct lldp_agent *, int);
> > +bool   process_dcbx_llink_tlv(struct port *, struct lldp_agent *);
> > 
> 
> I'm not sure how relevant multicast addresses besides 'nearest bridge' are
> to DCBX. Can we avoid adding the agent logic here for now? At least we need
> to verify that each ifname uses only enables DCBX on a single agent.

The problem is that the dcbx tlv code wants to access dupTlvs, dcbx_st and tooManyNghbrs
in the rx struct which has now moved into the agent struct. I do not know enough about the
dcbx module, but at least dupTlvs and dcbx_st look like they could move to a dcbx specific struct,
but tooManyNghbrs is from the LLDP standard and cannot be changed that easily.
Does DCBX really need access to this or can this be retrieved via some other method ?

> 
> [...]
> 
> 
> > +
> > +int lldp_add_agent(char *ifname, int type)
> > +{
> 
> Making these 'const char *ifname' would be good.
> 
> > +       int count;
> > +       struct port *port;
> > +       struct lldp_agent *agent, *newagent;
> > +
> > +       port = port_find_by_name(ifname);
> > +
> > +       if (port == NULL)
> > +               return -1;
> > +
> > +       /* check if lldp_agents for this if already exist */
> > +       LIST_FOREACH(agent, &port->agent_head, entry) {
> > +               if (agent->type != type)
> > +                       continue;
> > +               else
> > +                       return -1;
> > +       }
> > +
> > +       /* if not, create one and initialize it */
> > +       LLDPAD_DBG("%s(%i): creating new agent for port %s.\n", __func__,
> > +                  __LINE__, ifname);
> > +       newagent  = (struct lldp_agent *)malloc(sizeof(struct lldp_agent));
> > +       if (newagent == NULL) {
> > +               LLDPAD_DBG("%s(%i): creation of new agent failed !.\n",
> > +                          __func__,  __LINE__);
> > +               return -1;
> > +       }
> > +
> > +       lldp_init_agent(port, newagent, type);
> > +
> > +       if (get_config_setting_by_agent(ifname, newagent->type, ARG_ADMINSTATUS,
> > +                       (void *)&newagent->adminStatus, CONFIG_TYPE_INT))
> > +                       newagent->adminStatus = disabled;
> > +
> > +       LLDPAD_DBG("%s(%i): agent->adminStatus = %s (%i).\n", __func__,
> > +                  __LINE__, (newagent->adminStatus == disabled) ? "disabled" : "enabled",
> > +                  newagent->adminStatus);
> > +
> > +       LIST_INSERT_HEAD(&port->agent_head, newagent, entry);
> > +
> > +       count = 0;
> > +       LIST_FOREACH(agent, &port->agent_head, entry) {
> > +               count++;
> > +       }
> 
> Could probably count these in LIST_FOREACH above. If you think it is
> cleaner here though that is fine.

Done.

> 
> > +
> > +       LLDPAD_DBG("%s: %i agents on if %s.\n", __func__, count, port->ifname);
> > +
> > +       return 0;
> > +}
> > +
> > +int lldp_remove_agent(char *ifname, int type)
> > +{
> > +       struct lldp_agent *agent;
> > +
> > +       agent = lldp_agent_find_by_type(ifname, type);
> > +
> > +       if (agent == NULL)
> > +               return 0;
> > +
> > +       LIST_REMOVE(agent, entry);
> > +
> > +       free(agent);
> > +
> > +       return 0;
> > +}
> > 
> >  static void timer(void *eloop_data, void *user_ctx)
> >  {
> > -       struct port *port = porthead;
> >         struct lldp_module *n;
> > +       struct lldp_agent *agent;
> > +       struct port *port = porthead;
> > 
> >         while (port != NULL) {
> > -               update_tx_timers(port);
> > -               run_tx_timers_sm(port);
> > -               run_tx_sm(port);
> > -               run_rx_sm(port);
> > -               update_rx_timers(port);
> > +               /* execute rx and tx sm for all agents on a port */
> > +               LIST_FOREACH(agent, &port->agent_head, entry) {
> > +                       char macstring[30];
> > +                       mac2str(&agent->mac_addr[0], macstring, 29);
> > +
> > +                       update_tx_timers(agent);
> > +                       run_tx_timers_sm(port, agent);
> > +                       run_tx_sm(port, agent);
> > +                       run_rx_sm(port, agent);
> > +                       update_rx_timers(agent);
> > +
> >                 LIST_FOREACH(n, &lldp_head, lldp) {
>                   ^^^^^ -> 
> 
> Need another tab indent here.

Done.

> 
> >                         if (n->ops && n->ops->timer)
> > -                               n->ops->timer(port);
> > +                               n->ops->timer(port, agent);
> > +               }
> > +
> >                 }
> > -               if (port->timers.dormantDelay)
> > -                       port->timers.dormantDelay--;
> > +
> > +               if (port->dormantDelay)
> > +                       port->dormantDelay--;
> > +
> 
> Should dormantDelay be per agent? We can make this a follow up
> patch rather than make this patch even larger. I think enabling
> and disabling an agent would want a per agent instance of the
> dormantDelay. Any thoughts?

So far, only the dcbx and the qaz module use dormantDelay and I therefore think
it is safe to be left in port. Changing this would mean adding more agent 
dependencies to these 2 modules. Not sure how much effort this would be.

> 
> [...]
> 
> 
> > diff --git a/lldp/agent.h b/lldp/agent.h
> > index 96c6bef..27d2d1c 100644
> > --- a/lldp/agent.h
> > +++ b/lldp/agent.h
> > @@ -27,8 +27,145 @@
> >  #ifndef AGENT_H
> >  #define AGENT_H
> > 
> > -int start_lldp_agent(void);
> > -void stop_lldp_agent(void);
> > -void clean_lldp_agent(void);
> > +#include "lldp.h"
> > +#include "mibdata.h"
> > +
> > +#ifndef ETH_ALEN
> > +#define ETH_ALEN    6
> > +#endif
> > +#ifndef IFNAMSIZ
> > +#define IFNAMSIZ    16  /* must match MAX_DEVICE_NAME_LEN */
> > +#endif
> > +#ifndef ETH_P_ALL
> > +#define ETH_P_ALL   0x0003
> > +#endif
> 
> Why do we need ETH_xxx and IFNAMSIZ here? I'm sure I just missed it.
> Also is this done because in including the correct headers conflicts
> with something.

If we wanted to use this from /usr/include/linux/if.h we would get a lot of
collisions with other definitions.

> 
> > +
> > +enum agent_type {
> > +       NEAREST_BRIDGE = 0,
> > +       NEAREST_NONTPMR_BRIDGE,
> > +       NEAREST_CUSTOMER_BRIDGE,
> > +       AGENT_MAX,
> > +};
> > +
> > +/* IEEE 802.1AB-2009 - Table 7-1: group MAC addresses used by LLDP */
> > +static const u8 nearest_bridge[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x0e};
> > +static const u8 nearest_nontpmr_bridge[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x03};
> > +static const u8 nearest_customer_bridge[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x00};
> > +
> > +static const u8 groupmacs[AGENT_MAX][ETH_ALEN] = {
> > +       [NEAREST_BRIDGE] = {0x01,0x80,0xc2,0x00,0x00,0x0e},
> > +       [NEAREST_NONTPMR_BRIDGE] = {0x01,0x80,0xc2,0x00,0x00,0x03},
> > +       [NEAREST_CUSTOMER_BRIDGE] = {0x01,0x80,0xc2,0x00,0x00,0x00},
> > +};
> 
> Two constants for the same value?

Fixed.

> 
> > +
> > +struct agenttimers {
> > +/* Tx */
> > +       u16 state;
> > +       u16 reinitDelay;
> > +       u16 msgTxHold;
> > +       u16 msgTxInterval;
> > +       u16 msgFastTx;
> > +       u16 txFastInit;
> > +       u16 txTTR;
> > +       u16 txShutdownWhile;
> > +       u16 txCredit;
> > +       u16 txMaxCredit;
> > +       bool txTick;
> > +/* Rx */
> > +       u16 tooManyNghbrsTimer;
> > +       u16 rxTTL;
> > +       u16 lastrxTTL;  /* cache last received */
> > +};
> > +
> > +struct agenttx {
> > +       u8 *frameout;
> > +       u32 sizeout;
> > +       u8 state;
> > +       u8 localChange;
> > +       u16 txTTL;
> > +       bool txNow;
> > +       u16 txFast;
> > +};
> > +
> > +/* per agent statistical counter as in chapter 9.2.6
> > + * of IEEE 802.1AB-2009 */
> > +struct agentstats {
> > +/* Tx */
> > +       u32 statsFramesOutTotal;
> > +/* Rx */
> > +       u32 statsAgeoutsTotal;
> > +       u32 statsFramesDiscardedTotal;
> > +       u32 statsFramesInErrorsTotal;
> > +       u32 statsFramesInTotal;
> > +       u32 statsTLVsDiscardedTotal;
> > +       u32 statsTLVsUnrecognizedTotal;
> > +};
> > +
> > +typedef struct rxmanifest{
> > +       struct unpacked_tlv *chassis;
> > +       struct unpacked_tlv *portid;
> > +       struct unpacked_tlv *ttl;
> > +       struct unpacked_tlv *portdesc;
> > +       struct unpacked_tlv *sysname;
> > +       struct unpacked_tlv *sysdesc;
> > +       struct unpacked_tlv *syscap;
> > +       struct unpacked_tlv *mgmtadd;
> > +}rxmanifest;
>    ^^^
> 
> nit, should be a space between '}' and rxmanifest.

Fixed.

> 
> > +
> > +struct agentrx {
> > +       u8 *framein;
> > +       u16 sizein;
> > +       u8 state;
> > +       u8 badFrame;
> > +       u8 rcvFrame;
> > +       u8 rxInfoAge;
> > +       u8 remoteChange;
> > +       u8 tooManyNghbrs;
> > +       u8 dupTlvs;
> > +       u8 dcbx_st;
> > +       bool newNeighbor;
> > +       rxmanifest *manifest;
> > +};
> > +
> > +enum agentAdminStatus {
> > +       disabled,
> > +       enabledTxOnly,
> > +       enabledRxOnly,
> > +       enabledRxTx,
> > +};
> > +
> > +/* lldp agent specific structure as in chapter 9.2.5
> > + * of IEEE 802.1AB-2009 */
> > +struct lldp_agent {
> > +       int     adminStatus;
> > +
> > +       int     pad;
> 
> What is 'pad' here. Its not listed in 9.2.5 or is this to
> pad the struct for some reason?

It is to pad the struct, I saw a crash without it.

> 
> > +
> > +       u8      mac_addr[ETH_ALEN];
> > +
> > +       struct  agentrx rx;
> > +       struct  agenttx tx;
> > +       struct  agentstats stats;
> > +       struct  agenttimers timers;
> > +       u8      rxChanges;
> > +       u16     lldpdu;
> > +       struct  msap msap;
> > +
> > +       enum    agent_type type;
> > +
> > +        LIST_ENTRY(lldp_agent) entry;
> > +};
> > +
> > +struct lldp_agent *lldp_agent_find_by_type(const char *, int);
> > +int lldp_add_agent(char *ifname, int type);
>                       ^^^^^^^
> const char ifname

Done.

> 
> > +int lldp_remove_agent(char *ifname, int type);
> 			 ^^^^^
> const char ifname
> 
> 
> > +
> > +void set_lldp_agent_admin(const char *ifname, int type, int enable);
> > +int get_lldp_agent_admin(const char *ifname, int type);
> > +int get_lldp_agent_statistics(char *ifname, struct agentstats *, int);
> 			         ^^^^^^^^^^^^
> same 'const' comment throughout. Actually I believe these are just
> copied renamed routines. In that case don't worry about it here.
> We can post a patch on top to fixup that nit.

Done.

> 
> > +
> > +int start_lldp_agents(void);
> > +void stop_lldp_agents(void);
> > +void clean_lldp_agents(void);
> > 
> >  #endif /* AGENT_H */
> > diff --git a/lldp/l2_packet.h b/lldp/l2_packet.h
> > index 763b3f6..c1428ad 100644
> > --- a/lldp/l2_packet.h
> > +++ b/lldp/l2_packet.h
> > @@ -60,11 +60,6 @@
> >  #define ETH_MIN_PKT_LEN     (ETH_MIN_DATA_LEN + ETH_HDR_LEN)
> >  #endif
> > 
> > -#define LLDP_MULTICAST_MAC 0x0180c200000e
> > -
> > -
> > -static const u8 multi_cast_source[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x0e};
> > -
> >  /**
> >   * struct l2_packet_data - Internal l2_packet data structure
> >   *
> > @@ -129,7 +124,7 @@ int l2_packet_get_own_src_addr(struct l2_packet_data *l2, u8 *addr);
> >   */
> >  int l2_packet_get_own_addr(struct l2_packet_data *l2, u8 *addr);
> > 
> > -void get_remote_peer_mac_addr(struct port *port);
> > +void get_remote_peer_mac_addr(struct port *port, struct lldp_agent *);
> >  void l2_packet_get_remote_addr(struct l2_packet_data *l2, u8 *addr);
> > 
> 
> [...]
> 
> > 
> >  void set_port_oper_delay(const char *ifname)
> >  {
> > -       struct port *port = NULL;
> > -
> > -       port = porthead;
> > -       while (port != NULL) {
> > -               if (!strncmp(ifname, port->ifname, IFNAMSIZ))
> > -                       break;
> > -               port = port->next;
> > -       }
> > +       struct port *port = port_find_by_name(ifname);
> > 
> >         if (port == NULL)
> >                 return;
> > 
> > -       port->timers.dormantDelay = DORMANT_DELAY;
> > +       port->dormantDelay = DORMANT_DELAY;
> > +
> >         return;
> >  }
> > 
> > @@ -198,13 +201,7 @@ int set_port_hw_resetting(const char *ifname, int resetting)
> >  {
> >         struct port *port = NULL;
> > 
> > -       port = porthead;
> > -       while (port != NULL) {
> > -               if (!strncmp(ifname, port->ifname, IFNAMSIZ)) {
> > -                       break;
> > -               }
> > -               port = port->next;
> > -       }
> > +       port = port_find_by_name(ifname);
> > 
> 
> These optimizations could be there own patch.

Ok, I'll try to pull it out.

> 
> >         if (port == NULL)
> >                 return -1;
> > @@ -218,12 +215,7 @@ int get_port_hw_resetting(const char *ifname)
> >  {
> >         struct port *port = NULL;
> > 
> > -       port = porthead;
> > -       while (port != NULL) {
> > -               if (!strncmp(ifname, port->ifname, IFNAMSIZ))
> > -                       break;
> > -               port = port->next;
> > -       }
> > +       port = port_find_by_name(ifname);
> > 
> 
> same comment for all the rest.
> 
> >         if (port)
> >                 return port->hw_resetting;
> > @@ -233,43 +225,45 @@ int get_port_hw_resetting(const char *ifname)
> > 
> >  int reinit_port(const char *ifname)
> >  {
> > +       struct lldp_agent *agent;
> >         struct port *port;
> > 
> > -       port = porthead;
> > -       while (port != NULL) {
> > -               if (!strncmp(ifname, port->ifname, IFNAMSIZ))
> > -                       break;
> > -               port = port->next;
> > -       }
> > +       port = port_find_by_name(ifname);
> > 
> > -       if (!port)
> > +       if (port == NULL)
> 
> unnecessary change here.
> 
> >                 return -1;
> > 
> >         /* Reset relevant port variables */
> > -       port->tx.state  = TX_LLDP_INITIALIZE;
> > -       port->timers.state  = TX_TIMER_BEGIN;
> > -       port->rx.state = LLDP_WAIT_PORT_OPERATIONAL;
> >         port->hw_resetting = false;
> >         port->portEnabled = false;
> > -       port->tx.txTTL = 0;
> > -       port->msap.length1 = 0;
> > -       port->msap.msap1 = NULL;
> > -       port->msap.length2 = 0;
> > -       port->msap.msap2 = NULL;
> > -       port->lldpdu = false;
> > -       port->timers.dormantDelay = DORMANT_DELAY;
> > -
> > -       /* init & enable RX path */
> > -       rxInitializeLLDP(port);
> > -
> > -       /* init TX path */
> > -       txInitializeTimers(port);
> > -       txInitializeLLDP(port);
> > +       port->dormantDelay = DORMANT_DELAY;
> > +
> > +       LIST_FOREACH(agent, &port->agent_head, entry) {
> > +               /* init TX path */
> > +
> > +               /* Reset relevant state variables */
> > +               agent->tx.state  = TX_LLDP_INITIALIZE;
> > +               agent->rx.state = LLDP_WAIT_PORT_OPERATIONAL;
> > +               agent->tx.txTTL = 0;
> > +               agent->msap.length1 = 0;
> > +               agent->msap.msap1 = NULL;
> > +               agent->msap.length2 = 0;
> > +               agent->msap.msap2 = NULL;
> > +               agent->lldpdu = false;
> > +               agent->timers.state  = TX_TIMER_BEGIN;
> > +
> > +               /* init & enable RX path */
> > +               rxInitializeLLDP(port, agent);
> > +
> > +               /* init TX path */
> > +               txInitializeTimers(agent);
> > +               txInitializeLLDP(agent);
> > +       }
> > 
> >         return 0;
> >  }
> > 
> 
> [...]
> 
> > 
> > +/* lldp port specific structure */
> >  struct port {
> >         char *ifname;
> >         u8 hw_resetting;
> >         u8 portEnabled;
> >         u8 prevPortEnabled;
> > -       u8 adminStatus;
> > +       struct porttimers *timers;
> > 
> > -       /* protocol specific */
> > +       u16 dormantDelay;
> > +
> > +       LIST_HEAD(agent_head, lldp_agent) agent_head;
> >         struct l2_packet_data *l2;
> > -       struct portrx rx;
> > -       struct porttx tx;
> > -       struct portstats stats;
> > -       struct porttimers timers;
> > -       u8 rxChanges;
> > -       u16   lldpdu;
> > -       struct msap msap;
> > 
> >         struct port *next;
> >  };
> > 
> >  extern struct port *porthead;
> > -extern struct port *portcurrent;
> > -extern struct port *porttail;
> > 
> 
> [...]
> 
> > diff --git a/lldp_8021qaz.c b/lldp_8021qaz.c
> > index 2a6685b..0be1dad 100644
> > --- a/lldp_8021qaz.c
> > +++ b/lldp_8021qaz.c
> > @@ -51,8 +51,8 @@
> >  struct lldp_head lldp_head;
> >  struct config_t lldpad_cfg;
> > 
> > -static int ieee8021qaz_check_pending(struct port *port);
> > -static void run_all_sm(struct port *port);
> > +static int ieee8021qaz_check_pending(struct port *port, struct lldp_agent *);
> > +static void run_all_sm(struct port *port, struct lldp_agent *agent);
> >  static void ieee8021qaz_mibUpdateObjects(struct port *port);
> >  static void ieee8021qaz_app_reset(struct app_tlv_head *head);
> > 
> > @@ -68,7 +68,8 @@ static const struct lldp_mod_ops ieee8021qaz_ops = {
> >         .timer                  = ieee8021qaz_check_pending,
> >  };
> > 
> 
> Somewhere in 802.1Qaz there needs to be a check to only allow DCBX on
> on one agent per port. Additionally, 'nearest customer bridge' and
> 'nontpmr' should be disabled by default. This can be done with a follow
> up patch.

I already have an idea on how to do this. I will work on a followup patch for this.

> [Edit] Although it looks like 802.1qaz does nothing with the agent so
> this should be fine. Is this true?

Not sure right now, I need to look into this again.

> 
> > -static int ieee8021qaz_check_pending(struct port *port)
> > +static int ieee8021qaz_check_pending(struct port *port,
> > +                                    struct lldp_agent *agent)
> >  {
> >         struct ieee8021qaz_user_data *iud;
> >         struct ieee8021qaz_tlvs *tlv = NULL;
> > @@ -81,11 +82,12 @@ static int ieee8021qaz_check_pending(struct port *port)
> >                 LIST_FOREACH(tlv, &iud->head, entry) {
> >                         if (!strncmp(port->ifname, tlv->ifname, IFNAMSIZ)) {
> >                                 if (tlv->active && tlv->pending &&
> > -                                   port->timers.dormantDelay == 1) {
> > +                                   port->dormantDelay == 1) {
> >                                         tlv->pending = false;
> >                                         ieee8021qaz_app_reset(&tlv->app_head);
> > -                                       run_all_sm(port);
> > -                                       somethingChangedLocal(port->ifname);
> > +                                       run_all_sm(port, agent);
> > +                                       somethingChangedLocal(port->ifname,
> > +                                                             agent->type);
> >                                 }
> >                                 break;
> >                         }
> 
> > 
> > -void run_all_sm(struct port *port)
> > +void run_all_sm(struct port *port, struct lldp_agent *agent)
> >  {
> >         struct ieee8021qaz_tlvs *tlvs;
> >         struct ieee_ets *ets;
> > @@ -1004,7 +1020,8 @@ void run_all_sm(struct port *port)
> >         if (!tlvs)
> >                 return;
> > 
> > -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_ETSCFG))) {
> > +       if (!is_tlv_txdisabled(port->ifname,
> > +                              TLVID_8021(LLDP_8021QAZ_ETSCFG))) {
> 
> Why the new line wrapping? This can be done in a separate patch.

Ok, I'll try to pull out the unrelated changes.

> 
> 
> >                 ets_sm(tlvs->ets->cfgl, tlvs->ets->recr,
> >                        &tlvs->ets->current_state);
> >         }
> > @@ -1023,7 +1040,8 @@ void run_all_sm(struct port *port)
> >         else
> >                 ets_cfg_to_ieee(ets, tlvs->ets->cfgl);
> > 
> > -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_PFC)))
> > +       if (!is_tlv_txdisabled(port->ifname,
> > +                              TLVID_8021(LLDP_8021QAZ_PFC)))
> 
> same comment.
> 
> >                 pfc_sm(tlvs);
> > 
> >         if (tlvs->pfc->current_state == RX_RECOMMEND)
> > @@ -1281,7 +1299,8 @@ error:
> >   * ieee8021qaz_bld_tlv - builds all IEEE8021QAZ TLVs
> >   * Returns 1 on success, NULL if any of the TLVs fail to build correctly.
> >   */
> > -static struct packed_tlv *ieee8021qaz_bld_tlv(struct port *port)
> > +static struct packed_tlv *ieee8021qaz_bld_tlv(struct port *port,
> > +                                             struct lldp_agent *agent)
> >  {
> >         struct ieee8021qaz_tlvs *data;
> >         struct packed_tlv *ptlv = NULL;
> > @@ -1297,13 +1316,17 @@ static struct packed_tlv *ieee8021qaz_bld_tlv(struct port *port)
> >         if (!data->active)
> >                 return ptlv;
> > 
> > -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_ETSCFG)))
> > +       if (!is_tlv_txdisabled(port->ifname,
> > +                              TLVID_8021(LLDP_8021QAZ_ETSCFG)))
> >                 etscfg_tlv = bld_ieee8021qaz_etscfg_tlv(data);
> > -       if (is_tlv_txenabled(port->ifname, TLVID_8021(LLDP_8021QAZ_ETSREC)))
> > +       if (is_tlv_txenabled(port->ifname,
> > +                            TLVID_8021(LLDP_8021QAZ_ETSREC)))
> >                 etsrec_tlv = bld_ieee8021qaz_etsrec_tlv(data);
> > -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_PFC)))
> > +       if (!is_tlv_txdisabled(port->ifname,
> > +                              TLVID_8021(LLDP_8021QAZ_PFC)))
> >                 pfc_tlv = bld_ieee8021qaz_pfc_tlv(data);
> > -       if (is_tlv_txenabled(port->ifname, TLVID_8021(LLDP_8021QAZ_APP)))
> > +       if (is_tlv_txenabled(port->ifname,
> > +                            TLVID_8021(LLDP_8021QAZ_APP)))
> >                 app_tlv = bld_ieee8021qaz_app_tlv(port->ifname);
> > 
> 
> same comment on above changes.
> 
> >         size = TLVSIZE(etscfg_tlv)
> > @@ -1333,18 +1356,21 @@ err:
> >  }
> > 
> >  /* LLDP_8021QAZ_MOD_OPS - GETTLV */
> > -struct packed_tlv *ieee8021qaz_gettlv(struct port *port)
> > +struct packed_tlv *ieee8021qaz_gettlv(struct port *port,
> > +                                     struct lldp_agent *agent)
> >  {
> >         struct packed_tlv *ptlv = NULL;
> > 
> >         /* Update TLV State Machines */
> > -       run_all_sm(port);
> > +       run_all_sm(port, agent);
> >         /* Build TLVs */
> > -       ptlv = ieee8021qaz_bld_tlv(port);
> > +       ptlv = ieee8021qaz_bld_tlv(port, agent);
> >         return ptlv;
> >  }
> > 
> 
> [...]
> 
> 
> > diff --git a/lldp_dcbx.c b/lldp_dcbx.c
> > index 7949147..c2011ff 100644
> > --- a/lldp_dcbx.c
> > +++ b/lldp_dcbx.c
> > @@ -30,6 +30,8 @@
> >  #include "linux/if.h"
> >  #include "include/linux/dcbnl.h"
> >  #include "lldp.h"
> > +#include "lldp/ports.h"
> > +#include "lldp/states.h"
> >  #include "dcb_types.h"
> >  #include "lldp_dcbx.h"
> >  #include "dcb_protocol.h"
> > @@ -41,8 +43,6 @@
> >  #include "clif_msgs.h"
> >  #include "lldp_mod.h"
> >  #include "lldp_mand_clif.h"
> > -#include "lldp/ports.h"
> > -#include "lldp/states.h"
> >  #include "lldp_dcbx_nl.h"
> >  #include "lldp_dcbx_cfg.h"
> >  #include "lldp_dcbx_cmds.h"
> > @@ -55,11 +55,11 @@
> >  extern u8 gdcbx_subtype;
> > 
> >  void dcbx_free_tlv(struct dcbx_tlvs *tlvs);
> > -static int dcbx_check_operstate(struct port *port);
> > +static int dcbx_check_operstate(struct port *port, struct lldp_agent *agent);
> > 
> >  const struct lldp_mod_ops dcbx_ops = {
> > -       .lldp_mod_register      = dcbx_register,
> > -       .lldp_mod_unregister    = dcbx_unregister,
> > +       .lldp_mod_register      = dcbx_register,
> > +       .lldp_mod_unregister    = dcbx_unregister,
> 
> Some spacing fixes?

Yes, could be pulled out as well.

> 
> >         .lldp_mod_gettlv        = dcbx_gettlv,
> >         .lldp_mod_rchange       = dcbx_rchange,
> >         .lldp_mod_ifup          = dcbx_ifup,
> > @@ -70,7 +70,7 @@ const struct lldp_mod_ops dcbx_ops = {
> >         .timer                  = dcbx_check_operstate,
> >  };
> > 
> > -static int dcbx_check_operstate(struct port *port)
> > +static int dcbx_check_operstate(struct port *port, struct lldp_agent *agent)
> >  {
> >         int err;
> >         u8 app_good = 0;
> > @@ -78,7 +78,7 @@ static int dcbx_check_operstate(struct port *port)
> >         app_attribs app_data;
> >         pfc_attribs pfc_data;
> > 
> > -       if (!port->portEnabled || !port->timers.dormantDelay)
> > +       if (!port->portEnabled || !port->dormantDelay)
> >                 return 0;
> > 
> >         err = get_app(port->ifname, 0, &app_data);
> > @@ -95,10 +95,10 @@ static int dcbx_check_operstate(struct port *port)
> >             !app_data.protocol.Enable)
> >                 app_good = 1;
> > 
> > -       if ((pfc_good && app_good) || port->timers.dormantDelay == 1) {
> > +       if ((pfc_good && app_good) || port->dormantDelay == 1) {
> >                 LLDPAD_DBG("%s: %s: IF_OPER_UP delay, %u pfc oper %u"
> >                            "app oper %u\n",
> > -                       __func__, port->ifname, port->timers.dormantDelay,
> > +                       __func__, port->ifname, port->dormantDelay,
> >                         pfc_data.protocol.OperMode,
> >                         app_data.protocol.OperMode);
> >                 set_operstate(port->ifname, IF_OPER_UP);
> > @@ -122,7 +122,7 @@ struct dcbx_tlvs *dcbx_data(const char *ifname)
> >                                 return tlv;
> >                 }
> >         }
> > -
> > +
> >         return NULL;
> >  }
> > 
> > @@ -258,7 +258,7 @@ void dcbx_free_manifest(struct dcbx_manifest *manifest)
> >  {
> >         if (!manifest)
> >                 return;
> > -
> > +
> >         if (manifest->dcbx1)
> >                 manifest->dcbx1 = free_unpkd_tlv(manifest->dcbx1);
> >         if (manifest->dcbx2)
> > @@ -323,7 +323,7 @@ void dcbx_free_tlv(struct dcbx_tlvs *tlvs)
> >         return;
> >  }
> > 
> > -struct packed_tlv* dcbx_gettlv(struct port *port)
> > +struct packed_tlv* dcbx_gettlv(struct port *port, struct lldp_agent *agent)
> >  {
> >         struct packed_tlv *ptlv = NULL;
> >         struct dcbx_tlvs *tlvs;
> > @@ -453,7 +453,7 @@ void dcbx_unregister(struct lldp_module *mod)
> >         LLDPAD_DBG("%s: unregister dcbx complete.\n", __func__);
> >  }
> > 
> 
> [...]
> 
> The rest just looks like search and replace. This looks really good outside the
> few nits and questions above. The null check being the only real comment. Thanks
> for doing this Jens.


Thanks for reviewing ! I will resend the series soon.

Jens

-- 

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294



More information about the lldp-devel mailing list