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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

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;