Received-SPF: pass (google.com: domain of linux-kernel+bounces-98974-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <eb09593e-340f-41e0-b7a2-acfdb7d6fc3c@intel.com>
Date: Mon, 11 Mar 2024 15:36:55 +0100
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: Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <20240311135112.2642491-2-enachman@marvell.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WnhoQTNTNU8ybUxhWTRjUVJNZFRrV0xrSXdmVVVKSnFxdDl2MWhuRE1JNFRs?=
 =?utf-8?B?M29ZdmIvOUh0R281MGVmMGx1dFVzZy9JZENTdUlDMlpUTm41MlpBWUFMeTVv?=
 =?utf-8?B?NS9tVWZOd0JuWTBYVXQ3a1FjT0ZPUmxRckpNTXYzM1hhMWllN0VZN0tpeWtI?=
 =?utf-8?B?T2lkMnR2NjNldlZpTUlpd1JYMGl4anRRWVJsTFlNYmJoaTRza2ZXRzY5TjB6?=
 =?utf-8?B?RVZRSE9JSFZJMGwzL3RYYzBaQjFpc1hQTStsOTJLRnE3L1VlOXphc1Q1bFBO?=
 =?utf-8?B?K1Axai9CUGNjbmRNb3N1bnZQeVJpNi9jMWJwWkhlQ3RiWFFlUjVIVlorZWRP?=
 =?utf-8?B?emZXTVdjYlhvc0ltMEZ6N3VlY3FoaUdVR3p6WS9vdnJWRjFXM0sxQmdOOGJk?=
 =?utf-8?B?azRDZ1pHNW1IUGhMMXBrc2ZydzRBT2FkeU5TeEtJZVc3OXpDNFlBRkY1Y1dC?=
 =?utf-8?B?dVE2QnVVWFljcGlyWVM0NFhPTUgzMVlNei9ra2xZa093VWJ3R2lHQjdMOVRM?=
 =?utf-8?B?Wjg1R1JjeUNyWXd2aFdtekFwSlBVMlQ2NlFwQitsZGhwd1kvb2dONHRaYXJJ?=
 =?utf-8?B?KzRybzlwQitXYTBBK0ZrTFRoa2RjM1R4Nzl0dkNlQzdLUUtZYU8wWXhHS1kw?=
 =?utf-8?B?NFZ5QnhiU1pubEVYZ2NzVklkRVd0QXdzRXhhRFVWcllXMUE3dTIzRFM0Z1M5?=
 =?utf-8?B?MDZUM1lqMDZ2K0pqWlBLcVF0Y0ZzS2VNd3BIemRteUZJRDF3TGJzWU5zMXFI?=
 =?utf-8?B?ay9vTi9BODVEREI1S0tKaUowZkZJSnJCNHI2YVJOMGVZckRON1RleEVoR1dM?=
 =?utf-8?B?NWlZVjEwY1NhdStVendTMXRTWnZEbUs4QTFCVVNrRTBZSWJyOEVwbzQ3d2Er?=
 =?utf-8?B?YWxxU3NKWUJZQkVUMGdmekkvTVVub1ZWeS9yeXBYTG1uaS83cDkvUjJsZWN1?=
 =?utf-8?B?S2pXUndqZXJaZ1ZaZnQ4b3ZmYkhLeThzSHhJTEZ6VDlTd0w3d0dMZkV3QTVr?=
 =?utf-8?B?REFubk80RktBa09kTjV1M3czRnBsejl5NkpKdUxkYjI5NnVvdTAzanJvYndr?=
 =?utf-8?B?SmF5OVhkWldzTlR1SmRhanI3Y1NJZzI5VklXVnlNTWc0OCtLUmZESEJRaXVr?=
 =?utf-8?B?MTN0UWVmejEyUUpIVFJManBXenNkcHJ4NUwzTWNFR1htb2RzelJtekwrVzkr?=
 =?utf-8?B?WVF1SkNNTWtsOThXY3dQaXk2cjdMZ3RQL1FOL2hibC9qemdJUmlnSUswVTJh?=
 =?utf-8?B?d1lDdVh3bThCSVJVUUgxeVlMUytJaVFwbWF6bytKKzBpeEpQNUYxYUswWTlw?=
 =?utf-8?B?akxrajBVUXFwV3lDcXpqUStnZHlTV1NKUWRYTHF6MHp2LzY0K2IvWWJvZjlD?=
 =?utf-8?B?TFJvYmhLc3k3Ykw2SDQwd2JlT2tRMWtFMEpNaVlONy8yL3dRa1B4UEtncXRw?=
 =?utf-8?B?b25aWmlUNStYOE9pM3BrSWErb0o2eXJ1TXJGN3lKNXNMN2lteE1qaEI5Ui9i?=
 =?utf-8?B?OXEvL2VZRXpaT0g5ZWhiZ2h4K3cwWFcxYmV3QmJYRXE2MkNWa3pBRjIxZE56?=
 =?utf-8?B?SVRwVzlDaG9ocUxsZ3U4MWYvN01Jd2xRSmRiUWUvUW44NVRtclE5K3RwUE5S?=
 =?utf-8?B?MFBvUVFYNHFUYnc4VERXNWtxOFUvWjV2NUJ2VmFsN0xOZktqVkplamtFN016?=
 =?utf-8?B?dTh3SVRSVGFodTc5NThaWmhHL0hYYkFDbXRjcnJQb1NQaXRUVGF4ajRReThK?=
 =?utf-8?B?N0dvT1U0d0xWVlFrWUhqcHB4OUl3RUNEeTRaUzN0RGdCaGIvRnBuMGJaU2ND?=
 =?utf-8?B?eGNvRjh3NG9pMG9qWU13N041STV4OEswWi9GZmVFQ3JqNFgzOXZyUkpFa3dk?=
 =?utf-8?B?MUdrSmVEUVpJYUxhRzdYdDJmb3Yrc2NtTXhoejJ1bVN3UFIvcUpUdnhOSERQ?=
 =?utf-8?B?a0lZelcyOXhRTk1NbVEyZ2orN2N2NU1rRGVudVY1L1Rtb1c0aHdEMWhYT2NO?=
 =?utf-8?B?bEhaZ0I2Kyt1dVhWSUpyVkRpSkdOb2dnbkV3SkpBU3hWaDkrbUlQZGp4K3Nn?=
 =?utf-8?B?WE9Ca0lPdFdKSkV4NFdqdzZ3cE1ONFdpcWZkb1JPclhXQ3JHTDJFZHNLV3RB?=
 =?utf-8?B?TjZrcUhyYTl4NmR6U2J6MUVVOWc4NERkcHg3UERxYmxLSWhlSWJYWXJBWEJR?=
 =?utf-8?Q?RY1B4zpbGMDEBF81lKq5sGs=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c4abfbe-5b8f-4986-83d5-08dc41d8b7c9
X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8102.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2024 14:37:03.3573
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vH6nkiCBZwl/XHdGiQ5rcQodIUYMFv6/nDzZSV3/HNonM5oHja3R5/xYRAFjv7H5MsjTi9rY/eVTnIBsX/rOxbykOoptbCHb0sN7WNOQQJc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5266
X-OriginatorOrg: 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;