| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 11 Mar 2024 15:36:55 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Elad Nachman <enachman@...vell.com>, <taras.chornyi@...ision.eu>,
<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <andrew@...n.ch>, <kory.maincent@...tlin.com>,
<thomas.petazzoni@...tlin.com>, <miquel.raynal@...tlin.com>,
<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] net: marvell: prestera: fix driver reload
On 3/11/24 14:51, Elad Nachman wrote:
> From: Elad Nachman <enachman@...vell.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@...tlin.com>
> Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
> Signed-off-by: Elad Nachman <enachman@...vell.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;
Powered by blists - more mailing lists