Message-ID: <20210806161030.52a7ae93@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 6 Aug 2021 16:10:30 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Joel Stanley <joel@....id.au>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Stafford Horne <shorne@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Anton Blanchard <anton@...abs.org>,
        Gabriel Somlo <gsomlo@...il.com>, David Shah <dave@....me>,
        Karol Gugala <kgugala@...micro.com>,
        Mateusz Holenko <mholenko@...micro.com>,
        devicetree@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] net: Add driver for LiteX's LiteETH network
 interface

On Fri,  6 Aug 2021 15:19:04 +0930 Joel Stanley wrote:
> LiteX is a soft system-on-chip that targets FPGAs. LiteETH is a basic
> network device that is commonly used in LiteX designs.
> 
> The driver was first written in 2017 and has been maintained by the
> LiteX community in various trees. Thank you to all who have contributed.

> +config NET_VENDOR_LITEX
> +	bool "LiteX devices"
> +	default y
> +	help
> +	  If you have a network (Ethernet) card belonging to this class, say Y.
> +
> +	  Note that the answer to this question doesn't directly affect the
> +	  kernel: saying N will just cause the configurator to skip all
> +	  the questions about LiteX devices. If you say Y, you will be asked
> +	  for your specific card in the following questions.

Maybe mention where the device is usually found (FPGAs) like you did in
the commit message, to help folks make a decision here?

> +config LITEX_LITEETH
> +	tristate "LiteX Ethernet support"
> +	help
> +	  If you wish to compile a kernel for hardware with a LiteX LiteEth
> +	  device then you should answer Y to this.

> +struct liteeth {
> +	void __iomem *base;
> +	void __iomem *mdio_base;
> +	struct net_device *netdev;
> +	struct device *dev;
> +	struct mii_bus *mii_bus;

unused field

> +
> +	/* Link management */
> +	int cur_duplex;
> +	int cur_speed;
> +
> +	/* Tx */
> +	int tx_slot;
> +	int num_tx_slots;
> +	void __iomem *tx_base;
> +
> +	/* Rx */
> +	int rx_slot;
> +	int num_rx_slots;
> +	void __iomem *rx_base;
> +};
> +
> +
> +static int liteeth_rx(struct net_device *netdev)
> +{
> +	struct liteeth *priv = netdev_priv(netdev);
> +	struct sk_buff *skb;
> +	unsigned char *data;
> +	u8 rx_slot;
> +	int len;
> +
> +	rx_slot = readb(priv->base + LITEETH_WRITER_SLOT);
> +	len = readl(priv->base + LITEETH_WRITER_LENGTH);
> +
> +	skb = netdev_alloc_skb(netdev, len + NET_IP_ALIGN);

netdev_alloc_skb_ip_align() ...

> +	if (!skb) {
> +		netdev_err(netdev, "couldn't get memory");

\n at the end? You can skip it but be consistent across messages

> +		netdev->stats.rx_dropped++;
> +		return NET_RX_DROP;
> +	}
> +
> +	/* Ensure alignemnt of the ip header within the skb */
> +	skb_reserve(skb, NET_IP_ALIGN);

... then skip this

> +	if (len == 0 || len > 2048)
> +		return NET_RX_DROP;

Should this be counted somehow?

> +	data = skb_put(skb, len);
> +	memcpy_fromio(data, priv->rx_base + rx_slot * LITEETH_BUFFER_SIZE, len);
> +	skb->protocol = eth_type_trans(skb, netdev);
> +
> +	netdev->stats.rx_packets++;
> +	netdev->stats.rx_bytes += len;
> +
> +	return netif_rx(skb);
> +}
> +
> +static irqreturn_t liteeth_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *netdev = dev_id;
> +	struct liteeth *priv = netdev_priv(netdev);
> +	u8 reg;
> +
> +	reg = readb(priv->base + LITEETH_READER_EV_PENDING);
> +	if (reg) {
> +		netdev->stats.tx_packets++;
> +		writeb(reg, priv->base + LITEETH_READER_EV_PENDING);
> +	}
> +
> +	reg = readb(priv->base + LITEETH_WRITER_EV_PENDING);
> +	if (reg) {
> +		liteeth_rx(netdev);
> +		writeb(reg, priv->base + LITEETH_WRITER_EV_PENDING);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int liteeth_open(struct net_device *netdev)
> +{
> +	struct liteeth *priv = netdev_priv(netdev);
> +	int err;
> +
> +	/* Clear pending events */
> +	writeb(1, priv->base + LITEETH_WRITER_EV_PENDING);
> +	writeb(1, priv->base + LITEETH_READER_EV_PENDING);
> +
> +	err = request_irq(netdev->irq, liteeth_interrupt, 0, netdev->name, netdev);
> +	if (err) {
> +		netdev_err(netdev, "failed to request irq %d\n", netdev->irq);
> +		return err;
> +	}
> +
> +	/* Enable IRQs */
> +	writeb(1, priv->base + LITEETH_WRITER_EV_ENABLE);
> +	writeb(1, priv->base + LITEETH_READER_EV_ENABLE);
> +
> +	/* TODO: Remove these once we have working mdio support */
> +	priv->cur_duplex = DUPLEX_FULL;
> +	priv->cur_speed = SPEED_100;

please remove the fields until they're actually used

> +	netif_carrier_on(netdev);
> +
> +	netif_start_queue(netdev);
> +
> +	return 0;
> +}
> +
> +static int liteeth_stop(struct net_device *netdev)
> +{
> +	struct liteeth *priv = netdev_priv(netdev);

carrier_off() for symmetry?

> +	netif_stop_queue(netdev);
> +
> +	writeb(0, priv->base + LITEETH_WRITER_EV_ENABLE);
> +	writeb(0, priv->base + LITEETH_READER_EV_ENABLE);
> +
> +	free_irq(netdev->irq, netdev);
> +	return 0;
> +}
> +
> +static int liteeth_start_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> +	struct liteeth *priv = netdev_priv(netdev);
> +	void __iomem *txbuffer;
> +	int ret;
> +	u8 val;
> +
> +	/* Reject oversize packets */
> +	if (unlikely(skb->len > MAX_PKT_SIZE)) {
> +		if (net_ratelimit())
> +			netdev_dbg(netdev, "tx packet too big\n");
> +		goto drop;
> +	}
> +
> +	txbuffer = priv->tx_base + priv->tx_slot * LITEETH_BUFFER_SIZE;
> +	memcpy_toio(txbuffer, skb->data, skb->len);
> +	writeb(priv->tx_slot, priv->base + LITEETH_READER_SLOT);
> +	writew(skb->len, priv->base + LITEETH_READER_LENGTH);
> +
> +	ret = readl_poll_timeout_atomic(priv->base + LITEETH_READER_READY, val, val, 5, 1000);

Why the need for poll if there is an interrupt?
Why not stop the Tx queue once you're out of slots and restart 
it when the completion interrupt comes?

> +	if (ret == -ETIMEDOUT) {
> +		netdev_err(netdev, "LITEETH_READER_READY timed out\n");

ratelimit this as well, please

> +		goto drop;
> +	}
> +
> +	writeb(1, priv->base + LITEETH_READER_START);
> +
> +	netdev->stats.tx_bytes += skb->len;

Please count bytes and packets in the same place

> +	priv->tx_slot = (priv->tx_slot + 1) % priv->num_tx_slots;
> +	dev_kfree_skb_any(skb);
> +	return NETDEV_TX_OK;
> +drop:
> +	/* Drop the packet */
> +	dev_kfree_skb_any(skb);
> +	netdev->stats.tx_dropped++;
> +
> +	return NETDEV_TX_OK;
> +}

> +static int liteeth_probe(struct platform_device *pdev)
> +{
> +	struct net_device *netdev;
> +	void __iomem *buf_base;
> +	struct resource *res;
> +	struct liteeth *priv;
> +	int irq, err;
> +
> +	netdev = alloc_etherdev(sizeof(*priv));
> +	if (!netdev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(netdev);
> +	priv->netdev = netdev;
> +	priv->dev = &pdev->dev;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get IRQ\n");
> +		goto err;

`err` variable is not set here, you'd return 0

> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->base)) {
> +		err = PTR_ERR(priv->base);
> +		goto err;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	priv->mdio_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->mdio_base)) {
> +		err = PTR_ERR(priv->mdio_base);
> +		goto err;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	buf_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(buf_base)) {
> +		err = PTR_ERR(buf_base);
> +		goto err;
> +	}
> +
> +	err = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth",
> +			&priv->num_rx_slots);

Please run checkpatch --strict and fix what it points out

> +	if (err) {
> +		dev_err(&pdev->dev, "unable to get rx-fifo-depth\n");
> +		goto err;
> +	}

> +	return 0;
> +err:
> +	free_netdev(netdev);
> +	return err;
> +}

Powered by blists - more mailing lists