Received-SPF: pass (google.com: domain of linux-kernel+bounces-98963-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <48eb38be-41ea-4ce7-99b2-f26b328cdf19@suse.de>
Date: Mon, 11 Mar 2024 17:30:26 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] net: marvell: prestera: fix driver reload
Content-Language: en-US
To: Elad Nachman <enachman@marvell.com>, taras.chornyi@plvision.eu,
 davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
 pabeni@redhat.com, andrew@lunn.ch, kory.maincent@bootlin.com,
 thomas.petazzoni@bootlin.com, miquel.raynal@bootlin.com,
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240311135112.2642491-1-enachman@marvell.com>
 <20240311135112.2642491-2-enachman@marvell.com>
From: Denis Kirjanov <dkirjanov@suse.de>
In-Reply-To: <20240311135112.2642491-2-enachman@marvell.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


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;