Obtaining MAC address may be deferred in cases when the MAC is stored
in NVMEM block and it may now be ready upon the first retrieval attempt
returing EPROBE_DEFER. Handle it here and leave logic otherwise as it
was.

Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 105247582684..0694f53981f2 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2740,7 +2740,10 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	of_get_mac_address(pnp, ppd.mac_addr);
+	ret = of_get_mac_address(pnp, ppd.mac_addr);
+
+	if (ret == -EPROBE_DEFER)
+		return ret;
 
 	mv643xx_eth_property(pnp, "tx-queue-size", ppd.tx_queue_size);
 	mv643xx_eth_property(pnp, "tx-sram-addr", ppd.tx_sram_addr);

base-commit: cfb92440ee71adcc2105b0890bb01ac3cddb8507
-- 
2.25.1

On Mon, Feb 21, 2022 at 08:24:41AM +0200, Mauri Sandberg wrote:
> Obtaining MAC address may be deferred in cases when the MAC is stored
> in NVMEM block and it may now be ready upon the first retrieval attempt
> returing EPROBE_DEFER. Handle it here and leave logic otherwise as it
> was.
> 
> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
> ---
>  drivers/net/ethernet/marvell/mv643xx_eth.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index 105247582684..0694f53981f2 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
> @@ -2740,7 +2740,10 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> -	of_get_mac_address(pnp, ppd.mac_addr);
> +	ret = of_get_mac_address(pnp, ppd.mac_addr);
> +
> +	if (ret == -EPROBE_DEFER)
> +		return ret;

Hi Mauri

There appears to be a follow on issue. There can be multiple ports. So
it could be the first port does not use a MAC address from the NVMEM,
but the second one does. The first time in
mv643xx_eth_shared_of_add_port() is successful and a platform device
is added. The second port can then fail with -EPROBE_DEFER. That
causes the probe to fail, but the platform device will not be
removed. The next time the driver is probed, it will add a second
platform device for the first port, causing bad things to happen.

Please can you add code to remove the platform device when the probe
fails.

	Andrew

Hello Andrew,

On 21.02.22 14:21, Andrew Lunn wrote:
> On Mon, Feb 21, 2022 at 08:24:41AM +0200, Mauri Sandberg wrote:
>> Obtaining MAC address may be deferred in cases when the MAC is stored
>> in NVMEM block and it may now be ready upon the first retrieval attempt
>> returing EPROBE_DEFER. Handle it here and leave logic otherwise as it
>> was.
>>
>> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
>> ---
>>   drivers/net/ethernet/marvell/mv643xx_eth.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
>> index 105247582684..0694f53981f2 100644
>> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
>> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
>> @@ -2740,7 +2740,10 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
>>   		return -EINVAL;
>>   	}
>>   
>> -	of_get_mac_address(pnp, ppd.mac_addr);
>> +	ret = of_get_mac_address(pnp, ppd.mac_addr);
>> +
>> +	if (ret == -EPROBE_DEFER)
>> +		return ret;
> Hi Mauri
>
> There appears to be a follow on issue. There can be multiple ports. So
> it could be the first port does not use a MAC address from the NVMEM,
> but the second one does. The first time in
> mv643xx_eth_shared_of_add_port() is successful and a platform device
> is added. The second port can then fail with -EPROBE_DEFER. That
> causes the probe to fail, but the platform device will not be
> removed. The next time the driver is probed, it will add a second
> platform device for the first port, causing bad things to happen.
>
> Please can you add code to remove the platform device when the probe
> fails.

I am looking at the vector 'port_platdev' that holds pointers to already 
initialised ports. There is this mv643xx_eth_shared_of_remove(), which 
probably could be utilised to remove them. Should I remove the platform 
devices only in case of probe defer or always if probe fails?

> > Please can you add code to remove the platform device when the probe
> > fails.
> 
> I am looking at the vector 'port_platdev' that holds pointers to already
> initialised ports. There is this mv643xx_eth_shared_of_remove(), which
> probably could be utilised to remove them. Should I remove the platform
> devices only in case of probe defer or always if probe fails?
 
In general, a failing probe should always undo anything it has done so
far. Sometimes you can call the release function, or its
helpers. Other times you do a goto out: and then release stuff in the
reverse order it was taken.

It looks like platform_device_del() can take a NULL pointer, so it is
probably O.K. to call mv643xx_eth_shared_of_remove().

	 Andrew

On 22.2.2022 0.15, Andrew Lunn wrote:
>>> Please can you add code to remove the platform device when the probe
>>> fails.
>>
>> I am looking at the vector 'port_platdev' that holds pointers to already
>> initialised ports. There is this mv643xx_eth_shared_of_remove(), which
>> probably could be utilised to remove them. Should I remove the platform
>> devices only in case of probe defer or always if probe fails?
>   
> In general, a failing probe should always undo anything it has done so
> far. Sometimes you can call the release function, or its
> helpers. Other times you do a goto out: and then release stuff in the
> reverse order it was taken.
> 
> It looks like platform_device_del() can take a NULL pointer, so it is
> probably O.K. to call mv643xx_eth_shared_of_remove().

While I am on it, should I call of_node_put() to all port nodes as is
being done to the current child node if probe fails in function
mv643xx_eth_shared_of_probe() [1]?

[1] 
https://elixir.bootlin.com/linux/v5.16/source/drivers/net/ethernet/marvell/mv643xx_eth.c#L2800

-- Mauri

Obtaining a MAC address may be deferred in cases when the MAC is stored
in an NVMEM block, for example, and it may not be ready upon the first
retrieval attempt and return EPROBE_DEFER.

It is also possible that a port that does not rely on NVMEM has been
already created when getting the defer request. Thus, also the resources
allocated previously must be freed when doing a roll-back.

Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
Cc: Andrew Lunn <andrew@lunn.ch>
---
v1 -> v2
 - escalate all error values from of_get_mac_address()
 - move mv643xx_eth_shared_of_remove() before
   mv643xx_eth_shared_of_probe()
 - release all resources potentially allocated for previous port nodes
 - update commit title and message
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 24 +++++++++++++---------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 105247582684..143ca8be5eb5 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2704,6 +2704,16 @@ MODULE_DEVICE_TABLE(of, mv643xx_eth_shared_ids);
 
 static struct platform_device *port_platdev[3];
 
+static void mv643xx_eth_shared_of_remove(void)
+{
+	int n;
+
+	for (n = 0; n < 3; n++) {
+		platform_device_del(port_platdev[n]);
+		port_platdev[n] = NULL;
+	}
+}
+
 static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
 					  struct device_node *pnp)
 {
@@ -2740,7 +2750,9 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	of_get_mac_address(pnp, ppd.mac_addr);
+	ret = of_get_mac_address(pnp, ppd.mac_addr);
+	if (ret)
+		return ret;
 
 	mv643xx_eth_property(pnp, "tx-queue-size", ppd.tx_queue_size);
 	mv643xx_eth_property(pnp, "tx-sram-addr", ppd.tx_sram_addr);
@@ -2804,21 +2816,13 @@ static int mv643xx_eth_shared_of_probe(struct platform_device *pdev)
 		ret = mv643xx_eth_shared_of_add_port(pdev, pnp);
 		if (ret) {
 			of_node_put(pnp);
+			mv643xx_eth_shared_of_remove();
 			return ret;
 		}
 	}
 	return 0;
 }
 
-static void mv643xx_eth_shared_of_remove(void)
-{
-	int n;
-
-	for (n = 0; n < 3; n++) {
-		platform_device_del(port_platdev[n]);
-		port_platdev[n] = NULL;
-	}
-}
 #else
 static inline int mv643xx_eth_shared_of_probe(struct platform_device *pdev)
 {

base-commit: cfb92440ee71adcc2105b0890bb01ac3cddb8507
-- 
2.25.1

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 23 Feb 2022 16:23:37 +0200 you wrote:
> Obtaining a MAC address may be deferred in cases when the MAC is stored
> in an NVMEM block, for example, and it may not be ready upon the first
> retrieval attempt and return EPROBE_DEFER.
> 
> It is also possible that a port that does not rely on NVMEM has been
> already created when getting the defer request. Thus, also the resources
> allocated previously must be freed when doing a roll-back.
> 
> [...]

Here is the summary with links:
  - [v2] net: mv643xx_eth: process retval from of_get_mac_address
    https://git.kernel.org/netdev/net/c/42404d8f1c01

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Wed, 23 Feb 2022 16:23:37 +0200 Mauri Sandberg wrote:
> Obtaining a MAC address may be deferred in cases when the MAC is stored
> in an NVMEM block, for example, and it may not be ready upon the first
> retrieval attempt and return EPROBE_DEFER.
> 
> It is also possible that a port that does not rely on NVMEM has been
> already created when getting the defer request. Thus, also the resources
> allocated previously must be freed when doing a roll-back.
> 
> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
> Cc: Andrew Lunn <andrew@lunn.ch>

While we wait for Andrew's ack, is this the correct fixes tag?

Fixes: 76723bca2802 ("net: mv643xx_eth: add DT parsing support")

On Thu, Feb 24, 2022 at 08:57:54AM -0800, Jakub Kicinski wrote:
> On Wed, 23 Feb 2022 16:23:37 +0200 Mauri Sandberg wrote:
> > Obtaining a MAC address may be deferred in cases when the MAC is stored
> > in an NVMEM block, for example, and it may not be ready upon the first
> > retrieval attempt and return EPROBE_DEFER.
> > 
> > It is also possible that a port that does not rely on NVMEM has been
> > already created when getting the defer request. Thus, also the resources
> > allocated previously must be freed when doing a roll-back.
> > 
> > Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
> > Cc: Andrew Lunn <andrew@lunn.ch>
> 
> While we wait for Andrew's ack, is this the correct fixes tag?
> 
> Fixes: 76723bca2802 ("net: mv643xx_eth: add DT parsing support")


Yes, that looks correct. The history around here is convoluted, and
anybody trying to backport that far is going to need a few different
versions.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew