netdev - Re: [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter

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: <465844cf-ff19-9bb5-15d3-cc876a2d40f6@gmail.com>
Date:   Thu, 23 Jan 2020 13:13:01 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jeremy Linton <jeremy.linton@....com>, netdev@...r.kernel.org
Cc:     opendmb@...il.com, davem@...emloft.net,
        bcm-kernel-feedback-list@...adcom.com,
        linux-kernel@...r.kernel.org, wahrenst@....net
Subject: Re: [RFC 2/2] net: bcmgenet: Fetch MAC address from the adapter

On 1/22/20 10:08 PM, Jeremy Linton wrote:
> ARM/ACPI machines should utilize self describing hardware
> when possible. The MAC address on the BCM GENET can be
> read from the adapter if a full featured firmware has already
> programmed it. Lets try using the address already programmed,
> if it appears to be valid.

s/BCM GENET/BCMGENET/ or just GENET.

> 
> Signed-off-by: Jeremy Linton <jeremy.linton@....com>
> ---
>  .../net/ethernet/broadcom/genet/bcmgenet.c    | 47 ++++++++++++++-----
>  1 file changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index c736700f829e..6782bb0a24bd 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2779,6 +2779,27 @@ static void bcmgenet_set_hw_addr(struct bcmgenet_priv *priv,
>  	bcmgenet_umac_writel(priv, (addr[4] << 8) | addr[5], UMAC_MAC1);
>  }
>  
> +static void bcmgenet_get_hw_addr(struct bcmgenet_priv *priv,
> +				 unsigned char *addr)
> +{
> +	u32 addr_tmp;
> +	bool acpi_mode = has_acpi_companion(&priv->pdev->dev);
> +
> +	/* UEFI/ACPI machines and possibly others will preprogram the MAC */
> +	if (acpi_mode) {
> +		addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC0);
> +		addr[0] = addr_tmp >> 24;
> +		addr[1] = (addr_tmp >> 16) & 0xff;
> +		addr[2] = (addr_tmp >>	8) & 0xff;
> +		addr[3] = addr_tmp & 0xff;
> +		addr_tmp = bcmgenet_umac_readl(priv, UMAC_MAC1);
> +		addr[4] = (addr_tmp >> 8) & 0xff;
> +		addr[5] = addr_tmp & 0xff;
> +	} else {
> +		memset(addr, 0, ETH_ALEN);
> +	}
> +}
> +
>  /* Returns a reusable dma control register value */
>  static u32 bcmgenet_dma_disable(struct bcmgenet_priv *priv)
>  {
> @@ -3509,11 +3530,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  	}
>  	priv->wol_irq = platform_get_irq_optional(pdev, 2);
>  
> -	if (dn)
> -		macaddr = of_get_mac_address(dn);
> -	else if (pd)
> -		macaddr = pd->mac_address;
> -
>  	priv->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(priv->base)) {
>  		err = PTR_ERR(priv->base);
> @@ -3524,12 +3540,6 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  
>  	SET_NETDEV_DEV(dev, &pdev->dev);
>  	dev_set_drvdata(&pdev->dev, dev);
> -	if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
> -		dev_warn(&pdev->dev, "using random Ethernet MAC\n");
> -		eth_hw_addr_random(dev);
> -	} else {
> -		ether_addr_copy(dev->dev_addr, macaddr);
> -	}
>  	dev->watchdog_timeo = 2 * HZ;
>  	dev->ethtool_ops = &bcmgenet_ethtool_ops;
>  	dev->netdev_ops = &bcmgenet_netdev_ops;
> @@ -3601,6 +3611,21 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  	    !strcasecmp(phy_mode_str, "internal"))
>  		bcmgenet_power_up(priv, GENET_POWER_PASSIVE);
>  
> +	if (dn)
> +		macaddr = of_get_mac_address(dn);
> +	else if (pd)
> +		macaddr = pd->mac_address;

I would be adding an:

	else if (has_acpi_companion())
		bcmgenet_get_hw_addr(priv, macaddr);

such that bcmgenet_get_hw_addr() does not have much ACPI specific
knowledge and get be used outside, with the caller knowing whether it is
appropriate.

You should also indicate in your commit message that you are moving the
fetching of the MAC address at a later point where the clocks are turned
on, to guarantee that the registers can be read.

With that fixed:

Acked-by: Florian Fainelli <f.fainelli@...il.com>

> +
> +	if (IS_ERR_OR_NULL(macaddr) || !is_valid_ether_addr(macaddr)) {
> +		bcmgenet_get_hw_addr(priv, dev->dev_addr);
> +		if (!is_valid_ether_addr(dev->dev_addr)) {
> +			dev_warn(&pdev->dev, "using random Ethernet MAC\n");
> +			eth_hw_addr_random(dev);
> +		}
> +	} else {
> +		ether_addr_copy(dev->dev_addr, macaddr);
> +	}
> +
>  	reset_umac(priv);
>  
>  	err = bcmgenet_mii_init(dev);
> 


-- 
Florian