netdev - Re: [PATCH net-next 3/5] net: 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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b8ac3558-b6f0-4658-b406-8ceba062a52c@lunn.ch>
Date: Mon, 23 Oct 2023 19:50:53 +0200
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>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH net-next 3/5] net: ipqess: introduce the Qualcomm IPQESS
 driver

> +/* locking is handled by the caller */
> +static int ipqess_edma_rx_buf_alloc_napi(struct ipqess_edma_rx_ring *rx_ring)
> +{
> +	struct ipqess_edma_buf *buf = &rx_ring->buf[rx_ring->head];
> +
> +	buf->skb = napi_alloc_skb(&rx_ring->napi_rx,
> +				  IPQESS_EDMA_RX_HEAD_BUFF_SIZE);

You might want to look at using the page_pool code. Its shown to be
more efficient for some drivers, e.g. the FEC.

> +static int ipqess_edma_redirect(struct ipqess_edma_rx_ring *rx_ring,
> +				struct sk_buff *skb, int port_id)
> +{
> +	struct ipqess_port *port;
> +
> +	if (port_id == 0) {
> +		/* The switch probably redirected an unknown frame to the CPU port
> +		 * (IGMP,BC,unknown MC, unknown UC)
> +		 */
> +		return -EINVAL;
> +	}
> +
> +	if (port_id < 0 || port_id > QCA8K_NUM_PORTS) {
> +		dev_warn(rx_ring->edma->sw->priv->dev,
> +			 "received packet tagged with out-of-bounds port id %d\n",
> +			 port_id);

Maybe rate limit this?

> +static int ipqess_port_set_mac_address(struct net_device *netdev, void *a)
> +{
> +	struct sockaddr *addr = a;
> +	int err;
> +
> +	if (!is_valid_ether_addr(addr->sa_data))
> +		return -EADDRNOTAVAIL;

I would be surprised if that could happen.

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

It looks like you can reuse dsa_slave_port_fdb_do_dump(), if you
export 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,
> +	};

And with a little bit of refactoring, you should be able to use the
core of qca8k_port_fdb_dump(). All that seems to differ is how you get
to the struct qca8k_priv *priv.

That then makes me wounder if there is more code here which could be
removed with a little refactoring of the DSA driver?

> +static void ipqess_port_get_drvinfo(struct net_device *dev,
> +				    struct ethtool_drvinfo *drvinfo)
> +{
> +	strscpy(drvinfo->driver, "qca8k-ipqess", sizeof(drvinfo->driver));
> +	strscpy(drvinfo->fw_version, "N/A", sizeof(drvinfo->fw_version));

If you leave this alone, it will contain the git hash of the kernel,
which is more useful than 'N/A'.

> +	strscpy(drvinfo->bus_info, "platform", sizeof(drvinfo->bus_info));
> +}
> +

> +static int ipqess_port_get_eeprom_len(struct net_device *dev)
> +{
> +	return 0;
> +}

Is this actually useful? What does it default to if not provided? 42?

> +static void ipqess_port_get_ethtool_stats(struct net_device *dev,
> +					  struct ethtool_stats *stats,
> +					  uint64_t *data)
> +{

...

> +	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)
> +			continue;

Given the switch is built in, is this fast? The 8k driver avoids doing
register reads for this.

> +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 reg;
> +
> +	/* Port's PHY and MAC both need to be EEE capable */
> +	if (!dev->phydev || !port->pl)
> +		return -ENODEV;
> +
> +	mutex_lock(&priv->reg_mutex);
> +	ret = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &reg);
> +	if (ret < 0) {
> +		mutex_unlock(&priv->reg_mutex);
> +		return ret;
> +	}
> +
> +	if (eee->eee_enabled)
> +		reg |= lpi_en;
> +	else
> +		reg &= ~lpi_en;
> +	ret = qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
> +	mutex_unlock(&priv->reg_mutex);

Everybody gets EEE wrong. The best example to copy is mvneta.

I also have a patchset which basically re-writes EEE in all the
drivers and moves as much as possible into the core. Those patches may
someday make it in. But until then, copy mvneta.

	Andrew