Re: [PATCH v2 2/2] net: Add driver for LiteX's LiteEth network interface

On Tue, 24 Aug 2021 at 19:43, Gabriel L. Somlo [off-list ref] wrote:

Hi Joel,

Couple of comments below:

On Fri, Aug 20, 2021 at 05:17:26PM +0930, Joel Stanley wrote:

quoted

diff --git a/drivers/net/ethernet/litex/Kconfig b/drivers/net/ethernet/litex/Kconfig
new file mode 100644
index 000000000000..265dba414b41
--- /dev/null
+++ b/drivers/net/ethernet/litex/Kconfig

quoted

+
+config LITEX_LITEETH
+     tristate "LiteX Ethernet support"

Mostly cosmetic, but should there be a "depends on LITEX" statement in here?

No, there's as there is no dependency on the litex soc driver.

Maybe also "select MII" and "select PHYLIB"?

Again, there is no mii or phy code so the driver doesn't need these.

quoted

diff --git a/drivers/net/ethernet/litex/Makefile b/drivers/net/ethernet/litex/Makefile
new file mode 100644
index 000000000000..9343b73b8e49
--- /dev/null
+++ b/drivers/net/ethernet/litex/Makefile
+int liteeth_setup_slots(struct liteeth *priv)
+{
+     struct device_node *np = priv->dev->of_node;
+     int err, depth;
+
+     err = of_property_read_u32(np, "rx-fifo-depth", &depth);
+     if (err) {
+             dev_err(priv->dev, "unable to get rx-fifo-depth\n");
+             return err;
+     }
+     if (depth < LITEETH_BUFFER_SIZE) {

If I set depth to be *equal* to LITEETH_BUFFER_SIZE (2048) in DTS,
no traffic makes it out of my network interface (linux-on-litex-rocket
on an ecpix5 board, see github.com/litex-hub/linux-on-litex-rocket).

May I suggest rejecting if (depth / LITEETH_BUFFER_SIZE < 2) instead?
When that's enforced, the interface actually works fine for me.

Yes, I was using BUFFER_SIZE as the slot size, which it is not. I'll
rework it to use the slot size I think.

I spent some time digging through the migen source and I couldn't work
out where the 1024 length comes from. If anything it should be
eth_mtu, which is 1530.

Florent, can you clear that up?

quoted

+             dev_err(priv->dev, "invalid tx-fifo-depth: %d\n", depth);

This should read "rx-fifo-depth".

Thanks.

quoted

+             return -EINVAL;
+     }
+     priv->num_rx_slots = depth / LITEETH_BUFFER_SIZE;
+
+     err = of_property_read_u32(np, "tx-fifo-depth", &depth);
+     if (err) {
+             dev_err(priv->dev, "unable to get tx-fifo-depth\n");
+             return err;
+     }
+     if (depth < LITEETH_BUFFER_SIZE) {

Ditto reject if (depth / LITEETH_BUFFER_SIZE < 2) instead.

quoted

+             dev_err(priv->dev, "invalid rx-fifo-depth: %d\n", depth);

This should read "tx-fifo-depth".

Ack.

quoted

+             return -EINVAL;
+     }
+     priv->num_tx_slots = depth / LITEETH_BUFFER_SIZE;
+
+     return 0;
+}
+
+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 = devm_alloc_etherdev(&pdev->dev, sizeof(*priv));
+     if (!netdev)
+             return -ENOMEM;
+
+     SET_NETDEV_DEV(netdev, &pdev->dev);
+     platform_set_drvdata(pdev, netdev);
+
+     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 %d\n", irq);
+             return irq;

At this point, netdev has been dynamically allocated, and should
probably be free'd before liteeth_probe() is allowed to fail,
to avoid any potential leaks...

We use the managed variant of alloc_etherdev, which means the
structure is freed by the driver core when the driver is removed. This
saves having to open code the cleanup/free code.

Have a read of Documentation/driver-api/driver-model/devres.rst for
more information.

Thanks for the review Gabriel. I'll send a v3 with some fixes for the
fifo buffer handling.

Cheers,

Joel