netdev - Re: [PATCH v10 17/38] net: cirrus: add DT support for Cirrus EP93xx

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: Tue, 18 Jun 2024 19:37:06 +0300
From: Nikita Shubin <nikita.shubin@...uefel.me>
To: Simon Horman <horms@...nel.org>
Cc: Hartley Sweeten <hsweeten@...ionengravers.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
  linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>, Andrew Lunn
 <andrew@...n.ch>
Subject: Re: [PATCH v10 17/38] net: cirrus: add DT support for Cirrus EP93xx

Hi Simon!

On Tue, 2024-06-18 at 13:46 +0100, Simon Horman wrote:
> On Mon, Jun 17, 2024 at 12:36:51PM +0300, Nikita Shubin via B4 Relay
> wrote:
> > From: Nikita Shubin <nikita.shubin@...uefel.me>
> > 
> > - add OF ID match table
> > - get phy_id from the device tree, as part of mdio
> > - copy_addr is now always used, as there is no SoC/board that
> > aren't
> > - dropped platform header
> > 
> > Signed-off-by: Nikita Shubin <nikita.shubin@...uefel.me>
> > Tested-by: Alexander Sverdlin <alexander.sverdlin@...il.com>
> > Reviewed-by: Andrew Lunn <andrew@...n.ch>
> > Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> 
> Hi Nikita,
> 
> Some minor feedback from my side.

Thanks for catches!

I hope can address them next spin or after series will apply (desirably
the last one).

> 
> ...
> 
> > @@ -786,27 +766,47 @@ static void ep93xx_eth_remove(struct
> > platform_device *pdev)
> >  
> >  static int ep93xx_eth_probe(struct platform_device *pdev)
> >  {
> > -       struct ep93xx_eth_data *data;
> >         struct net_device *dev;
> >         struct ep93xx_priv *ep;
> >         struct resource *mem;
> > +       void __iomem *base_addr;
> > +       struct device_node *np;
> > +       u32 phy_id;
> >         int irq;
> >         int err;
> 
> nit: Please consider maintaining reverse xmas tree order - longest
> line
>      to shortest - for local variable declarations. As preferred in
>      Networking code.
> 
>      Edward Cree's tool can be of assistance here.
>      https://github.com/ecree-solarflare/xmastree
> 
> >  
> >         if (pdev == NULL)
> >                 return -ENODEV;
> > -       data = dev_get_platdata(&pdev->dev);
> >  
> >         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >         irq = platform_get_irq(pdev, 0);
> >         if (!mem || irq < 0)
> >                 return -ENXIO;
> >  
> > -       dev = ep93xx_dev_alloc(data);
> > +       base_addr = ioremap(mem->start, resource_size(mem));
> > +       if (!base_addr)
> > +               return dev_err_probe(&pdev->dev, -EIO, "Failed to
> > ioremap ethernet registers\n");
> 
> nit: Please consider line-wrapping to limiting lines to 80 columns
> wide
>      where it can be trivially achieved, which seems to be the case
> here.
>      80 columns is still preferred for Networking code.
> 
>      Flagged by checkpatch.pl --max-line-length=80
> 
> > +
> > +       np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> > +       if (!np)
> > +               return dev_err_probe(&pdev->dev, -ENODEV, "Please
> > provide \"phy-handle\"\n");
> > +
> > +       err = of_property_read_u32(np, "reg", &phy_id);
> > +       of_node_put(np);
> > +       if (err)
> > +               return dev_err_probe(&pdev->dev, -ENOENT, "Failed
> > to locate \"phy_id\"\n");
> > +
> > +       dev = alloc_etherdev(sizeof(struct ep93xx_priv));
> >         if (dev == NULL) {
> >                 err = -ENOMEM;
> >                 goto err_out;
> >         }
> > +
> > +       eth_hw_addr_set(dev, base_addr + 0x50);
> 
> base_addr is an __iomem address. As such I don't think it is correct
> to pass it (+ offset) to eth_hw_addr_set. Rather, I would expect
> base_addr
> to be read using a suitable iomem accessor, f.e. readl. And one
> possible
> solution would be to use readl to read the mac address into a buffer
> which is passed to eth_hw_addr_set.
> 
> Flagged by Sparse.
> 
> > +       dev->ethtool_ops = &ep93xx_ethtool_ops;
> > +       dev->netdev_ops = &ep93xx_netdev_ops;
> > +       dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM;
> > +
> >         ep = netdev_priv(dev);
> >         ep->dev = dev;
> >         SET_NETDEV_DEV(dev, &pdev->dev);
> 
> ...