[PATCH v2 0/5] Fix prestera driver fail to probe twice

[PATCH v2 0/5] Fix prestera driver fail to probe twice

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Fix issues resulting from insmod, rmmod and insmod of the
prestera driver:

1. Call of firmware switch HW reset was missing, and is required
   in order to make the firmware loader shift to the correct state
   needed for loading the next firmware.
2. Time-out for waiting for firmware loader to be ready was too small.
3. memory referencing after freeing
4. MAC addresses wrapping
5. Missing SFP unbind (phylink release) of a port during the port release.

v2:
  1) Split first patch to firmware call for switch HW reset and to
     increasing of firmware loader wait to be ready timeout
  2) Explain why is switch HW reset call to the firmware required
     before shutdown in commit message, and fix wording
  3) Use a simpler bitwise-and on the last byte of the base MAC address
     instead of randomizing again the entire MAC address.
     reflect that change in the commit message, and explain why it is
     needed (switch HW implementation requirement).
  4) Add Fixes Tags to all commits.
  5) For timeout enlargement commit, fix wording in comment and move
     timeout units to milliseconds.
  6) Add Tested-By tags.
  7) Add patch to call prestera_port_sfp_unbind() from
     prestera_destroy_ports() in order to release phylink.

Elad Nachman (5):
  net: marvell: prestera: fix driver reload
  net: marvell: prestera: enlarge fw restart time
  net: marvell: prestera: fix memory use after free
  net: marvell: prestera: force good base mac
  net: marvell: prestera: unbind sfp port on exit

 drivers/net/ethernet/marvell/prestera/prestera_hw.c       | 8 ++++++++
 drivers/net/ethernet/marvell/prestera/prestera_hw.h       | 1 +
 drivers/net/ethernet/marvell/prestera/prestera_main.c     | 5 ++++-
 drivers/net/ethernet/marvell/prestera/prestera_pci.c      | 7 ++++++-
 drivers/net/ethernet/marvell/prestera/prestera_router.c   | 1 -
 .../net/ethernet/marvell/prestera/prestera_router_hw.c    | 1 -
 6 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/5] net: marvell: prestera: fix driver reload

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Driver rmmod after insmod would fail because API call to reset the switch
HW and restart the firmware CPU code loading procedure was missing in
driver removal code handler.

Firmware reset and reload is needed as the firmware termination will make
the firmware loader change its state machine to the firmware loading state,
and thus will be able to load new firmware, which is done at the beginning
of the probing of the prestera_pci module.

Without this reset, the firmware loader will stay in the wrong state,
causing the next firmware loading phase in the probe to fail.

Fix by adding API call to reset the switch HW and restart the firmware CPU
firmware code loading when handling the driver removal procedure.

Reported-by: Köry Maincent <kory.maincent@bootlin.com>
Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
Fixes: 501ef3066c89 ("net: marvell: prestera: Add driver for Prestera family ASIC devices")
Tested-by: Kory Maincent <kory.maincent@bootlin.com>
Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_hw.c   | 8 ++++++++
 drivers/net/ethernet/marvell/prestera/prestera_hw.h   | 1 +
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 3 ++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
index fc6f7d2746e8..08de8b498e0a 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
@@ -21,6 +21,7 @@
 enum prestera_cmd_type_t {
 	PRESTERA_CMD_TYPE_SWITCH_INIT = 0x1,
 	PRESTERA_CMD_TYPE_SWITCH_ATTR_SET = 0x2,
+	PRESTERA_CMD_TYPE_SWITCH_RESET = 0x4,
 
 	PRESTERA_CMD_TYPE_PORT_ATTR_SET = 0x100,
 	PRESTERA_CMD_TYPE_PORT_ATTR_GET = 0x101,
@@ -1087,6 +1088,13 @@ void prestera_hw_switch_fini(struct prestera_switch *sw)
 	WARN_ON(!list_empty(&sw->event_handlers));
 }
 
+int prestera_hw_switch_reset(struct prestera_switch *sw)
+{
+	struct prestera_msg_common_req req;
+
+	return prestera_cmd(sw, PRESTERA_CMD_TYPE_SWITCH_RESET, &req.cmd, sizeof(req));
+}
+
 int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms)
 {
 	struct prestera_msg_switch_attr_req req = {
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.h b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
index 0a929279e1ce..86217bea2ca0 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_hw.h
+++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
@@ -150,6 +150,7 @@ struct prestera_neigh_info;
 
 /* Switch API */
 int prestera_hw_switch_init(struct prestera_switch *sw);
+int prestera_hw_switch_reset(struct prestera_switch *sw);
 void prestera_hw_switch_fini(struct prestera_switch *sw);
 int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms);
 int prestera_hw_switch_mac_set(struct prestera_switch *sw, const char *mac);
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index 4fb886c57cd7..bcaa8ea27b49 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -1444,7 +1444,7 @@ static int prestera_switch_init(struct prestera_switch *sw)
 err_router_init:
 	prestera_netdev_event_handler_unregister(sw);
 	prestera_hw_switch_fini(sw);
-
+	prestera_hw_switch_reset(sw);
 	return err;
 }
 
@@ -1463,6 +1463,7 @@ static void prestera_switch_fini(struct prestera_switch *sw)
 	prestera_router_fini(sw);
 	prestera_netdev_event_handler_unregister(sw);
 	prestera_hw_switch_fini(sw);
+	prestera_hw_switch_reset(sw);
 	of_node_put(sw->np);
 }
 
-- 
2.25.1

Re: [PATCH v2 1/5] net: marvell: prestera: fix driver reload

[Andrew Lunn]

On Wed, Mar 20, 2024 at 07:20:04PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Driver rmmod after insmod would fail because API call to reset the switch
> HW and restart the firmware CPU code loading procedure was missing in
> driver removal code handler.
> 
> Firmware reset and reload is needed as the firmware termination will make
> the firmware loader change its state machine to the firmware loading state,
> and thus will be able to load new firmware, which is done at the beginning
> of the probing of the prestera_pci module.
> 
> Without this reset, the firmware loader will stay in the wrong state,
> causing the next firmware loading phase in the probe to fail.

What is missing from this is an explanation why you need to reload the
firmware at the next re-probe. That just seems like a waste of time if
you have already loaded it once.

    Andrew

RE: [EXTERNAL] Re: [PATCH v2 1/5] net: marvell: prestera: fix driver reload

[Elad Nachman]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, March 21, 2024 12:58 AM
> To: Elad Nachman <enachman@marvell.com>
> Cc: Taras Chornyi <taras.chornyi@plvision.eu>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> kory.maincent@bootlin.com; thomas.petazzoni@bootlin.com;
> miquel.raynal@bootlin.com; przemyslaw.kitszel@intel.com;
> dkirjanov@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v2 1/5] net: marvell: prestera: fix driver
> reload
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> On Wed, Mar 20, 2024 at 07:20:04PM +0200, Elad Nachman wrote:
> > From: Elad Nachman <enachman@marvell.com>
> >
> > Driver rmmod after insmod would fail because API call to reset the
> > switch HW and restart the firmware CPU code loading procedure was
> > missing in driver removal code handler.
> >
> > Firmware reset and reload is needed as the firmware termination will
> > make the firmware loader change its state machine to the firmware
> > loading state, and thus will be able to load new firmware, which is
> > done at the beginning of the probing of the prestera_pci module.
> >
> > Without this reset, the firmware loader will stay in the wrong state,
> > causing the next firmware loading phase in the probe to fail.
> 
> What is missing from this is an explanation why you need to reload the
> firmware at the next re-probe. That just seems like a waste of time if you have
> already loaded it once.
> 
>     Andrew

Unfortunately that's how the firmware loader on the firmware cpu state machine works.
There is no ABI interface to verify which firmware is already loaded, and then supporting
Warm boot reading of the values back to the kernel.
Since many of these firmware binaries are secure-boot protected, upgrading is very tricky.

Elad.

[PATCH v2 2/5] net: marvell: prestera: enlarge fw restart time

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Increase firmware restart timeout, as current timeout value of 5 seconds
was too small, in actual life it can take up to 30 seconds for firmware
to shutdown and for the firmware loader to shift to the ready to receive
firmware code state.

Reported-by: Köry Maincent <kory.maincent@bootlin.com>
Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
Fixes: 4c2703dfd7fa ("net: marvell: prestera: Add PCI interface support")
Tested-by: Kory Maincent <kory.maincent@bootlin.com>
Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_pci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
index 35857dc19542..2caa33d195ab 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
@@ -24,6 +24,11 @@
 #define PRESTERA_FW_ARM64_PATH_FMT "mrvl/prestera/mvsw_prestera_fw_arm64-v%u.%u.img"
 
 #define PRESTERA_FW_HDR_MAGIC		0x351D9D06
+/* Timeout waiting for firmware CPU to reboot and restart
+ * the firmware loading software layer, hence becoming
+ * ready for the next firmware downloading phase:
+ */
+#define PRESTERA_FW_READY_TIMEOUT_MS	30000
 #define PRESTERA_FW_DL_TIMEOUT_MS	50000
 #define PRESTERA_FW_BLK_SZ		1024
 
@@ -765,7 +770,7 @@ static int prestera_fw_load(struct prestera_fw *fw)
 
 	err = prestera_ldr_wait_reg32(fw, PRESTERA_LDR_READY_REG,
 				      PRESTERA_LDR_READY_MAGIC,
-				      5 * MSEC_PER_SEC);
+				      PRESTERA_FW_READY_TIMEOUT_MS);
 	if (err) {
 		dev_err(fw->dev.dev, "waiting for FW loader is timed out");
 		return err;
-- 
2.25.1

Re: [PATCH v2 2/5] net: marvell: prestera: enlarge fw restart time

[Andrew Lunn]

On Wed, Mar 20, 2024 at 07:20:05PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Increase firmware restart timeout, as current timeout value of 5 seconds
> was too small, in actual life it can take up to 30 seconds for firmware
> to shutdown and for the firmware loader to shift to the ready to receive
> firmware code state.

So this means the probe can be waiting 30 seconds?

This seems like a really good reason not to reset the hardware during
-EPROBE_DEFER.

	Andrew

RE: [EXTERNAL] Re: [PATCH v2 2/5] net: marvell: prestera: enlarge fw restart time

[Elad Nachman]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, March 21, 2024 2:10 AM
> To: Elad Nachman <enachman@marvell.com>
> Cc: Taras Chornyi <taras.chornyi@plvision.eu>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> kory.maincent@bootlin.com; thomas.petazzoni@bootlin.com;
> miquel.raynal@bootlin.com; przemyslaw.kitszel@intel.com;
> dkirjanov@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v2 2/5] net: marvell: prestera: enlarge fw
> restart time
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> On Wed, Mar 20, 2024 at 07:20:05PM +0200, Elad Nachman wrote:
> > From: Elad Nachman <enachman@marvell.com>
> >
> > Increase firmware restart timeout, as current timeout value of 5
> > seconds was too small, in actual life it can take up to 30 seconds for
> > firmware to shutdown and for the firmware loader to shift to the ready
> > to receive firmware code state.
> 
> So this means the probe can be waiting 30 seconds?
> 
> This seems like a really good reason not to reset the hardware during -
> EPROBE_DEFER.
> 
> 	Andrew

Unfortunately that's how the firmware loader on the firmware cpu state machine works.
The so called HW reset will force exit of the current firmware and will move the state machine
To the state that it can receive the next firmware.

Unfortunately, there is no ABI interface to verify which firmware is already loaded, and then supporting
Warm boot reading of the values back to the kernel.
Since many of these firmware binaries are secure-boot protected, upgrading is very tricky.

Elad.

[PATCH v2 3/5] net: marvell: prestera: fix memory use after free

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Prestera driver routing module cleanup process would release memory,
then reference it again and eventually free it again the second time.

Remove the redundant first memory free call.
All such double free calls were detected using KASAN.

Fixes: 4394fbcb78cf ("net: marvell: prestera: handle fib notifications")
Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_router.c    | 1 -
 drivers/net/ethernet/marvell/prestera/prestera_router_hw.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router.c b/drivers/net/ethernet/marvell/prestera/prestera_router.c
index de317179a7dc..2da04a17efad 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_router.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_router.c
@@ -1638,7 +1638,6 @@ void prestera_router_fini(struct prestera_switch *sw)
 	prestera_k_arb_abort(sw);
 
 	kfree(sw->router->nhgrp_hw_state_cache);
-	rhashtable_destroy(&sw->router->kern_fib_cache_ht);
 	prestera_router_hw_fini(sw);
 	kfree(sw->router);
 	sw->router = NULL;
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
index 02faaea2aefa..254107f664b4 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
@@ -102,7 +102,6 @@ void prestera_router_hw_fini(struct prestera_switch *sw)
 				    prestera_fib_node_destroy_ht_cb, sw);
 	WARN_ON(!list_empty(&sw->router->vr_list));
 	WARN_ON(!list_empty(&sw->router->rif_entry_list));
-	rhashtable_destroy(&sw->router->fib_ht);
 	rhashtable_destroy(&sw->router->nexthop_group_ht);
 	rhashtable_destroy(&sw->router->nh_neigh_ht);
 }
-- 
2.25.1

Re: [PATCH v2 3/5] net: marvell: prestera: fix memory use after free

[Andrew Lunn]

On Wed, Mar 20, 2024 at 07:20:06PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Prestera driver routing module cleanup process would release memory,
> then reference it again and eventually free it again the second time.
> 
> Remove the redundant first memory free call.
> All such double free calls were detected using KASAN.
> 
> Fixes: 4394fbcb78cf ("net: marvell: prestera: handle fib notifications")
> Signed-off-by: Elad Nachman <enachman@marvell.com>

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

    Andrew

[PATCH v2 4/5] net: marvell: prestera: force good base mac

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

The switching ASIC router HW unit MAC Source Address is configured with
40-bits of MAC base address in its registers (done in the firmware code),
requiring all ports doing L3 routing in HW to have the same upper 40-bit
MAC address.

Since each switchport MAC address uses the switch base mac address and
then adds the physical port number to it, Force the last byte of the
switch base mac address to be at most 127, so when adding to it, we
will not wrap around the previous (more significant) mac address byte,
resulting in a warning message.

Fixes: 501ef3066c89 ("net: marvell: prestera: Add driver for Prestera family ASIC devices")
Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index bcaa8ea27b49..87d8fc4162b3 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -860,6 +860,7 @@ static int prestera_switch_set_base_mac_addr(struct prestera_switch *sw)
 		ret = of_get_mac_address(sw->np, sw->base_mac);
 	if (!is_valid_ether_addr(sw->base_mac) || ret) {
 		eth_random_addr(sw->base_mac);
+		sw->base_mac[5] &= 0x7f;
 		dev_info(prestera_dev(sw), "using random base mac address\n");
 	}
 
-- 
2.25.1

Re: [PATCH v2 4/5] net: marvell: prestera: force good base mac

[Andrew Lunn]

On Wed, Mar 20, 2024 at 07:20:07PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> The switching ASIC router HW unit MAC Source Address is configured with
> 40-bits of MAC base address in its registers (done in the firmware code),
> requiring all ports doing L3 routing in HW to have the same upper 40-bit
> MAC address.
> 
> Since each switchport MAC address uses the switch base mac address and
> then adds the physical port number to it, Force the last byte of the
> switch base mac address to be at most 127, so when adding to it, we
> will not wrap around the previous (more significant) mac address byte,
> resulting in a warning message.
> 
> Fixes: 501ef3066c89 ("net: marvell: prestera: Add driver for Prestera family ASIC devices")
> Signed-off-by: Elad Nachman <enachman@marvell.com>

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

    Andrew

[PATCH v2 5/5] net: marvell: prestera: unbind sfp port on exit

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Call unbinding of the sfp port when ports are released, This will call
phylink_destroy() to cleanup and destroy the phylink instance, needed
to unregister SFP and free used memory, which will otherwise leak.

Reported-by: Köry Maincent <kory.maincent@bootlin.com>
Closes: https://lore.kernel.org/lkml/20240312164526.4a0e242a@kmaincent-XPS-13-7390/
Fixes: 501ef3066c89 ("net: marvell: prestera: Add driver for Prestera family ASIC devices")
Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index 87d8fc4162b3..20c0ebb52af8 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -756,6 +756,7 @@ static void prestera_port_destroy(struct prestera_port *port)
 	cancel_delayed_work_sync(&port->cached_hw_stats.caching_dw);
 	unregister_netdev(dev);
 	prestera_port_list_del(port);
+	prestera_port_sfp_unbind(port);
 	prestera_devlink_port_unregister(port);
 	free_netdev(dev);
 }
-- 
2.25.1

Re: [PATCH v2 5/5] net: marvell: prestera: unbind sfp port on exit

[Andrew Lunn]

On Wed, Mar 20, 2024 at 07:20:08PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Call unbinding of the sfp port when ports are released, This will call
> phylink_destroy() to cleanup and destroy the phylink instance, needed
> to unregister SFP and free used memory, which will otherwise leak.
> 
> Reported-by: Köry Maincent <kory.maincent@bootlin.com>
> Closes: https://lore.kernel.org/lkml/20240312164526.4a0e242a@kmaincent-XPS-13-7390/
> Fixes: 501ef3066c89 ("net: marvell: prestera: Add driver for Prestera family ASIC devices")
> Signed-off-by: Elad Nachman <enachman@marvell.com>

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

    Andrew

Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Andrew Lunn]

On Wed, Mar 20, 2024 at 07:20:03PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Fix issues resulting from insmod, rmmod and insmod of the
> prestera driver:
> 
> 1. Call of firmware switch HW reset was missing, and is required
>    in order to make the firmware loader shift to the correct state
>    needed for loading the next firmware.
> 2. Time-out for waiting for firmware loader to be ready was too small.
> 3. memory referencing after freeing
> 4. MAC addresses wrapping
> 5. Missing SFP unbind (phylink release) of a port during the port release.

I don't see any problems with 3-5. Maybe post them independent of the
first 2 and they can be merged.

Are you really saying it is impossible to determine if the hardware is
in the boot loader waiting for firmware, or is running the firmware?

   Andrew

RE: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Elad Nachman]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, March 21, 2024 2:18 AM
> To: Elad Nachman <enachman@marvell.com>
> Cc: Taras Chornyi <taras.chornyi@plvision.eu>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> kory.maincent@bootlin.com; thomas.petazzoni@bootlin.com;
> miquel.raynal@bootlin.com; przemyslaw.kitszel@intel.com;
> dkirjanov@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> On Wed, Mar 20, 2024 at 07:20:03PM +0200, Elad Nachman wrote:
> > From: Elad Nachman <enachman@marvell.com>
> >
> > Fix issues resulting from insmod, rmmod and insmod of the prestera
> > driver:
> >
> > 1. Call of firmware switch HW reset was missing, and is required
> >    in order to make the firmware loader shift to the correct state
> >    needed for loading the next firmware.
> > 2. Time-out for waiting for firmware loader to be ready was too small.
> > 3. memory referencing after freeing
> > 4. MAC addresses wrapping
> > 5. Missing SFP unbind (phylink release) of a port during the port release.
> 
> I don't see any problems with 3-5. Maybe post them independent of the first
> 2 and they can be merged.
> 
> Are you really saying it is impossible to determine if the hardware is in the
> boot loader waiting for firmware, or is running the firmware?
> 
>    Andrew

Originally, the pain point for Kory was the rmmod + insmod re-probing failure,
Which is only fixed by the first two commits, so I see little point in submitting 3-5 alone,
Without fixing Kory's problem.

The problem is not with the hardware, but with the existing firmware code on the
Firmware cpu, most probably secure-boot protected, which lacks the ABIs to report to
The kernel what is loaded, what version, what state, etc.

I agree that with better original design, we could have made a better work of avoiding
This lengthy reload, but at this point, I believe my options are quite limited here, unfortunately.

Elad.

Re: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Andrew Lunn]

> Originally, the pain point for Kory was the rmmod + insmod re-probing failure,
> Which is only fixed by the first two commits, so I see little point in submitting 3-5 alone,
> Without fixing Kory's problem.

I thought Kory's problem was actually EPROBE_DEFER? The resources
needed for the PoE are not available, so probing the switch needs to
happen again later, when PoE can get the resources it needs.

But if that is going to take 30 seconds, i'm not sure we can call
EPROBE_DEFER solved.

The later patches are pretty simple, don't need discussion, so could
be merged. However, i think we need to explore different possible
solutions for firmware {re}loading.

> The problem is not with the hardware, but with the existing firmware code on the
> Firmware cpu, most probably secure-boot protected, which lacks the ABIs to report to
> The kernel what is loaded, what version, what state, etc.

Can you at least tell if it is running firmware?

Can you explain the boot in a bit more detail. Are you saying it could
be running an old firmware when the driver first loads? So you need to
hit it with a reset in order to load the firmware for /lib/firmware,
which might be newer than what it is already running?

That would imply the device has FLASH and has a copy of firmware in
it? And if that is true, i think that also implies you have no way to
upgrade the image in FLASH? Otherwise you would implement "devlink
flash" to allow it to be upgraded. You then would not need to load the
firmware on driver probe....

	Andrew

RE: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Elad Nachman]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, March 21, 2024 9:22 PM
> To: Elad Nachman <enachman@marvell.com>
> Cc: Taras Chornyi <taras.chornyi@plvision.eu>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> kory.maincent@bootlin.com; thomas.petazzoni@bootlin.com;
> miquel.raynal@bootlin.com; przemyslaw.kitszel@intel.com;
> dkirjanov@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe
> twice
> 
> > Originally, the pain point for Kory was the rmmod + insmod re-probing
> > failure, Which is only fixed by the first two commits, so I see little
> > point in submitting 3-5 alone, Without fixing Kory's problem.
> 
> I thought Kory's problem was actually EPROBE_DEFER? The resources needed
> for the PoE are not available, so probing the switch needs to happen again
> later, when PoE can get the resources it needs.

No, the PoE is the general high level application where he noted the problem.
There is no PoE code nor special PoE resources in the Prestera driver.
The problem was caused because the module exit was lacking the so called
"switch HW reset" API call which would cause the firmware to exit to the firmware
loader on the firmware CPU, and move to the state in the state machine when
it can receive new firmware from the host CPU (running the Prestera switchDev
driver).

> 
> But if that is going to take 30 seconds, i'm not sure we can call EPROBE_DEFER
> solved.
> 
> The later patches are pretty simple, don't need discussion, so could be
> merged. However, i think we need to explore different possible solutions for
> firmware {re}loading.
> 
> > The problem is not with the hardware, but with the existing firmware
> > code on the Firmware cpu, most probably secure-boot protected, which
> > lacks the ABIs to report to The kernel what is loaded, what version, what
> state, etc.
> 
> Can you at least tell if it is running firmware?

There is no existing API/ABI for that.

> 
> Can you explain the boot in a bit more detail. Are you saying it could be
> running an old firmware when the driver first loads? So you need to hit it with

Exactly.

> a reset in order to load the firmware for /lib/firmware, which might be newer
> than what it is already running?

Right. And there is also the configuration. There is no telling what kind of
Configuration the existing firmware is running. Just using the existing firmware
Will lead to the situation where Linux kernel side will report certain configuration
(via ip link / ip addr / tc , etc.) but the firmware configuration is completely different.
Loading the firmware again will configure the switch to the default setting, making
Sure that the Linux kernel switchDev side is synchronized with the firmware side
And the actual switch configuration.
Unfortunately, there is currently no API/ABI for warm-boot synchronization from
The firmware side to the Kernel switchdev side.

> 
> That would imply the device has FLASH and has a copy of firmware in it? And

Not of the firmware, the flash holds the firmware loader code.
This is a limited functionality code which has only the minimal API/ABI to load
The next firmware.

> if that is true, i think that also implies you have no way to upgrade the image
> in FLASH? Otherwise you would implement "devlink flash" to allow it to be
> upgraded. You then would not need to load the firmware on driver probe....

Right. This is a limitation of the design made. There is no option to upgrade the
Firmware loader binary on the flash, and many boards have it in secure boot,
Which means it cannot be upgraded without bricking the firmware 
CPU loader binary...

> 
> 	Andrew
> 
> 

Re: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Andrew Lunn]

> > > Originally, the pain point for Kory was the rmmod + insmod re-probing
> > > failure, Which is only fixed by the first two commits, so I see little
> > > point in submitting 3-5 alone, Without fixing Kory's problem.
> > 
> > I thought Kory's problem was actually EPROBE_DEFER? The resources needed
> > for the PoE are not available, so probing the switch needs to happen again
> > later, when PoE can get the resources it needs.
> 
> No, the PoE is the general high level application where he noted the problem.
> There is no PoE code nor special PoE resources in the Prestera driver.

So here is Köry email:

https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/#mb898bb2a4bf07776d79f1a19b6a8420716ecb4a3

I don't see why the prestera needs to be involved in PoE itself. It is
just a MAC. PoE happens much lower down in the network stack. Same as
Prestera uses phylink, it does not need to know about the PHYs or the
SFP modules, phylink manages them, not prestera.

> The problem was caused because the module exit was lacking the so called
> "switch HW reset" API call which would cause the firmware to exit to the firmware
> loader on the firmware CPU, and move to the state in the state machine when
> it can receive new firmware from the host CPU (running the Prestera switchDev
> driver).
> 
> > 
> > But if that is going to take 30 seconds, i'm not sure we can call EPROBE_DEFER
> > solved.
> > 
> > The later patches are pretty simple, don't need discussion, so could be
> > merged. However, i think we need to explore different possible solutions for
> > firmware {re}loading.
> > 
> > > The problem is not with the hardware, but with the existing firmware
> > > code on the Firmware cpu, most probably secure-boot protected, which
> > > lacks the ABIs to report to The kernel what is loaded, what version, what
> > state, etc.
> > 
> > Can you at least tell if it is running firmware?
> 
> There is no existing API/ABI for that.

Do you at least have the ability to determine if an API call exists or
not? It sounds like your firmware needs extending to support returning
the version. If the API is missing, you know it is 4.1 or older. If it
does exist, it will return 4.2 or higher.

> > Can you explain the boot in a bit more detail. Are you saying it could be
> > running an old firmware when the driver first loads? So you need to hit it with
> 
> Exactly.
> 
> > a reset in order to load the firmware for /lib/firmware, which might be newer
> > than what it is already running?
> 
> Right. And there is also the configuration. There is no telling what kind of
> Configuration the existing firmware is running. Just using the existing firmware
> Will lead to the situation where Linux kernel side will report certain configuration
> (via ip link / ip addr / tc , etc.) but the firmware configuration is completely different.

Well, during probe and -EPRODE_DEFER, linux has no configuration,
since the driver failed to probe. However, for a rmmod/modprobe, the
firmware could have stale configuration. However pretty much every
device i've come across has the concept of a software reset which
clears out the configuration. Seems to be something else your firmware
is missing.

	Andrew

Re: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Kory Maincent]

On Sun, 24 Mar 2024 16:25:28 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > > Originally, the pain point for Kory was the rmmod + insmod re-probing
> > > > failure, Which is only fixed by the first two commits, so I see little
> > > > point in submitting 3-5 alone, Without fixing Kory's problem.  
> > > 
> > > I thought Kory's problem was actually EPROBE_DEFER? The resources needed
> > > for the PoE are not available, so probing the switch needs to happen again
> > > later, when PoE can get the resources it needs.  
> > 
> > No, the PoE is the general high level application where he noted the
> > problem. There is no PoE code nor special PoE resources in the Prestera
> > driver.  
> 
> So here is Köry email:
> 
> https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/#mb898bb2a4bf07776d79f1a19b6a8420716ecb4a3
> 
> I don't see why the prestera needs to be involved in PoE itself. It is
> just a MAC. PoE happens much lower down in the network stack. Same as
> Prestera uses phylink, it does not need to know about the PHYs or the
> SFP modules, phylink manages them, not prestera.

Prestera is indeed not directly involved in PoE. I wrote a hack to be able to
get the PoE ports control, for testing my PoE patch series.

The aim in the future will be to add RJ45 port abstraction.
The Prestera will get the port abstraction which will get the PoE ports control.
The prestera driver then might receive an EPROBE_DEFER from it.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Andrew Lunn]

> Prestera is indeed not directly involved in PoE. I wrote a hack to be able to
> get the PoE ports control, for testing my PoE patch series.
> 
> The aim in the future will be to add RJ45 port abstraction.
> The Prestera will get the port abstraction which will get the PoE ports control.
> The prestera driver then might receive an EPROBE_DEFER from it.

O.K, so EPRODE_DEFER has to work in a meaningful way. I'm not sure we
can call a 30 second delay meaningful.

And it is not just PoE. phylink_create() can return EPROBE_DEFER, and
there are probably others.

      Andrew

RE: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Elad Nachman]

Hi Andrew,

We have made internal technical review of the issues you have raised (return version API, try to get version API before starting to initialize and load the firmware, clear configuration API) versus the delay saved (almost 30 seconds minus several seconds to perform and complete the API calls) - around 20 seconds or so.

Existing customers we have talked to seem to be able to cope with the existing delay.

Unfortunately, the amount of coding and testing involved with saving these 20 seconds or so is beyond our available development manpower at this specific point in time.

Unfortunately, we will have to defer making the development you have requested to a later period in time.

Elad.


> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Sunday, March 24, 2024 5:25 PM
> To: Elad Nachman <enachman@marvell.com>
> Cc: Taras Chornyi <taras.chornyi@plvision.eu>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> kory.maincent@bootlin.com; thomas.petazzoni@bootlin.com;
> miquel.raynal@bootlin.com; przemyslaw.kitszel@intel.com;
> dkirjanov@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXTERNAL] Re: [PATCH v2 0/5] Fix prestera driver fail to probe
> twice
> 
> > > > Originally, the pain point for Kory was the rmmod + insmod
> > > > re-probing failure, Which is only fixed by the first two commits,
> > > > so I see little point in submitting 3-5 alone, Without fixing Kory's
> problem.
> > >
> > > I thought Kory's problem was actually EPROBE_DEFER? The resources
> > > needed for the PoE are not available, so probing the switch needs to
> > > happen again later, when PoE can get the resources it needs.
> >
> > No, the PoE is the general high level application where he noted the
> problem.
> > There is no PoE code nor special PoE resources in the Prestera driver.
> 
> So here is Köry email:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lore.kernel.org_netdev_20240208101005.29e8c7f3-40kmaincent-2DXPS-
> 2D13-2D7390_T_-
> 23mb898bb2a4bf07776d79f1a19b6a8420716ecb4a3&d=DwIDAw&c=nKjWec2
> b6R0mOyPaz7xtfQ&r=eTeNTLEK5-
> TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=SD1MhKC11sFmp4Q8l76N_DgGdac
> 4aMCTdPsa7Pofb73HEqAGtJ-1p0-
> etIyyldC7&s=VWat9LPub52H3nUez4itmkpuMipnYD3Ngn-paFC9wd4&e=
> 
> I don't see why the prestera needs to be involved in PoE itself. It is just a MAC.
> PoE happens much lower down in the network stack. Same as Prestera uses
> phylink, it does not need to know about the PHYs or the SFP modules, phylink
> manages them, not prestera.
> 
> > The problem was caused because the module exit was lacking the so
> > called "switch HW reset" API call which would cause the firmware to
> > exit to the firmware loader on the firmware CPU, and move to the state
> > in the state machine when it can receive new firmware from the host
> > CPU (running the Prestera switchDev driver).
> >
> > >
> > > But if that is going to take 30 seconds, i'm not sure we can call
> > > EPROBE_DEFER solved.
> > >
> > > The later patches are pretty simple, don't need discussion, so could
> > > be merged. However, i think we need to explore different possible
> > > solutions for firmware {re}loading.
> > >
> > > > The problem is not with the hardware, but with the existing
> > > > firmware code on the Firmware cpu, most probably secure-boot
> > > > protected, which lacks the ABIs to report to The kernel what is
> > > > loaded, what version, what
> > > state, etc.
> > >
> > > Can you at least tell if it is running firmware?
> >
> > There is no existing API/ABI for that.
> 
> Do you at least have the ability to determine if an API call exists or not? It
> sounds like your firmware needs extending to support returning the version.
> If the API is missing, you know it is 4.1 or older. If it does exist, it will return
> 4.2 or higher.
> 
> > > Can you explain the boot in a bit more detail. Are you saying it
> > > could be running an old firmware when the driver first loads? So you
> > > need to hit it with
> >
> > Exactly.
> >
> > > a reset in order to load the firmware for /lib/firmware, which might
> > > be newer than what it is already running?
> >
> > Right. And there is also the configuration. There is no telling what
> > kind of Configuration the existing firmware is running. Just using the
> > existing firmware Will lead to the situation where Linux kernel side
> > will report certain configuration (via ip link / ip addr / tc , etc.) but the
> firmware configuration is completely different.
> 
> Well, during probe and -EPRODE_DEFER, linux has no configuration, since the
> driver failed to probe. However, for a rmmod/modprobe, the firmware could
> have stale configuration. However pretty much every device i've come across
> has the concept of a software reset which clears out the configuration. Seems
> to be something else your firmware is missing.
> 
> 	Andrew

Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Kory Maincent]

On Wed, 20 Mar 2024 19:20:03 +0200
Elad Nachman <enachman@marvell.com> wrote:

> From: Elad Nachman <enachman@marvell.com>
> 
> Fix issues resulting from insmod, rmmod and insmod of the
> prestera driver:

Please add "net" prefixes to all your patches subject, like that:
[PATCH net v2 x/5]

I think the maintainers bots won't works if you don't.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v2 0/5] Fix prestera driver fail to probe twice

[Jakub Kicinski]

On Thu, 21 Mar 2024 10:06:00 +0100 Kory Maincent wrote:
> > Fix issues resulting from insmod, rmmod and insmod of the
> > prestera driver:  
> 
> Please add "net" prefixes to all your patches subject, like that:
> [PATCH net v2 x/5]
> 
> I think the maintainers bots won't works if you don't.

They will default to net-next, which right now is perfectly fine.