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

Simon Horman
2024-03-28T20:34:33Z

On Tue, Mar 26, 2024 at 12:18:44PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.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
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> 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);

...