[Aspeed, v1 0/1] net: ftgmac100: Change the order of getting MAC address

[Aspeed, v1 0/1] net: ftgmac100: Change the order of getting MAC address

[Hongwei Zhang]

Dear Reviewer,

Use native MAC address is preferred over other choices, thus change the order
of reading MAC address, try to read it from MAC chip first, if it's not
 availabe, then try to read it from device tree.

Hongwei Zhang (1):
  net: ftgmac100: Change the order of getting MAC address

 drivers/net/ethernet/faraday/ftgmac100.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.17.1

[Aspeed, v1 1/1] net: ftgmac100: Change the order of getting MAC address

[Hongwei Zhang]

Change the order of reading MAC address, try to read it from MAC chip
first, if it's not availabe, then try to read it from device tree.

Fixes: 35c54922dc97 ("ARM: dts: tacoma: Add reserved memory for ramoops")
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 65cd25372020..9be69cbdab96 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -184,14 +184,7 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
 	unsigned int l;
 	void *addr;
 
-	addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
-	if (addr) {
-		ether_addr_copy(priv->netdev->dev_addr, mac);
-		dev_info(priv->dev, "Read MAC address %pM from device tree\n",
-			 mac);
-		return;
-	}
-
+	/* Read from Chip if not from chip */
 	m = ioread32(priv->base + FTGMAC100_OFFSET_MAC_MADR);
 	l = ioread32(priv->base + FTGMAC100_OFFSET_MAC_LADR);
 
@@ -205,7 +198,18 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
 	if (is_valid_ether_addr(mac)) {
 		ether_addr_copy(priv->netdev->dev_addr, mac);
 		dev_info(priv->dev, "Read MAC address %pM from chip\n", mac);
-	} else {
+		return;
+	}
+
+	/* Read from Chip if not from device tree */
+	addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
+	if (addr) {
+		ether_addr_copy(priv->netdev->dev_addr, mac);
+		dev_info(priv->dev, "Read MAC address %pM from device tree\n",
+				mac);
+		return;
+	}
+	else {
 		eth_hw_addr_random(priv->netdev);
 		dev_info(priv->dev, "Generated random MAC address %pM\n",
 			 priv->netdev->dev_addr);
-- 
2.17.1

Re: [Aspeed, v1 1/1] net: ftgmac100: Change the order of getting MAC address

[Heiner Kallweit]

Am 21.12.2020 um 21:51 schrieb Hongwei Zhang:
> Change the order of reading MAC address, try to read it from MAC chip
> first, if it's not availabe, then try to read it from device tree.
> 
This commit message leaves a number of questions. It seems the change
isn't related at all to the change that it's supposed to fix.

- What is the issue that you're trying to fix?
- And what is wrong with the original change?

> Fixes: 35c54922dc97 ("ARM: dts: tacoma: Add reserved memory for ramoops")
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 65cd25372020..9be69cbdab96 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -184,14 +184,7 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
>  	unsigned int l;
>  	void *addr;
>  
> -	addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
> -	if (addr) {
> -		ether_addr_copy(priv->netdev->dev_addr, mac);
> -		dev_info(priv->dev, "Read MAC address %pM from device tree\n",
> -			 mac);
> -		return;
> -	}
> -
> +	/* Read from Chip if not from chip */

?!?

>  	m = ioread32(priv->base + FTGMAC100_OFFSET_MAC_MADR);
>  	l = ioread32(priv->base + FTGMAC100_OFFSET_MAC_LADR);
>  
> @@ -205,7 +198,18 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
>  	if (is_valid_ether_addr(mac)) {
>  		ether_addr_copy(priv->netdev->dev_addr, mac);
>  		dev_info(priv->dev, "Read MAC address %pM from chip\n", mac);
> -	} else {
> +		return;
> +	}
> +
> +	/* Read from Chip if not from device tree */

Isn't this how it works now?

> +	addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
> +	if (addr) {
> +		ether_addr_copy(priv->netdev->dev_addr, mac);
> +		dev_info(priv->dev, "Read MAC address %pM from device tree\n",
> +				mac);
> +		return;
> +	}
> +	else {
>  		eth_hw_addr_random(priv->netdev);
>  		dev_info(priv->dev, "Generated random MAC address %pM\n",
>  			 priv->netdev->dev_addr);
> 

[Aspeed, v2 0/2] net: ftgmac100: Change the order of getting MAC address

[Hongwei Zhang]

Dear Reviewer,

Use native MAC address is preferred over other choices, thus change the order
of reading MAC address, try to read it from MAC chip first, if it's not
 availabe, then try to read it from device tree.

Thanks,
--Hongwei

Changelog:
v2:
- Corrected comments in the patch

v1: https://patchwork.ozlabs.org/project/linux-aspeed/list/?series=221576
- Initial submission

Hongwei Zhang (1):
  net: ftgmac100: Change the order of getting MAC address

 drivers/net/ethernet/faraday/ftgmac100.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.17.1

[Aspeed, v2 1/2] net: ftgmac100: Change the order of getting MAC address

[Hongwei Zhang]

Change the order of reading MAC address, try to read it from MAC chip
first, if it's not availabe, then try to read it from device tree.

Fixes: 35c54922dc97 ("ARM: dts: tacoma: Add reserved memory for ramoops")
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 65cd25372020..713e9325bef8 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -184,14 +184,7 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
 	unsigned int l;
 	void *addr;
 
-	addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
-	if (addr) {
-		ether_addr_copy(priv->netdev->dev_addr, mac);
-		dev_info(priv->dev, "Read MAC address %pM from device tree\n",
-			 mac);
-		return;
-	}
-
+	/* Try to read MAC from chip first */
 	m = ioread32(priv->base + FTGMAC100_OFFSET_MAC_MADR);
 	l = ioread32(priv->base + FTGMAC100_OFFSET_MAC_LADR);
 
@@ -205,7 +198,18 @@ static void ftgmac100_initial_mac(struct ftgmac100 *priv)
 	if (is_valid_ether_addr(mac)) {
 		ether_addr_copy(priv->netdev->dev_addr, mac);
 		dev_info(priv->dev, "Read MAC address %pM from chip\n", mac);
-	} else {
+		return;
+	}
+
+	/* Get MAC from device tree if it cannot be read from the chip */
+	addr = device_get_mac_address(priv->dev, mac, ETH_ALEN);
+	if (addr) {
+		ether_addr_copy(priv->netdev->dev_addr, mac);
+		dev_info(priv->dev, "Read MAC address %pM from device tree\n",
+				mac);
+		return;
+	}
+	else {
 		eth_hw_addr_random(priv->netdev);
 		dev_info(priv->dev, "Generated random MAC address %pM\n",
 			 priv->netdev->dev_addr);
-- 
2.17.1

[Aspeed, v2 2/2] net: ftgmac100: Change the order of getting MAC address

[Hongwei Zhang]

Dear Reviewer,

Use native MAC address is preferred over other choices, thus change the order
of reading MAC address, try to read it from MAC chip first, if it's not
 availabe, then try to read it from device tree.


Hi Heiner,

> From:	Heiner Kallweit <hkallweit1@gmail.com>
> Sent:	Monday, December 21, 2020 4:37 PM
> > Change the order of reading MAC address, try to read it from MAC chip 
> > first, if it's not availabe, then try to read it from device tree.
> > 
> This commit message leaves a number of questions. It seems the change isn't related at all to the 
> change that it's supposed to fix.
> 
> - What is the issue that you're trying to fix?
> - And what is wrong with the original change?

There is no bug or something wrong with the original code. This patch is for
improving the code. We thought if the native MAC address is available, then
it's preferred over MAC address from dts (assuming both sources are available).

One possible scenario, a MAC address is set in dts and the BMC image is 
compiled and loaded into more than one platform, then the platforms will
have network issue due to the same MAC address they read.

Thanks for your review, I've update the patch to fix the comments.
> 
> > Fixes: 35c54922dc97 ("ARM: dts: tacoma: Add reserved memory for 
> > ramoops")
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> >  drivers/net/ethernet/faraday/ftgmac100.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)

--Hongwei

Re: [Aspeed, v2 2/2] net: ftgmac100: Change the order of getting MAC address

[Heiner Kallweit]

On 22.12.2020 21:14, Hongwei Zhang wrote:
> Dear Reviewer,
> 
> Use native MAC address is preferred over other choices, thus change the order
> of reading MAC address, try to read it from MAC chip first, if it's not
>  availabe, then try to read it from device tree.
> 
> 
> Hi Heiner,
> 
>> From:	Heiner Kallweit <hkallweit1@gmail.com>
>> Sent:	Monday, December 21, 2020 4:37 PM
>>> Change the order of reading MAC address, try to read it from MAC chip 
>>> first, if it's not availabe, then try to read it from device tree.
>>>
>> This commit message leaves a number of questions. It seems the change isn't related at all to the 
>> change that it's supposed to fix.
>>
>> - What is the issue that you're trying to fix?
>> - And what is wrong with the original change?
> 
> There is no bug or something wrong with the original code. This patch is for
> improving the code. We thought if the native MAC address is available, then
> it's preferred over MAC address from dts (assuming both sources are available).
> 
> One possible scenario, a MAC address is set in dts and the BMC image is 
> compiled and loaded into more than one platform, then the platforms will
> have network issue due to the same MAC address they read.
> 

Typically the DTS MAC address is overwritten by the boot loader, e.g. uboot.
And the boot loader can read it from chip registers. There are more drivers
trying to read the MAC address from DTS first. Eventually, I think, the code
here will read the same MAC address from chip registers as uboot did before.

> Thanks for your review, I've update the patch to fix the comments.
>>
>>> Fixes: 35c54922dc97 ("ARM: dts: tacoma: Add reserved memory for 
>>> ramoops")
>>> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
>>> ---
>>>  drivers/net/ethernet/faraday/ftgmac100.c | 22 +++++++++++++---------
>>>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> --Hongwei
> 

Re: [Aspeed, v2 2/2] net: ftgmac100: Change the order of getting MAC address

[Andrew Lunn]

On Tue, Dec 22, 2020 at 09:46:52PM +0100, Heiner Kallweit wrote:
> On 22.12.2020 21:14, Hongwei Zhang wrote:
> > Dear Reviewer,
> > 
> > Use native MAC address is preferred over other choices, thus change the order
> > of reading MAC address, try to read it from MAC chip first, if it's not
> >  availabe, then try to read it from device tree.
> > 
> > 
> > Hi Heiner,
> > 
> >> From:	Heiner Kallweit <hkallweit1@gmail.com>
> >> Sent:	Monday, December 21, 2020 4:37 PM
> >>> Change the order of reading MAC address, try to read it from MAC chip 
> >>> first, if it's not availabe, then try to read it from device tree.
> >>>
> >> This commit message leaves a number of questions. It seems the change isn't related at all to the 
> >> change that it's supposed to fix.
> >>
> >> - What is the issue that you're trying to fix?
> >> - And what is wrong with the original change?
> > 
> > There is no bug or something wrong with the original code. This patch is for
> > improving the code. We thought if the native MAC address is available, then
> > it's preferred over MAC address from dts (assuming both sources are available).
> > 
> > One possible scenario, a MAC address is set in dts and the BMC image is 
> > compiled and loaded into more than one platform, then the platforms will
> > have network issue due to the same MAC address they read.
> > 
> 
> Typically the DTS MAC address is overwritten by the boot loader, e.g. uboot.
> And the boot loader can read it from chip registers. There are more drivers
> trying to read the MAC address from DTS first. Eventually, I think, the code
> here will read the same MAC address from chip registers as uboot did before.

Do we need to worry about, the chip contains random junk, which passes
the validitiy test? Before this patch the value from DT would be used,
and the random junk is ignored. Is this change possibly going to cause
a regression?

	Andrew

Re: [Aspeed, v2 2/2] net: ftgmac100: Change the order of getting MAC address

[Jakub Kicinski]

On Tue, 22 Dec 2020 22:00:34 +0100 Andrew Lunn wrote:
> On Tue, Dec 22, 2020 at 09:46:52PM +0100, Heiner Kallweit wrote:
> > On 22.12.2020 21:14, Hongwei Zhang wrote:  
> > > Dear Reviewer,
> > > 
> > > Use native MAC address is preferred over other choices, thus change the order
> > > of reading MAC address, try to read it from MAC chip first, if it's not
> > >  availabe, then try to read it from device tree.
> > > 
> > > Hi Heiner,
> > >   
> > >> From:	Heiner Kallweit <hkallweit1@gmail.com>
> > >> Sent:	Monday, December 21, 2020 4:37 PM  
> > >>> Change the order of reading MAC address, try to read it from MAC chip 
> > >>> first, if it's not availabe, then try to read it from device tree.
> > >>>  
> > >> This commit message leaves a number of questions. It seems the change isn't related at all to the 
> > >> change that it's supposed to fix.
> > >>
> > >> - What is the issue that you're trying to fix?
> > >> - And what is wrong with the original change?  
> > > 
> > > There is no bug or something wrong with the original code. This patch is for
> > > improving the code. We thought if the native MAC address is available, then
> > > it's preferred over MAC address from dts (assuming both sources are available).
> > > 
> > > One possible scenario, a MAC address is set in dts and the BMC image is 
> > > compiled and loaded into more than one platform, then the platforms will
> > > have network issue due to the same MAC address they read.
> > >   
> > 
> > Typically the DTS MAC address is overwritten by the boot loader, e.g. uboot.
> > And the boot loader can read it from chip registers. There are more drivers
> > trying to read the MAC address from DTS first. Eventually, I think, the code
> > here will read the same MAC address from chip registers as uboot did before.  
> 
> Do we need to worry about, the chip contains random junk, which passes
> the validitiy test? Before this patch the value from DT would be used,
> and the random junk is ignored. Is this change possibly going to cause
> a regression?

Hongwei, please address Andrew's questions.

Once the discussion is over please repost the patches as
git-format-patch would generate them. The patch 2/2 of this 
series is not really a patch, which confuses all patch handling 
systems.

It also appears that 35c54922dc97 ("ARM: dts: tacoma: Add reserved
memory for ramoops") does not exist upstream.

[Aspeed, v1 1/1] net: ftgmac100: Change the order of getting MAC address

[Hongwei Zhang]

> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, December 28, 2020 5:01 PM
>
> On Tue, 22 Dec 2020 22:00:34 +0100 Andrew Lunn wrote:
> > On Tue, Dec 22, 2020 at 09:46:52PM +0100, Heiner Kallweit wrote:
> > > On 22.12.2020 21:14, Hongwei Zhang wrote:
> > > > Dear Reviewer,
> > > >
> > > > Use native MAC address is preferred over other choices, thus change the order
> > > > of reading MAC address, try to read it from MAC chip first, if it's not
> > > >  availabe, then try to read it from device tree.
> > > >
> > > > Hi Heiner,
> > > >
> > > >> From:  Heiner Kallweit <hkallweit1@gmail.com>
> > > >> Sent:  Monday, December 21, 2020 4:37 PM
> > > >>> Change the order of reading MAC address, try to read it from MAC chip
> > > >>> first, if it's not availabe, then try to read it from device tree.
> > > >>>
> > > >> This commit message leaves a number of questions. It seems the change isn't related at all to the
> > > >> change that it's supposed to fix.
> > > >>
> > > >> - What is the issue that you're trying to fix?
> > > >> - And what is wrong with the original change?
> > > >
> > > > There is no bug or something wrong with the original code. This patch is for
> > > > improving the code. We thought if the native MAC address is available, then
> > > > it's preferred over MAC address from dts (assuming both sources are available).
> > > >
> > > > One possible scenario, a MAC address is set in dts and the BMC image is
> > > > compiled and loaded into more than one platform, then the platforms will
> > > > have network issue due to the same MAC address they read.
> > > >
> > >
> > > Typically the DTS MAC address is overwritten by the boot loader, e.g. uboot.
> > > And the boot loader can read it from chip registers. There are more drivers
> > > trying to read the MAC address from DTS first. Eventually, I think, the code
> > > here will read the same MAC address from chip registers as uboot did before.

Thanks for your review, Heiner,

I am working on a platform and want to use the method you said, reading from DTS
is easy, but overwrite the MAC in DTS with chip MAC address, it will change the
checksum of the image. Would you please provide an implementation example?

Thanks!
> >
> > Do we need to worry about, the chip contains random junk, which passes
> > the validitiy test? Before this patch the value from DT would be used,
> > and the random junk is ignored. Is this change possibly going to cause
> > a regression?

Hi Andrew,

Thanks for your review. Yes, yours is a good point, as my change relies on
the driver's ability to read correct MAC from the chip, or the check of
is_valid_ether_addr(), which only checking for zeros and multicasting MAC.
On the other hand, your concern is still true if no MAC is defined in DTS
file.

Thanks!
>
> Hongwei, please address Andrew's questions.
>
> Once the discussion is over please repost the patches as
> git-format-patch would generate them. The patch 2/2 of this
> series is not really a patch, which confuses all patch handling
> systems.
>
> It also appears that 35c54922dc97 ("ARM: dts: tacoma: Add reserved
> memory for ramoops") does not exist upstream.
>

Hi Jakub,

Thanks for your review; I am quite new to the contribution process. I will resubmit my
patch with the SHA value issue fixed. Please see my response at above.

--Hongwei

-- 
2.17.1

Re: [Aspeed, v1 1/1] net: ftgmac100: Change the order of getting MAC address

[Heiner Kallweit]

On 04.01.2021 18:28, Hongwei Zhang wrote:
> 
>> From: Jakub Kicinski <kuba@kernel.org>
>> Sent: Monday, December 28, 2020 5:01 PM
>>
>> On Tue, 22 Dec 2020 22:00:34 +0100 Andrew Lunn wrote:
>>> On Tue, Dec 22, 2020 at 09:46:52PM +0100, Heiner Kallweit wrote:
>>>> On 22.12.2020 21:14, Hongwei Zhang wrote:
>>>>> Dear Reviewer,
>>>>>
>>>>> Use native MAC address is preferred over other choices, thus change the order
>>>>> of reading MAC address, try to read it from MAC chip first, if it's not
>>>>>  availabe, then try to read it from device tree.
>>>>>
>>>>> Hi Heiner,
>>>>>
>>>>>> From:  Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> Sent:  Monday, December 21, 2020 4:37 PM
>>>>>>> Change the order of reading MAC address, try to read it from MAC chip
>>>>>>> first, if it's not availabe, then try to read it from device tree.
>>>>>>>
>>>>>> This commit message leaves a number of questions. It seems the change isn't related at all to the
>>>>>> change that it's supposed to fix.
>>>>>>
>>>>>> - What is the issue that you're trying to fix?
>>>>>> - And what is wrong with the original change?
>>>>>
>>>>> There is no bug or something wrong with the original code. This patch is for
>>>>> improving the code. We thought if the native MAC address is available, then
>>>>> it's preferred over MAC address from dts (assuming both sources are available).
>>>>>
>>>>> One possible scenario, a MAC address is set in dts and the BMC image is
>>>>> compiled and loaded into more than one platform, then the platforms will
>>>>> have network issue due to the same MAC address they read.
>>>>>
>>>>
>>>> Typically the DTS MAC address is overwritten by the boot loader, e.g. uboot.
>>>> And the boot loader can read it from chip registers. There are more drivers
>>>> trying to read the MAC address from DTS first. Eventually, I think, the code
>>>> here will read the same MAC address from chip registers as uboot did before.
> 
> Thanks for your review, Heiner,
> 
> I am working on a platform and want to use the method you said, reading from DTS
> is easy, but overwrite the MAC in DTS with chip MAC address, it will change the
> checksum of the image. Would you please provide an implementation example?
> 
One example is the igb driver. That's the relevant code snippet:

if (eth_platform_get_mac_address(&pdev->dev, hw->mac.addr)) {
	/* copy the MAC address out of the NVM */
	if (hw->mac.ops.read_mac_addr(hw))
		dev_err(&pdev->dev, "NVM Read Error\n");
}

And I'm not sure the image checksum is relevant here. The boot loader
dynamically replaces the MAC address before handing over the DTS to
Linux kernel. At that time an image checksum shouldn't be relevant.
Who would be supposed to check it?

> Thanks!
>>>
>>> Do we need to worry about, the chip contains random junk, which passes
>>> the validitiy test? Before this patch the value from DT would be used,
>>> and the random junk is ignored. Is this change possibly going to cause
>>> a regression?
> 
> Hi Andrew,
> 
> Thanks for your review. Yes, yours is a good point, as my change relies on
> the driver's ability to read correct MAC from the chip, or the check of
> is_valid_ether_addr(), which only checking for zeros and multicasting MAC.
> On the other hand, your concern is still true if no MAC is defined in DTS
> file.
> 
> Thanks!
>>
>> Hongwei, please address Andrew's questions.
>>
>> Once the discussion is over please repost the patches as
>> git-format-patch would generate them. The patch 2/2 of this
>> series is not really a patch, which confuses all patch handling
>> systems.
>>
>> It also appears that 35c54922dc97 ("ARM: dts: tacoma: Add reserved
>> memory for ramoops") does not exist upstream.
>>
> 
> Hi Jakub,
> 
> Thanks for your review; I am quite new to the contribution process. I will resubmit my
> patch with the SHA value issue fixed. Please see my response at above.
> 
> --Hongwei
>