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: <20240618124610.GN8447@kernel.org>
Date: Tue, 18 Jun 2024 13:46:10 +0100
From: Simon Horman <horms@...nel.org>
To: nikita.shubin@...uefel.me
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

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.

...

> @@ -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);

...

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ