diff mbox series

[net-next,1/9] octeon_ep: wait for firmware ready

Message ID 20221107072524.9485-2-vburru@marvell.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series octeon_ep: Update PF mailbox for VF | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 229 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Veerasenareddy Burru Nov. 7, 2022, 7:25 a.m. UTC
Make driver initialize the device only after firmware is ready
 - add async device setup routine.
 - poll firmware status register.
 - once firmware is ready, call async device setup routine.

Signed-off-by: Veerasenareddy Burru <vburru@marvell.com>
Signed-off-by: Abhijit Ayarekar <aayarekar@marvell.com>
---
 .../ethernet/marvell/octeon_ep/octep_main.c   | 156 +++++++++++++-----
 .../ethernet/marvell/octeon_ep/octep_main.h   |  15 ++
 2 files changed, 133 insertions(+), 38 deletions(-)

Comments

Leon Romanovsky Nov. 7, 2022, 8:18 a.m. UTC | #1
On Sun, Nov 06, 2022 at 11:25:15PM -0800, Veerasenareddy Burru wrote:
> Make driver initialize the device only after firmware is ready
>  - add async device setup routine.
>  - poll firmware status register.
>  - once firmware is ready, call async device setup routine.

Please don't do it. It is extremely hard to do it right. The proposed
code that has combination of atomics used as a locks together with absence
of proper locking from PCI and driver cores supports my claim.

Thanks
Veerasenareddy Burru Nov. 14, 2022, 8:47 p.m. UTC | #2
> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Monday, November 7, 2022 12:19 AM
> To: Veerasenareddy Burru <vburru@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Liron Himi
> <lironh@marvell.com>; Abhijit Ayarekar <aayarekar@marvell.com>; Sathesh
> B Edara <sedara@marvell.com>; Satananda Burla <sburla@marvell.com>;
> linux-doc@vger.kernel.org; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>
> Subject: [EXT] Re: [PATCH net-next 1/9] octeon_ep: wait for firmware ready
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Sun, Nov 06, 2022 at 11:25:15PM -0800, Veerasenareddy Burru wrote:
> > Make driver initialize the device only after firmware is ready
> >  - add async device setup routine.
> >  - poll firmware status register.
> >  - once firmware is ready, call async device setup routine.
> 
> Please don't do it. It is extremely hard to do it right. The proposed code that
> has combination of atomics used as a locks together with absence of proper
> locking from PCI and driver cores supports my claim.
> 
> Thanks
Leon
          What is the alternate approach you suggest here ?  Are you suggesting usage of deferred probe ? the driver initialization cannot proceed till firmware ready is set by firmware.
Thanks
Leon Romanovsky Nov. 15, 2022, 9:04 a.m. UTC | #3
On Mon, Nov 14, 2022 at 08:47:42PM +0000, Veerasenareddy Burru wrote:
> 
> 
> > -----Original Message-----
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Monday, November 7, 2022 12:19 AM
> > To: Veerasenareddy Burru <vburru@marvell.com>
> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Liron Himi
> > <lironh@marvell.com>; Abhijit Ayarekar <aayarekar@marvell.com>; Sathesh
> > B Edara <sedara@marvell.com>; Satananda Burla <sburla@marvell.com>;
> > linux-doc@vger.kernel.org; David S. Miller <davem@davemloft.net>; Eric
> > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> > Paolo Abeni <pabeni@redhat.com>
> > Subject: [EXT] Re: [PATCH net-next 1/9] octeon_ep: wait for firmware ready
> > 
> > External Email
> > 
> > ----------------------------------------------------------------------
> > On Sun, Nov 06, 2022 at 11:25:15PM -0800, Veerasenareddy Burru wrote:
> > > Make driver initialize the device only after firmware is ready
> > >  - add async device setup routine.
> > >  - poll firmware status register.
> > >  - once firmware is ready, call async device setup routine.
> > 
> > Please don't do it. It is extremely hard to do it right. The proposed code that
> > has combination of atomics used as a locks together with absence of proper
> > locking from PCI and driver cores supports my claim.
> > 
> > Thanks
> Leon
>           What is the alternate approach you suggest here ?  Are you suggesting usage of deferred probe ? the driver initialization cannot proceed till firmware ready is set by firmware.

This is what I initially thought.

Thanks

> Thanks
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 1cbfa800a8af..488159217030 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -867,6 +867,14 @@  static const struct net_device_ops octep_netdev_ops = {
 	.ndo_change_mtu          = octep_change_mtu,
 };
 
+/* Cancel all tasks except hb task */
+static void cancel_all_tasks(struct octep_device *oct)
+{
+	cancel_work_sync(&oct->tx_timeout_task);
+	cancel_work_sync(&oct->ctrl_mbox_task);
+	octep_ctrl_mbox_uninit(&oct->ctrl_mbox);
+}
+
 /**
  * octep_ctrl_mbox_task - work queue task to handle ctrl mbox messages.
  *
@@ -979,6 +987,9 @@  int octep_device_setup(struct octep_device *oct)
 							   ctrl_mbox->f2hq.elem_sz,
 							   ctrl_mbox->f2hq.elem_cnt);
 
+	INIT_WORK(&oct->tx_timeout_task, octep_tx_timeout_task);
+	INIT_WORK(&oct->ctrl_mbox_task, octep_ctrl_mbox_task);
+
 	return 0;
 
 unsupported_dev:
@@ -1003,7 +1014,7 @@  static void octep_device_cleanup(struct octep_device *oct)
 		oct->mbox[i] = NULL;
 	}
 
-	octep_ctrl_mbox_uninit(&oct->ctrl_mbox);
+	cancel_all_tasks(oct);
 
 	oct->hw_ops.soft_reset(oct);
 	for (i = 0; i < OCTEP_MMIO_REGIONS; i++) {
@@ -1015,6 +1026,92 @@  static void octep_device_cleanup(struct octep_device *oct)
 	oct->conf = NULL;
 }
 
+static u8 get_fw_ready_status(struct octep_device *oct)
+{
+	u32 pos = 0;
+	u16 vsec_id;
+	u8 status = 0;
+
+	while ((pos = pci_find_next_ext_capability(oct->pdev, pos,
+						   PCI_EXT_CAP_ID_VNDR))) {
+		pci_read_config_word(oct->pdev, pos + 4, &vsec_id);
+#define FW_STATUS_VSEC_ID  0xA3
+		if (vsec_id == FW_STATUS_VSEC_ID) {
+			pci_read_config_byte(oct->pdev, (pos + 8), &status);
+			dev_info(&oct->pdev->dev, "Firmware ready %u\n",
+				 status);
+			return status;
+		}
+	}
+	return 0;
+}
+
+/**
+ * octep_dev_setup_task - work queue task to setup octep device.
+ *
+ * @work: pointer to dev setup work_struct
+ *
+ * Wait for firmware to be ready, then continue with device setup.
+ * Check for module exit while waiting for firmware.
+ *
+ **/
+static void octep_dev_setup_task(struct work_struct *work)
+{
+	struct octep_device *oct = container_of(work, struct octep_device,
+						dev_setup_task);
+	struct net_device *netdev = oct->netdev;
+	u8 status;
+	int err;
+
+	atomic_set(&oct->status, OCTEP_DEV_STATUS_WAIT_FOR_FW);
+	while (true) {
+		status = get_fw_ready_status(oct);
+#define FW_STATUS_READY    1
+		if (status == FW_STATUS_READY)
+			break;
+
+		schedule_timeout_interruptible(HZ * 1);
+
+		if (atomic_read(&oct->status) >= OCTEP_DEV_STATUS_READY) {
+			dev_info(&oct->pdev->dev,
+				 "Stopping firmware ready work.\n");
+			return;
+		}
+	}
+
+	/* Do not free resources on failure. driver unload will
+	 * lead to freeing resources.
+	 */
+	atomic_set(&oct->status, OCTEP_DEV_STATUS_INIT);
+	err = octep_device_setup(oct);
+	if (err) {
+		dev_err(&oct->pdev->dev, "Device setup failed\n");
+		atomic_set(&oct->status, OCTEP_DEV_STATUS_ALLOC);
+		return;
+	}
+
+	netdev->netdev_ops = &octep_netdev_ops;
+	octep_set_ethtool_ops(netdev);
+	netif_carrier_off(netdev);
+
+	netdev->hw_features = NETIF_F_SG;
+	netdev->features |= netdev->hw_features;
+	netdev->min_mtu = OCTEP_MIN_MTU;
+	netdev->max_mtu = OCTEP_MAX_MTU;
+	netdev->mtu = OCTEP_DEFAULT_MTU;
+
+	octep_get_mac_addr(oct, oct->mac_addr);
+	eth_hw_addr_set(netdev, oct->mac_addr);
+
+	err = register_netdev(netdev);
+	if (err) {
+		dev_err(&oct->pdev->dev, "Failed to register netdev\n");
+		atomic_set(&oct->status, OCTEP_DEV_STATUS_INIT);
+		return;
+	}
+	atomic_set(&oct->status, OCTEP_DEV_STATUS_READY);
+}
+
 /**
  * octep_probe() - Octeon PCI device probe handler.
  *
@@ -1066,39 +1163,13 @@  static int octep_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	octep_dev->dev = &pdev->dev;
 	pci_set_drvdata(pdev, octep_dev);
 
-	err = octep_device_setup(octep_dev);
-	if (err) {
-		dev_err(&pdev->dev, "Device setup failed\n");
-		goto err_octep_config;
-	}
-	INIT_WORK(&octep_dev->tx_timeout_task, octep_tx_timeout_task);
-	INIT_WORK(&octep_dev->ctrl_mbox_task, octep_ctrl_mbox_task);
+	atomic_set(&octep_dev->status, OCTEP_DEV_STATUS_ALLOC);
+	INIT_WORK(&octep_dev->dev_setup_task, octep_dev_setup_task);
+	queue_work(octep_wq, &octep_dev->dev_setup_task);
+	dev_info(&pdev->dev, "Device setup task queued\n");
 
-	netdev->netdev_ops = &octep_netdev_ops;
-	octep_set_ethtool_ops(netdev);
-	netif_carrier_off(netdev);
-
-	netdev->hw_features = NETIF_F_SG;
-	netdev->features |= netdev->hw_features;
-	netdev->min_mtu = OCTEP_MIN_MTU;
-	netdev->max_mtu = OCTEP_MAX_MTU;
-	netdev->mtu = OCTEP_DEFAULT_MTU;
-
-	octep_get_mac_addr(octep_dev, octep_dev->mac_addr);
-	eth_hw_addr_set(netdev, octep_dev->mac_addr);
-
-	err = register_netdev(netdev);
-	if (err) {
-		dev_err(&pdev->dev, "Failed to register netdev\n");
-		goto register_dev_err;
-	}
-	dev_info(&pdev->dev, "Device probe successful\n");
 	return 0;
 
-register_dev_err:
-	octep_device_cleanup(octep_dev);
-err_octep_config:
-	free_netdev(netdev);
 err_alloc_netdev:
 	pci_disable_pcie_error_reporting(pdev);
 	pci_release_mem_regions(pdev);
@@ -1119,20 +1190,29 @@  static int octep_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 static void octep_remove(struct pci_dev *pdev)
 {
 	struct octep_device *oct = pci_get_drvdata(pdev);
-	struct net_device *netdev;
+	int status;
 
 	if (!oct)
 		return;
 
-	cancel_work_sync(&oct->tx_timeout_task);
-	cancel_work_sync(&oct->ctrl_mbox_task);
-	netdev = oct->netdev;
-	if (netdev->reg_state == NETREG_REGISTERED)
-		unregister_netdev(netdev);
+	dev_info(&pdev->dev, "Removing device.\n");
+	status = atomic_read(&oct->status);
+	if (status <= OCTEP_DEV_STATUS_ALLOC)
+		goto free_resources;
+
+	atomic_set(&oct->status, OCTEP_DEV_STATUS_UNINIT);
+	if (status == OCTEP_DEV_STATUS_WAIT_FOR_FW) {
+		cancel_work_sync(&oct->dev_setup_task);
+		goto free_resources;
+	}
+	if (oct->netdev->reg_state == NETREG_REGISTERED)
+		unregister_netdev(oct->netdev);
 
 	octep_device_cleanup(oct);
+
+free_resources:
 	pci_release_mem_regions(pdev);
-	free_netdev(netdev);
+	free_netdev(oct->netdev);
 	pci_disable_pcie_error_reporting(pdev);
 	pci_disable_device(pdev);
 }
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
index 123ffc13754d..45226282bf21 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
@@ -192,6 +192,16 @@  struct octep_iface_link_info {
 	u8  oper_up;
 };
 
+/* Device status */
+enum octep_dev_status {
+	OCTEP_DEV_STATUS_INVALID,
+	OCTEP_DEV_STATUS_ALLOC,
+	OCTEP_DEV_STATUS_WAIT_FOR_FW,
+	OCTEP_DEV_STATUS_INIT,
+	OCTEP_DEV_STATUS_READY,
+	OCTEP_DEV_STATUS_UNINIT
+};
+
 /* The Octeon device specific private data structure.
  * Each Octeon device has this structure to represent all its components.
  */
@@ -271,6 +281,11 @@  struct octep_device {
 	/* Work entry to handle ctrl mbox interrupt */
 	struct work_struct ctrl_mbox_task;
 
+	/* Work entry to handle device setup */
+	struct work_struct dev_setup_task;
+
+	/* Device status */
+	atomic_t status;
 };
 
 static inline u16 OCTEP_MAJOR_REV(struct octep_device *oct)