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

[Simon Horman]

On Tue, Mar 26, 2024 at 12:18:44PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> 
> - 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
> 
> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Signed-off-by: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> ---
>  drivers/net/ethernet/cirrus/ep93xx_eth.c | 63 ++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cirrus/ep93xx_eth.c b/drivers/net/ethernet/cirrus/ep93xx_eth.c
> index 1f495cfd7959..2523d9c9d1b8 100644
> --- a/drivers/net/ethernet/cirrus/ep93xx_eth.c
> +++ b/drivers/net/ethernet/cirrus/ep93xx_eth.c
> @@ -16,13 +16,12 @@
>  #include <linux/ethtool.h>
>  #include <linux/interrupt.h>
>  #include <linux/moduleparam.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  
> -#include <linux/platform_data/eth-ep93xx.h>
> -
>  #define DRV_MODULE_NAME		"ep93xx-eth"
>  
>  #define RX_QUEUE_ENTRIES	64
> @@ -738,25 +737,6 @@ static const struct net_device_ops ep93xx_netdev_ops = {
>  	.ndo_set_mac_address	= eth_mac_addr,
>  };
>  
> -static struct net_device *ep93xx_dev_alloc(struct ep93xx_eth_data *data)
> -{
> -	struct net_device *dev;
> -
> -	dev = alloc_etherdev(sizeof(struct ep93xx_priv));
> -	if (dev == NULL)
> -		return NULL;
> -
> -	eth_hw_addr_set(dev, data->dev_addr);
> -
> -	dev->ethtool_ops = &ep93xx_ethtool_ops;
> -	dev->netdev_ops = &ep93xx_netdev_ops;
> -
> -	dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM;
> -
> -	return dev;
> -}
> -
> -
>  static void ep93xx_eth_remove(struct platform_device *pdev)
>  {
>  	struct net_device *dev;
> @@ -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;

Please consider preserving reverse xmas tree order - longest line to
shortest, for local variables in Networking code.

>  
>  	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");
> +
> +	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");

This function, not entirely due to this patch, seems to leak
resources in error paths.

F.e., here base_addr is not unmapped and mem->start is not released.

I expect that to resolve this problem it would be best to move
to idiomatic error handling by:

* using a ladder of goto labels in ep93xx_eth_probe(); and
* as a clean up, remove the conditions from ep93xx_eth_remove.

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

This doesn't look right.

eth_hw_addr_set() expects it's second argument to
be a char * which represents an Ethernet address.
But the type of base_addr + 0x50 is __iomem *.

I suspect that you need some construction based on readb/readw/readl
to copy the Ethernet address from io memory into a buffer,
taking into account endiness and possibly taking into account
unaligned access.

Also, it would be good a descriptively named #define in place of 0x50.

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

..