| Message ID | 1450286343-5514-1-git-send-email-zajec5@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Rafał, On Mittwoch, 16. Dezember 2015 18:19:01 Rafał Miłecki wrote: > So far we were sending link data as a string. It got some drawbacks: > 1) Didn't allow writing clean user space apps reading link state. It was > needed to do some screen scraping. > 2) Forced whole PORT_LINK communication to be string based. Adding > support for *setting* port link required passing string and parting > it in the kernel space. indeed, this interface looks a lot better than passing a string. It will make setting the link parameters easier. Minor nitpick: I'm not sure about the naming of these attributes: > + SWITCH_LINK_FLAG_ADVERTISED_100BASET_FULL, > + SWITCH_LINK_FLAG_ADVERTISED_1000BASET_FULL, As far as I understand the code, they specify whether EEE should be adverstised / is enabled for a specific speed. Therefore maybe they should be called f.e. SWITCH_LINK_EEE_100BASET and SWITCH_LINK_EEE_1000BASET Stefan
Le 16 déc. 2015 09:19, "Rafał Miłecki" <zajec5@gmail.com> a écrit : > > So far we were sending link data as a string. It got some drawbacks: > 1) Didn't allow writing clean user space apps reading link state. It was > needed to do some screen scraping. > 2) Forced whole PORT_LINK communication to be string based. Adding > support for *setting* port link required passing string and parting > it in the kernel space. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- [snip] (struct sk_buff *msg, struct genl_info *info, int attr, > + const struct switch_port_link *link) > +{ > + struct nlattr *p = NULL; > + int err = 0; > + > + p = nla_nest_start(msg, attr); > + if (nla_put_u32(msg, SWITCH_LINK_LINK, link->link)) Boolean? > + goto nla_put_failure; > + if (nla_put_u32(msg, SWITCH_LINK_DUPLEX, link->duplex)) Same here > + goto nla_put_failure; > + if (nla_put_u32(msg, SWITCH_LINK_ANEG, link->aneg)) And here > + goto nla_put_failure; > + if (nla_put_u32(msg, SWITCH_LINK_TX_FLOW, link->tx_flow)) And here > + goto nla_put_failure; > + if (nla_put_u32(msg, SWITCH_LINK_RX_FLOW, link->rx_flow)) And here too > + goto nla_put_failure; > + if (nla_put_u32(msg, SWITCH_LINK_SPEED, link->speed)) > + goto nla_put_failure; > + if (link->eee & ADVERTISED_100baseT_Full) { > + if (nla_put_flag(msg, SWITCH_LINK_FLAG_ADVERTISED_100BASET_FULL)) > + goto nla_put_failure; > + } > + if (link->eee & ADVERTISED_1000baseT_Full) { > + if (nla_put_flag(msg, SWITCH_LINK_FLAG_ADVERTISED_1000BASET_FULL)) > + goto nla_put_failure; > + } As mentioned by Stefan, these should reflect that is is EEE-related. -- Florian
On 20 December 2015 at 19:23, Stefan Rompf <stefan@loplof.de> wrote: > On Mittwoch, 16. Dezember 2015 18:19:01 Rafał Miłecki wrote: > >> So far we were sending link data as a string. It got some drawbacks: >> 1) Didn't allow writing clean user space apps reading link state. It was >> needed to do some screen scraping. >> 2) Forced whole PORT_LINK communication to be string based. Adding >> support for *setting* port link required passing string and parting >> it in the kernel space. > > indeed, this interface looks a lot better than passing a string. It will make > setting the link parameters easier. > > Minor nitpick: I'm not sure about the naming of these attributes: > >> + SWITCH_LINK_FLAG_ADVERTISED_100BASET_FULL, >> + SWITCH_LINK_FLAG_ADVERTISED_1000BASET_FULL, > > As far as I understand the code, they specify whether EEE should be > adverstised / is enabled for a specific speed. Therefore maybe they should be > called f.e. > > SWITCH_LINK_EEE_100BASET and > SWITCH_LINK_EEE_1000BASET In the current code swconfig_get_link does following: link.eee & ADVERTISED_100baseT_Full ? "eee100 " : "", link.eee & ADVERTISED_1000baseT_Full ? "eee1000 " : "", this is what made me call flags like that. I'll rename them.
diff --git a/target/linux/generic/files/drivers/net/phy/swconfig.c b/target/linux/generic/files/drivers/net/phy/swconfig.c index 6bb3be1..731bdcb 100644 --- a/target/linux/generic/files/drivers/net/phy/swconfig.c +++ b/target/linux/generic/files/drivers/net/phy/swconfig.c @@ -771,6 +771,43 @@ done: } static int +swconfig_send_link(struct sk_buff *msg, struct genl_info *info, int attr, + const struct switch_port_link *link) +{ + struct nlattr *p = NULL; + int err = 0; + + p = nla_nest_start(msg, attr); + if (nla_put_u32(msg, SWITCH_LINK_LINK, link->link)) + goto nla_put_failure; + if (nla_put_u32(msg, SWITCH_LINK_DUPLEX, link->duplex)) + goto nla_put_failure; + if (nla_put_u32(msg, SWITCH_LINK_ANEG, link->aneg)) + goto nla_put_failure; + if (nla_put_u32(msg, SWITCH_LINK_TX_FLOW, link->tx_flow)) + goto nla_put_failure; + if (nla_put_u32(msg, SWITCH_LINK_RX_FLOW, link->rx_flow)) + goto nla_put_failure; + if (nla_put_u32(msg, SWITCH_LINK_SPEED, link->speed)) + goto nla_put_failure; + if (link->eee & ADVERTISED_100baseT_Full) { + if (nla_put_flag(msg, SWITCH_LINK_FLAG_ADVERTISED_100BASET_FULL)) + goto nla_put_failure; + } + if (link->eee & ADVERTISED_1000baseT_Full) { + if (nla_put_flag(msg, SWITCH_LINK_FLAG_ADVERTISED_1000BASET_FULL)) + goto nla_put_failure; + } + nla_nest_end(msg, p); + + return err; + +nla_put_failure: + nla_nest_cancel(msg, p); + return -1; +} + +static int swconfig_get_attr(struct sk_buff *skb, struct genl_info *info) { struct genlmsghdr *hdr = nlmsg_data(info->nlhdr); @@ -794,6 +831,9 @@ swconfig_get_attr(struct sk_buff *skb, struct genl_info *info) val.value.ports = dev->portbuf; memset(dev->portbuf, 0, sizeof(struct switch_port) * dev->ports); + } else if (attr->type == SWITCH_TYPE_LINK) { + val.value.link = &dev->linkbuf; + memset(&dev->linkbuf, 0, sizeof(struct switch_port_link)); } err = attr->get(dev, attr, &val); @@ -824,6 +864,12 @@ swconfig_get_attr(struct sk_buff *skb, struct genl_info *info) if (err < 0) goto nla_put_failure; break; + case SWITCH_TYPE_LINK: + err = swconfig_send_link(msg, info, + SWITCH_ATTR_OP_VALUE_LINK, val.value.link); + if (err < 0) + goto nla_put_failure; + break; default: pr_debug("invalid type in attribute\n"); err = -EINVAL; diff --git a/target/linux/generic/files/include/linux/switch.h b/target/linux/generic/files/include/linux/switch.h index 4291364..eac35f9 100644 --- a/target/linux/generic/files/include/linux/switch.h +++ b/target/linux/generic/files/include/linux/switch.h @@ -122,6 +122,7 @@ struct switch_dev { struct mutex sw_mutex; struct switch_port *portbuf; struct switch_portmap *portmap; + struct switch_port_link linkbuf; char buf[128]; @@ -148,6 +149,7 @@ struct switch_val { const char *s; u32 i; struct switch_port *ports; + struct switch_port_link *link; } value; }; diff --git a/target/linux/generic/files/include/uapi/linux/switch.h b/target/linux/generic/files/include/uapi/linux/switch.h index a59b239..b599943 100644 --- a/target/linux/generic/files/include/uapi/linux/switch.h +++ b/target/linux/generic/files/include/uapi/linux/switch.h @@ -50,6 +50,7 @@ enum { SWITCH_ATTR_OP_VALUE_INT, SWITCH_ATTR_OP_VALUE_STR, SWITCH_ATTR_OP_VALUE_PORTS, + SWITCH_ATTR_OP_VALUE_LINK, SWITCH_ATTR_OP_DESCRIPTION, /* port lists */ SWITCH_ATTR_PORT, @@ -86,6 +87,7 @@ enum switch_val_type { SWITCH_TYPE_INT, SWITCH_TYPE_STRING, SWITCH_TYPE_PORTS, + SWITCH_TYPE_LINK, SWITCH_TYPE_NOVAL, }; @@ -97,6 +99,20 @@ enum { SWITCH_PORT_ATTR_MAX }; +/* link nested attributes */ +enum { + SWITCH_LINK_UNSPEC, + SWITCH_LINK_LINK, + SWITCH_LINK_DUPLEX, + SWITCH_LINK_ANEG, + SWITCH_LINK_TX_FLOW, + SWITCH_LINK_RX_FLOW, + SWITCH_LINK_SPEED, + SWITCH_LINK_FLAG_ADVERTISED_100BASET_FULL, + SWITCH_LINK_FLAG_ADVERTISED_1000BASET_FULL, + SWITCH_LINK_ATTR_MAX, +}; + #define SWITCH_ATTR_DEFAULTS_OFFSET 0x1000
So far we were sending link data as a string. It got some drawbacks: 1) Didn't allow writing clean user space apps reading link state. It was needed to do some screen scraping. 2) Forced whole PORT_LINK communication to be string based. Adding support for *setting* port link required passing string and parting it in the kernel space. Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- .../linux/generic/files/drivers/net/phy/swconfig.c | 46 ++++++++++++++++++++++ target/linux/generic/files/include/linux/switch.h | 2 + .../generic/files/include/uapi/linux/switch.h | 16 ++++++++ 3 files changed, 64 insertions(+)