From: Elad Nachman <[email protected]>
Fix issues resulting from insmod, rmmod and insmod of the
prestera driver:
1. Time-out
2. memory referencing after freeing
3. MAC addresses wrapping
Elad Nachman (3):
net: marvell: prestera: fix driver reload
net: marvell: prestera: fix memory use after free
net: marvell: prestera: force good base mac
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 | 7 +++++--
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, 20 insertions(+), 5 deletions(-)
--
2.25.1
From: Elad Nachman <[email protected]>
Prestera driver routing module cleanup process would
release memory and then reference it again, and eventually
free it again.
Remove the redundant first memory free call.
All such double free calls were detected using KASAN.
Signed-off-by: Elad Nachman <[email protected]>
---
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
From: Elad Nachman <[email protected]>
Driver rmmod after insmod would fail because of
the following issues:
1. API call to reset the switch HW and restart the
firmware CPU firmware code loading was missing in
driver removal code handler.
2. Timeout waiting for the firmware CPU firmware
loader code to start was too small.
Fix by adding API call to reset the switch HW and
restart the firmware CPU firmware code loading when
handling the driver removal procedure,
increase the timeout waiting for this restart operation
from 5 to 30 seconds.
Reported-by: Köry Maincent <[email protected]>
Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
Signed-off-by: Elad Nachman <[email protected]>
---
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 ++-
drivers/net/ethernet/marvell/prestera/prestera_pci.c | 7 ++++++-
4 files changed, 17 insertions(+), 2 deletions(-)
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);
}
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
index 35857dc19542..b75a263ad8cb 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 prestera 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_SECS 30
#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_SECS * MSEC_PER_SEC);
if (err) {
dev_err(fw->dev.dev, "waiting for FW loader is timed out");
return err;
--
2.25.1
From: Elad Nachman <[email protected]>
Since each switchport MAC address uses the switch base mac address
and adds the physical port number to it,
Force the last byte of the switch base mac address to be at
least 128, so when adding to it, we will not wrap around the
previous (more significant) mac address byte, resulting in a
warning message.
Signed-off-by: Elad Nachman <[email protected]>
---
drivers/net/ethernet/marvell/prestera/prestera_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index bcaa8ea27b49..e17b1a24fe18 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -859,7 +859,9 @@ static int prestera_switch_set_base_mac_addr(struct prestera_switch *sw)
if (sw->np)
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);
+ do {
+ eth_random_addr(sw->base_mac);
+ } while (sw->base_mac[5] > 0x80);
dev_info(prestera_dev(sw), "using random base mac address\n");
}
--
2.25.1
On Mon, Mar 11, 2024 at 03:51:10PM +0200, Elad Nachman wrote:
> From: Elad Nachman <[email protected]>
>
> Driver rmmod after insmod would fail because of
> the following issues:
>
> 1. API call to reset the switch HW and restart the
> firmware CPU firmware code loading was missing in
> driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
> loader code to start was too small.
>
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when
> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.
The commit messages suggests this does two things. So it should be two
patches.
Please could you explain why the firmware needs resetting during
shutdown. That is odd, so should be explained. The commit message is
about Why?, not What?
Andrew
On 3/11/24 14:51, Elad Nachman wrote:
> From: Elad Nachman <[email protected]>
>
> Since each switchport MAC address uses the switch base mac address
> and adds the physical port number to it,
> Force the last byte of the switch base mac address to be at
> least 128, so when adding to it, we will not wrap around the
to be at *most* 128
> previous (more significant) mac address byte, resulting in a
> warning message.
>
> Signed-off-by: Elad Nachman <[email protected]>
> ---
> drivers/net/ethernet/marvell/prestera/prestera_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> index bcaa8ea27b49..e17b1a24fe18 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> @@ -859,7 +859,9 @@ static int prestera_switch_set_base_mac_addr(struct prestera_switch *sw)
> if (sw->np)
> 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);
> + do {
> + eth_random_addr(sw->base_mac);
> + } while (sw->base_mac[5] > 0x80);
Instead of this loop, that uses 6 bytes of random data at each step,
I would just fix the last byte.
Either by calling get_random_u8() in a loop, or perhaps better, just
toggle of MSB unconditionally:
sw->base_mac[5] &= ~0x80; // or '&= 127'
what would change your condition in commit message to "to be at most
127", but I think that should be fine, granted simpler code.
> dev_info(prestera_dev(sw), "using random base mac address\n");
> }
>
On 3/11/24 16:51, Elad Nachman wrote:
> From: Elad Nachman <[email protected]>
>
> Driver rmmod after insmod would fail because of
> the following issues:
>
> 1. API call to reset the switch HW and restart the
> firmware CPU firmware code loading was missing in
> driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
> loader code to start was too small.
>
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when
> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.
>
> Reported-by: Köry Maincent <[email protected]>
> Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
> Signed-off-by: Elad Nachman <[email protected]>
Please add Fixes tag
> ---
> 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 ++-
> drivers/net/ethernet/marvell/prestera/prestera_pci.c | 7 ++++++-
> 4 files changed, 17 insertions(+), 2 deletions(-)
>
> 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);
> }
>
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> index 35857dc19542..b75a263ad8cb 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 prestera 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_SECS 30
> #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_SECS * MSEC_PER_SEC);
> if (err) {
> dev_err(fw->dev.dev, "waiting for FW loader is timed out");
> return err;
On Mon, Mar 11, 2024 at 03:51:12PM +0200, Elad Nachman wrote:
> From: Elad Nachman <[email protected]>
>
> Since each switchport MAC address uses the switch base mac address
> and adds the physical port number to it,
> Force the last byte of the switch base mac address to be at
> least 128, so when adding to it, we will not wrap around the
> previous (more significant) mac address byte, resulting in a
> warning message.
It is not clear to me what the real problem is here. Does the hardware
require that the MAC addresses always have the same upper 5 bytes?
Andrew
On 3/11/24 14:51, Elad Nachman wrote:
> From: Elad Nachman <[email protected]>
Just a note, not an issue for me:
it's unusual to have "From:" line about yourself, especially that
you use the very same address that your "Sender", and your Signoff.
>
> Driver rmmod after insmod would fail because of
> the following issues:
>
> 1. API call to reset the switch HW and restart the
> firmware CPU firmware code loading was missing in
> driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
> loader code to start was too small.
>
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when
'firmware CPU firmware' sounds off;
you could also reflow your commit message to use up to 75 chars,
instead of shorter lines.
> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.
>
> Reported-by: Köry Maincent <[email protected]>
> Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
> Signed-off-by: Elad Nachman <[email protected]>
> ---
> 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 ++-
> drivers/net/ethernet/marvell/prestera/prestera_pci.c | 7 ++++++-
> 4 files changed, 17 insertions(+), 2 deletions(-)
>
> 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);
> }
>
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> index 35857dc19542..b75a263ad8cb 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 prestera firmware CPU to reboot and
it's implicit in your driver that firmware is "prestera firmware"
> + * restart the firmware loading software layer, hence becoming
> + * ready for the next firmware downloading phase:
> + */
> +#define PRESTERA_FW_READY_TIMEOUT_SECS 30
> #define PRESTERA_FW_DL_TIMEOUT_MS 50000
nice that you have separate timeout for Download and
BeReadyToStartDownload ;)
it would be less typing and more consistent to have 30000 and _MS
suffix, but that's just a nitpick
> #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_SECS * MSEC_PER_SEC);
> if (err) {
> dev_err(fw->dev.dev, "waiting for FW loader is timed out");
> return err;
On Mon, 11 Mar 2024 15:51:10 +0200
Elad Nachman <[email protected]> wrote:
> From: Elad Nachman <[email protected]>
>
> Driver rmmod after insmod would fail because of
> the following issues:
>
> 1. API call to reset the switch HW and restart the
> firmware CPU firmware code loading was missing in
> driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
> loader code to start was too small.
>
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when
> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.
Tested-by: Kory Maincent <[email protected]>
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
On Mon, 11 Mar 2024 15:51:11 +0200
Elad Nachman <[email protected]> wrote:
> From: Elad Nachman <[email protected]>
>
> Prestera driver routing module cleanup process would
> release memory and then reference it again, and eventually
> free it again.
> Remove the redundant first memory free call.
> All such double free calls were detected using KASAN.
Not directly related to this patch but I am wondering if
the call to prestera_port_sfp_unbind(port) is not missing in
prestera_destroy_ports() function?
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
> -----Original Message-----
> From: Kory Maincent <[email protected]>
> Sent: Tuesday, March 12, 2024 5:45 PM
> To: Elad Nachman <[email protected]>
> Cc: Taras Chornyi <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: [EXTERNAL] Re: [PATCH 2/3] net: marvell: prestera: fix memory use
> after free
>
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
>
> ----------------------------------------------------------------------
> On Mon, 11 Mar 2024 15:51:11 +0200
> Elad Nachman <[email protected]> wrote:
>
> > From: Elad Nachman <[email protected]>
> >
> > Prestera driver routing module cleanup process would release memory
> > and then reference it again, and eventually free it again.
> > Remove the redundant first memory free call.
> > All such double free calls were detected using KASAN.
>
> Not directly related to this patch but I am wondering if the call to
> prestera_port_sfp_unbind(port) is not missing in
> prestera_destroy_ports() function?
Yes, it is.
>
> Regards,
> --
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__bootlin.com&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-
> TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=VnjwUBq4QS_Onl_07gp1OUp2XoG
> pT-
> bZ5snEOAg5gf3CM3l5GPgXQ1pb3GzEa6bb&s=GchzqI3lHyGuFKhepEFMFsXwp
> 5oMSH9iqwBZuTtzhjc&e=
Elad.