linux-kernel - Re: [PATCH net-next v3 3/8] net: qualcomm: ipqess: introduce the Qualcomm IPQESS driver

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f6ce0c15-8b72-4568-8ba2-f0216db84ffd@lunn.ch>
Date:   Tue, 14 Nov 2023 20:10:44 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Romain Gantois <romain.gantois@...tlin.com>
Cc:     davem@...emloft.net, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        thomas.petazzoni@...tlin.com,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org,
        Vladimir Oltean <vladimir.oltean@....com>,
        Luka Perkov <luka.perkov@...tura.hr>,
        Robert Marko <robert.marko@...tura.hr>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>
Subject: Re: [PATCH net-next v3 3/8] net: qualcomm: ipqess: introduce the
 Qualcomm IPQESS driver

> +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