Message ID | 20231114105600.1012056-1-romain.gantois@bootlin.com |
---|---|
Headers | show |
Series | net: qualcomm: ipqess: introduce Qualcomm IPQESS driver | expand |
On Tue, Nov 14, 2023 at 11:55:50AM +0100, Romain Gantois wrote: > Hello everyone, > > This is the 3rd iteration on the Qualcomm IPQ4019 Ethernet Switch Subsystem > driver. I made some patch separation mistakes in the v2, sorry about that. > > Notable changes in v3: > - Fixed formatting of 3/8. Please wait at least 24 hours between submitted versions. Otherwise you get people reviewing the wrong version. Andrew
> +static void ipqess_port_stp_state_set(struct ipqess_port *port, > + u8 state) > +{ > + struct qca8k_priv *priv = port->sw->priv; > + u32 stp_state; > + int err; > + > + switch (state) { > + case BR_STATE_DISABLED: > + stp_state = QCA8K_PORT_LOOKUP_STATE_DISABLED; > + break; > + case BR_STATE_BLOCKING: > + stp_state = QCA8K_PORT_LOOKUP_STATE_BLOCKING; > + break; > + case BR_STATE_LISTENING: > + stp_state = QCA8K_PORT_LOOKUP_STATE_LISTENING; > + break; > + case BR_STATE_LEARNING: > + stp_state = QCA8K_PORT_LOOKUP_STATE_LEARNING; > + break; > + case BR_STATE_FORWARDING: > + default: > + stp_state = QCA8K_PORT_LOOKUP_STATE_FORWARD; > + break; > + } > + > + err = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port->index), > + QCA8K_PORT_LOOKUP_STATE_MASK, stp_state); When i compare this to qca8k_port_stp_state_set() it is 90% identical. What differs is how you get to struct qca8k_priv *priv. What you need to do is refactor the existing functions to separate the DSA parts out and have a core function which takes qca8k_priv and in port. The DSA core can then call it, and this function can call it, after extracting qca8k_priv and index from port. > +static int ipqess_port_enable_rt(struct ipqess_port *port, > + struct phy_device *phy) > +{ > + struct qca8k_priv *priv = port->sw->priv; > + > + qca8k_port_set_status(priv, port->index, 1); > + priv->port_enabled_map |= BIT(port->index); > + > + phy_support_asym_pause(phy); > + > + ipqess_port_set_state_now(port, BR_STATE_FORWARDING, false); > + > + if (port->pl) > + phylink_start(port->pl); That looks odd. You unconditionally call phy_support_asym_pause() yet conditionally call phylink_start(). I would expect there to always be a phylink instance. Also, you should be telling phylink about the pause capabilities in config->mac_capabilities. It is then phylinks problem to tell the PHY, or the PCS driving the SFP etc about pause. > +static int > +ipqess_port_fdb_do_dump(const unsigned char *addr, u16 vid, > + bool is_static, void *data) > +{ > + struct ipqess_port_dump_ctx *dump = data; > + u32 portid = NETLINK_CB(dump->cb->skb).portid; > + u32 seq = dump->cb->nlh->nlmsg_seq; > + struct nlmsghdr *nlh; > + struct ndmsg *ndm; > + > + if (dump->idx < dump->cb->args[2]) > + goto skip; > + > + nlh = nlmsg_put(dump->skb, portid, seq, RTM_NEWNEIGH, > + sizeof(*ndm), NLM_F_MULTI); > + if (!nlh) > + return -EMSGSIZE; > + > + ndm = nlmsg_data(nlh); > + ndm->ndm_family = AF_BRIDGE; > + ndm->ndm_pad1 = 0; > + ndm->ndm_pad2 = 0; > + ndm->ndm_flags = NTF_SELF; > + ndm->ndm_type = 0; > + ndm->ndm_ifindex = dump->dev->ifindex; > + ndm->ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE; > + > + if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, addr)) > + goto nla_put_failure; > + > + if (vid && nla_put_u16(dump->skb, NDA_VLAN, vid)) > + goto nla_put_failure; > + > + nlmsg_end(dump->skb, nlh); > + > +skip: > + dump->idx++; > + return 0; > + > +nla_put_failure: > + nlmsg_cancel(dump->skb, nlh); > + return -EMSGSIZE; > +} This looks identical to dsa_slave_port_fdb_do_dump(). Please export and reuse it. > + > +static int > +ipqess_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb, > + struct net_device *dev, struct net_device *filter_dev, > + int *idx) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + struct qca8k_priv *priv = port->sw->priv; > + struct ipqess_port_dump_ctx dump = { > + .dev = dev, > + .skb = skb, > + .cb = cb, > + .idx = *idx, > + }; > + int cnt = QCA8K_NUM_FDB_RECORDS; > + struct qca8k_fdb _fdb = { 0 }; > + bool is_static; > + int ret = 0; > + > + mutex_lock(&priv->reg_mutex); > + while (cnt-- && !qca8k_fdb_next(priv, &_fdb, port->index)) { > + if (!_fdb.aging) > + break; > + is_static = (_fdb.aging == QCA8K_ATU_STATUS_STATIC); > + ret = ipqess_port_fdb_do_dump(_fdb.mac, _fdb.vid, is_static, &dump); > + if (ret) > + break; > + } > + mutex_unlock(&priv->reg_mutex); > + > + *idx = dump.idx; > + > + return ret; > +} And with a little bit of refactoring you can reuse the core of qca8k_port_fdb_dump. > +static void ipqess_phylink_mac_link_up(struct phylink_config *config, > + struct phy_device *phydev, > + unsigned int mode, > + phy_interface_t interface, > + int speed, int duplex, > + bool tx_pause, bool rx_pause) > +{ > + struct ipqess_port *port = ipqess_port_from_pl_state(config, pl_config); > + struct qca8k_priv *priv = port->sw->priv; > + u32 reg; > + > + if (phylink_autoneg_inband(mode)) { > + reg = QCA8K_PORT_STATUS_LINK_AUTO; > + } else { > + switch (speed) { > + case SPEED_10: > + reg = QCA8K_PORT_STATUS_SPEED_10; > + break; > + case SPEED_100: > + reg = QCA8K_PORT_STATUS_SPEED_100; > + break; > + case SPEED_1000: > + reg = QCA8K_PORT_STATUS_SPEED_1000; > + break; > + default: > + reg = QCA8K_PORT_STATUS_LINK_AUTO; > + break; > + } > + > + if (duplex == DUPLEX_FULL) > + reg |= QCA8K_PORT_STATUS_DUPLEX; > + > + if (rx_pause || port->index == 0) > + reg |= QCA8K_PORT_STATUS_RXFLOW; > + > + if (tx_pause || port->index == 0) > + reg |= QCA8K_PORT_STATUS_TXFLOW; > + } > + > + reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC; > + > + qca8k_write(priv, QCA8K_REG_PORT_STATUS(port->index), reg); > +} qca8k_phylink_mac_link_up() with some refactoring can be reused. Please look through the driver and find other instances like this where you can reuse more code. Andrew
On 14.11.2023 11:55, Romain Gantois wrote: > The IPQESS driver registers one netdevice for each front-facing switch > port. Add support for several ethtool operations to these netdevices. > > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> > --- > drivers/net/ethernet/qualcomm/ipqess/Makefile | 2 +- > .../ethernet/qualcomm/ipqess/ipqess_ethtool.c | 245 ++++++++++++++++++ > .../ethernet/qualcomm/ipqess/ipqess_port.c | 1 + > .../ethernet/qualcomm/ipqess/ipqess_port.h | 3 + > 4 files changed, 250 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c > > diff --git a/drivers/net/ethernet/qualcomm/ipqess/Makefile b/drivers/net/ethernet/qualcomm/ipqess/Makefile > index f389080cc5aa..6253f1b0ffd2 100644 > --- a/drivers/net/ethernet/qualcomm/ipqess/Makefile > +++ b/drivers/net/ethernet/qualcomm/ipqess/Makefile > @@ -5,4 +5,4 @@ > > obj-$(CONFIG_QCOM_IPQ4019_ESS) += ipqess.o > > -ipqess-objs := ipqess_port.o ipqess_switch.o ipqess_edma.o > +ipqess-objs := ipqess_port.o ipqess_switch.o ipqess_edma.o ipqess_ethtool.o > diff --git a/drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c b/drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c > new file mode 100644 > index 000000000000..06a8f2cccc4e > --- /dev/null > +++ b/drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c > @@ -0,0 +1,245 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Ethtool operations for a single switch port > + * > + * Copyright (c) 2023, Romain Gantois <romain.gantois@bootlin.com> > + * Based on net/dsa > + */ > + > +#include <net/selftests.h> > + > +#include "ipqess_port.h" > + > +static void ipqess_port_get_drvinfo(struct net_device *dev, > + struct ethtool_drvinfo *drvinfo) > +{ > + strscpy(drvinfo->driver, "qca8k-ipqess", sizeof(drvinfo->driver)); > + strscpy(drvinfo->bus_info, "platform", sizeof(drvinfo->bus_info)); > +} > + > +static int ipqess_port_nway_reset(struct net_device *dev) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + return phylink_ethtool_nway_reset(port->pl); > +} > + > +static const char ipqess_gstrings_base_stats[][ETH_GSTRING_LEN] = { > + "tx_packets", > + "tx_bytes", > + "rx_packets", > + "rx_bytes", > +}; > + > +static void ipqess_port_get_strings(struct net_device *dev, > + u32 stringset, u8 *data) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + struct qca8k_priv *priv = port->sw->priv; > + int i; > + > + if (stringset == ETH_SS_STATS) { > + memcpy(data, &ipqess_gstrings_base_stats, > + sizeof(ipqess_gstrings_base_stats)); > + > + if (stringset != ETH_SS_STATS) > + return; > + > + for (i = 0; i < priv->info->mib_count; i++) > + memcpy(data + (4 + i) * ETH_GSTRING_LEN, > + ar8327_mib[i].name, > + ETH_GSTRING_LEN); > + > + } else if (stringset == ETH_SS_TEST) { > + net_selftest_get_strings(data); > + } > +} > + > +static void ipqess_port_get_ethtool_stats(struct net_device *dev, > + struct ethtool_stats *stats, > + uint64_t *data) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + struct qca8k_priv *priv = port->sw->priv; > + const struct qca8k_mib_desc *mib; > + struct pcpu_sw_netstats *s; > + unsigned int start; > + u32 reg, c, val; > + u32 hi = 0; > + int ret; > + int i; > + > + for_each_possible_cpu(i) { > + u64 tx_packets, tx_bytes, rx_packets, rx_bytes; > + > + s = per_cpu_ptr(dev->tstats, i); > + do { > + start = u64_stats_fetch_begin(&s->syncp); > + tx_packets = u64_stats_read(&s->tx_packets); > + tx_bytes = u64_stats_read(&s->tx_bytes); > + rx_packets = u64_stats_read(&s->rx_packets); > + rx_bytes = u64_stats_read(&s->rx_bytes); > + } while (u64_stats_fetch_retry(&s->syncp, start)); > + data[0] += tx_packets; > + data[1] += tx_bytes; > + data[2] += rx_packets; > + data[3] += rx_bytes; > + } > + > + for (c = 0; c < priv->info->mib_count; c++) { > + mib = &ar8327_mib[c]; > + reg = QCA8K_PORT_MIB_COUNTER(port->index) + mib->offset; > + > + ret = qca8k_read(priv, reg, &val); > + if (ret < 0) if (ret) > + continue; > + > + if (mib->size == 2) { > + ret = qca8k_read(priv, reg + 4, &hi); > + if (ret < 0) same > + continue; > + } > + > + data[4 + c] = val; > + if (mib->size == 2) > + data[4 + c] |= (u64)hi << 32; > + } > +} > + > +static int ipqess_port_get_sset_count(struct net_device *dev, int sset) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + struct qca8k_priv *priv = port->sw->priv; > + > + if (sset == ETH_SS_STATS) { > + int count = 0; > + > + if (sset != ETH_SS_STATS) > + count = 0; > + else > + count = priv->info->mib_count; > + > + if (count < 0) > + return count; > + > + return count + 4; > + } else if (sset == ETH_SS_TEST) { > + return net_selftest_get_count(); > + } > + > + return -EOPNOTSUPP; > +} > + > +static int ipqess_port_set_wol(struct net_device *dev, > + struct ethtool_wolinfo *w) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + return phylink_ethtool_set_wol(port->pl, w); > +} > + > +static void ipqess_port_get_wol(struct net_device *dev, > + struct ethtool_wolinfo *w) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + phylink_ethtool_get_wol(port->pl, w); > +} > + > +static int ipqess_port_set_eee(struct net_device *dev, struct ethtool_eee *eee) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + int ret; > + u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port->index); > + struct qca8k_priv *priv = port->sw->priv; > + u32 lpi_ctl1; RCT > + > + /* Port's PHY and MAC both need to be EEE capable */ > + if (!dev->phydev || !port->pl) > + return -ENODEV; > + > + mutex_lock(&priv->reg_mutex); > + lpi_ctl1 = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &lpi_ctl1); > + if (lpi_ctl1 < 0) { if (lpi_ctl1) > + mutex_unlock(&priv->reg_mutex); > + return ret; ret is not initialized at this point > + } > + > + if (eee->tx_lpi_enabled && eee->eee_enabled) > + lpi_ctl1 |= lpi_en; > + else > + lpi_ctl1 &= ~lpi_en; > + ret = qca8k_write(priv, QCA8K_REG_EEE_CTRL, lpi_ctl1); > + mutex_unlock(&priv->reg_mutex); > + > + if (ret) > + return ret; > + > + return phylink_ethtool_set_eee(port->pl, eee); > +} > + > +static int ipqess_port_get_eee(struct net_device *dev, struct ethtool_eee *e) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + /* Port's PHY and MAC both need to be EEE capable */ > + if (!dev->phydev || !port->pl) > + return -ENODEV; > + > + return phylink_ethtool_get_eee(port->pl, e); > +} > + > +static int ipqess_port_get_link_ksettings(struct net_device *dev, > + struct ethtool_link_ksettings *cmd) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + return phylink_ethtool_ksettings_get(port->pl, cmd); > +} > + > +static int ipqess_port_set_link_ksettings(struct net_device *dev, > + const struct ethtool_link_ksettings *cmd) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + return phylink_ethtool_ksettings_set(port->pl, cmd); > +} > + > +static void ipqess_port_get_pauseparam(struct net_device *dev, > + struct ethtool_pauseparam *pause) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + phylink_ethtool_get_pauseparam(port->pl, pause); > +} > + > +static int ipqess_port_set_pauseparam(struct net_device *dev, > + struct ethtool_pauseparam *pause) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + > + return phylink_ethtool_set_pauseparam(port->pl, pause); > +} > + > +static const struct ethtool_ops ipqess_port_ethtool_ops = { > + .get_drvinfo = ipqess_port_get_drvinfo, > + .nway_reset = ipqess_port_nway_reset, > + .get_link = ethtool_op_get_link, > + .get_strings = ipqess_port_get_strings, > + .get_ethtool_stats = ipqess_port_get_ethtool_stats, > + .get_sset_count = ipqess_port_get_sset_count, > + .self_test = net_selftest, > + .set_wol = ipqess_port_set_wol, > + .get_wol = ipqess_port_get_wol, > + .set_eee = ipqess_port_set_eee, > + .get_eee = ipqess_port_get_eee, > + .get_link_ksettings = ipqess_port_get_link_ksettings, > + .set_link_ksettings = ipqess_port_set_link_ksettings, > + .get_pauseparam = ipqess_port_get_pauseparam, > + .set_pauseparam = ipqess_port_set_pauseparam, > +}; > + > +void ipqess_port_set_ethtool_ops(struct net_device *netdev) > +{ > + netdev->ethtool_ops = &ipqess_port_ethtool_ops; > +} > diff --git a/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.c b/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.c > index f0f5fe3a7c24..52d7baa7cae0 100644 > --- a/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.c > +++ b/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.c > @@ -684,6 +684,7 @@ int ipqess_port_register(struct ipqess_switch *sw, > netdev->dev.of_node = port->dn; > > netdev->rtnl_link_ops = &ipqess_port_link_ops; > + ipqess_port_set_ethtool_ops(netdev); > > netdev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats); > if (!netdev->tstats) { > diff --git a/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.h b/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.h > index 26bac45dd811..19d4b5d73625 100644 > --- a/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.h > +++ b/drivers/net/ethernet/qualcomm/ipqess/ipqess_port.h > @@ -59,4 +59,7 @@ int ipqess_port_register(struct ipqess_switch *sw, > struct device_node *port_node); > void ipqess_port_unregister(struct ipqess_port *port); > > +/* Defined in ipqess_ethtool.c */ > +void ipqess_port_set_ethtool_ops(struct net_device *netdev); > + > #endif
On Wed, 15 Nov 2023, Wojciech Drewek wrote: ... > > +static int ipqess_port_vlan_rx_add_vid(struct net_device *dev, __be16 proto, > > + u16 vid) > > +{ > > + struct ipqess_port *port = netdev_priv(dev); > > + struct switchdev_obj_port_vlan vlan = { > > + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, > > + .vid = vid, > > + /* This API only allows programming tagged, non-PVID VIDs */ > > + .flags = 0, > > + }; > > + struct netlink_ext_ack extack = {0}; > > + int ret; > > + > > + /* User port... */ > > + ret = ipqess_port_do_vlan_add(port->sw->priv, port->index, &vlan, &extack); > > + if (ret) { > > + if (extack._msg) > > + netdev_err(dev, "%s\n", extack._msg); > > + return ret; > > + } > > + > > + /* And CPU port... */ > > + ret = ipqess_port_do_vlan_add(port->sw->priv, 0, &vlan, &extack); > > + if (ret) { > > Should we delete vlan from user port if this fails? I'll have to look into how and when this API is called in more detail but I think this would indeed make sense. > > + > > + /* Flush the FDB table */ > > + qca8k_fdb_flush(priv); > > + > > + if (ret < 0) > > + goto devlink_free; > > + > > + /* set Port0 status */ > > + reg = QCA8K_PORT_STATUS_LINK_AUTO; > > + reg |= QCA8K_PORT_STATUS_DUPLEX; > > + reg |= QCA8K_PORT_STATUS_SPEED_1000; > > + reg |= QCA8K_PORT_STATUS_RXFLOW; > > + reg |= QCA8K_PORT_STATUS_TXFLOW; > > + reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC; > > + qca8k_write(priv, QCA8K_REG_PORT_STATUS(0), reg); > > + sw->port0_enabled = true; > > + > > + return 0; > > + > > +devlink_free: > > Why is it called devlink_free, I don't see any connection to devlink. I think this is leftover from a previous version of this function, where it interacted with devlink. I'll rename it to error. Best,
On Tue, Nov 14, 2023 at 11:55:54AM +0100, Romain Gantois wrote: > The IPQESS driver registers one netdevice for each front-facing switch > port. Add support for several ethtool operations to these netdevices. > > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> Hi Romain, some more minor feedback from my side. > --- > drivers/net/ethernet/qualcomm/ipqess/Makefile | 2 +- > .../ethernet/qualcomm/ipqess/ipqess_ethtool.c | 245 ++++++++++++++++++ > .../ethernet/qualcomm/ipqess/ipqess_port.c | 1 + > .../ethernet/qualcomm/ipqess/ipqess_port.h | 3 + > 4 files changed, 250 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c ... > diff --git a/drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c b/drivers/net/ethernet/qualcomm/ipqess/ipqess_ethtool.c ... > +static int ipqess_port_set_eee(struct net_device *dev, struct ethtool_eee *eee) > +{ > + struct ipqess_port *port = netdev_priv(dev); > + int ret; > + u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port->index); > + struct qca8k_priv *priv = port->sw->priv; > + u32 lpi_ctl1; nit: Please consider using reverse xmas tree - longest line to shortest - for local variable declarations in networking code. > + > + /* Port's PHY and MAC both need to be EEE capable */ > + if (!dev->phydev || !port->pl) > + return -ENODEV; > + > + mutex_lock(&priv->reg_mutex); > + lpi_ctl1 = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &lpi_ctl1); > + if (lpi_ctl1 < 0) { lpi_ctl1 is unsigned, it can never be less than zero. As flagged by Smatch and Coccinelle. I think this should probably be (completely untested!): ret = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &lpi_ctl1); if (ret < 0) { Which would also resolve the issue immediately below too. > + mutex_unlock(&priv->reg_mutex); > + return ret; It seems that ret is used uninitialised here. Flagged by clang-16 W=1 builds. > + } ...