| Message ID | 20240311135112.2642491-2-enachman@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix prestera driver fail to probe twice | expand |
On Mon, Mar 11, 2024 at 03:51:10PM +0200, Elad Nachman wrote: > From: Elad Nachman <enachman@marvell.com> > > 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 16:51, Elad Nachman wrote: > From: Elad Nachman <enachman@marvell.com> > > 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 <kory.maincent@bootlin.com> > Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/ > Signed-off-by: Elad Nachman <enachman@marvell.com> 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 3/11/24 14:51, Elad Nachman wrote: > From: Elad Nachman <enachman@marvell.com> 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 <kory.maincent@bootlin.com> > Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/ > 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 ++- > 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 <enachman@marvell.com> wrote: > From: Elad Nachman <enachman@marvell.com> > > 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 <kory.maincent@bootlin.com>
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;