| Message ID | 20240320172008.2989693-2-enachman@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix prestera driver fail to probe twice | expand |
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
> -----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.
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); }