netdev - Re: [PATCH 2/2] net: Add driver for LiteX's LiteETH network interface

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]

Date:   Mon, 9 Aug 2021 03:20:57 +0000
From:   Joel Stanley <joel@....id.au>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        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 <devicetree@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] net: Add driver for LiteX's LiteETH network interface

On Sat, 7 Aug 2021 at 19:19, Andrew Lunn <andrew@...n.ch> wrote:
>
> > +static void liteeth_reset_hw(struct liteeth *priv)
> > +{
> > +     /* Reset, twice */
> > +     writeb(0, priv->base + LITEETH_PHY_CRG_RESET);
> > +     udelay(10);
> > +     writeb(1, priv->base + LITEETH_PHY_CRG_RESET);
> > +     udelay(10);
> > +     writeb(0, priv->base + LITEETH_PHY_CRG_RESET);
> > +     udelay(10);
>
> What is this actually resetting?

This comes from the reference firmware that many (but not all) litex
systems run before loading their operating system.

I'm not completely sure how necessary it still is; I will drop it for now.

>
> > +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;
> > +     }
> > +
> > +     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;
> > +     }
>
> So you don't have any PHY handling, or any MDIO bus master code. So i
> would drop this, until the MDIO architecture question is answered. I
> also wonder how much use the MAC driver is without any PHY code?
> Unless you have a good reason, i don't think we should merge this
> until it makes the needed calls into phylib. It is not much code to
> add.

You mean I should skip out the parsing of the mdio base until I'm
using it? That's reasonable.