diff mbox series

[net-next,2/5] net: txgbe: Initialize service task

Message ID 20221108111907.48599-3-mengyuanlou@net-swift.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: WangXun ethernet drivers | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
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 warning 4 maintainers not CCed: davem@davemloft.net pabeni@redhat.com edumazet@google.com kuba@kernel.org
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, 227 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Mengyuan Lou Nov. 8, 2022, 11:19 a.m. UTC
From: Jiawen Wu <jiawenwu@trustnetic.com>

Setup work queue, and initialize service task to process the following
tasks.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/txgbe/txgbe.h    |  15 +++
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 113 +++++++++++++++++-
 2 files changed, 124 insertions(+), 4 deletions(-)

Comments

Jakub Kicinski Nov. 8, 2022, 11:55 p.m. UTC | #1
On Tue,  8 Nov 2022 19:19:04 +0800 Mengyuan Lou wrote:
> +	__TXGBE_TESTING,
> +	__TXGBE_RESETTING,
> +	__TXGBE_DOWN,
> +	__TXGBE_HANGING,
> +	__TXGBE_DISABLED,
> +	__TXGBE_REMOVING,
> +	__TXGBE_SERVICE_SCHED,
> +	__TXGBE_SERVICE_INITED,

Please don't try to implement a state machine in the driver.
Protect data structures with locks, like a normal piece of SW.
Jiawen Wu Nov. 9, 2022, 2:16 a.m. UTC | #2
On Wednesday, November 9, 2022 7:56 AM, Jakub wrote:
> On Tue,  8 Nov 2022 19:19:04 +0800 Mengyuan Lou wrote:
> > +	__TXGBE_TESTING,
> > +	__TXGBE_RESETTING,
> > +	__TXGBE_DOWN,
> > +	__TXGBE_HANGING,
> > +	__TXGBE_DISABLED,
> > +	__TXGBE_REMOVING,
> > +	__TXGBE_SERVICE_SCHED,
> > +	__TXGBE_SERVICE_INITED,
> 
> Please don't try to implement a state machine in the driver.
> Protect data structures with locks, like a normal piece of SW.
> 

The state machine will be used in interrupt events, locks don't seem to fit it.
Andrew Lunn Nov. 9, 2022, 2:36 a.m. UTC | #3
On Wed, Nov 09, 2022 at 10:16:32AM +0800, Jiawen Wu wrote:
> On Wednesday, November 9, 2022 7:56 AM, Jakub wrote:
> > On Tue,  8 Nov 2022 19:19:04 +0800 Mengyuan Lou wrote:
> > > +	__TXGBE_TESTING,
> > > +	__TXGBE_RESETTING,
> > > +	__TXGBE_DOWN,
> > > +	__TXGBE_HANGING,
> > > +	__TXGBE_DISABLED,
> > > +	__TXGBE_REMOVING,
> > > +	__TXGBE_SERVICE_SCHED,
> > > +	__TXGBE_SERVICE_INITED,
> > 
> > Please don't try to implement a state machine in the driver.
> > Protect data structures with locks, like a normal piece of SW.
> > 
> 
> The state machine will be used in interrupt events, locks don't seem to fit it.

spinlock can be used with interrupts.

Also, once you make use of phylink, you might not need any of this.

	 Andrew
Alexander Lobakin Nov. 14, 2022, 3:39 p.m. UTC | #4
From: Mengyuan Lou <mengyuanlou@net-swift.com>
Date: Tue,  8 Nov 2022 19:19:04 +0800

> From: Jiawen Wu <jiawenwu@trustnetic.com>
> 
> Setup work queue, and initialize service task to process the following
> tasks.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---

[...]

> @@ -468,6 +524,7 @@ static int txgbe_probe(struct pci_dev *pdev,
>  	}
>  
>  	netdev->netdev_ops = &txgbe_netdev_ops;
> +	netdev->watchdog_timeo = 5 * HZ;

Default value is 5 sec already[0], why...

>  
>  	/* setup the private structure */
>  	err = txgbe_sw_init(adapter);
> @@ -518,6 +575,11 @@ static int txgbe_probe(struct pci_dev *pdev,
>  	eth_hw_addr_set(netdev, wxhw->mac.perm_addr);
>  	txgbe_mac_set_default_filter(adapter, wxhw->mac.perm_addr);

[...]

> +static int __init txgbe_init_module(void)
> +{
> +	int ret;
> +
> +	txgbe_wq = create_singlethread_workqueue(txgbe_driver_name);
> +	if (!txgbe_wq) {
> +		pr_err("%s: Failed to create workqueue\n", txgbe_driver_name);
> +		return -ENOMEM;
> +	}
> +
> +	ret = pci_register_driver(&txgbe_driver);
> +	return ret;
> +}
> +
> +module_init(txgbe_init_module);

I think no empty lines in between the function and module/initcall
init/exit declaration is preferred.

> +
> +/**
> + * txgbe_exit_module - Driver Exit Cleanup Routine
> + *
> + * txgbe_exit_module is called just before the driver is removed
> + * from memory.
> + **/
> +static void __exit txgbe_exit_module(void)
> +{
> +	pci_unregister_driver(&txgbe_driver);
> +	destroy_workqueue(txgbe_wq);
> +}
> +
> +module_exit(txgbe_exit_module);

^

>  
>  MODULE_DEVICE_TABLE(pci, txgbe_pci_tbl);
>  MODULE_AUTHOR("Beijing WangXun Technology Co., Ltd, <software@trustnetic.com>");
> -- 
> 2.38.1

[0] https://elixir.bootlin.com/linux/v6.1-rc5/source/net/sched/sch_generic.c#L547

Thanks,
Olek
diff mbox series

Patch

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe.h b/drivers/net/ethernet/wangxun/txgbe/txgbe.h
index 19e61377bd00..fb8fd413b755 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe.h
@@ -24,6 +24,17 @@  struct txgbe_mac_addr {
 #define TXGBE_MAC_STATE_MODIFIED        0x2
 #define TXGBE_MAC_STATE_IN_USE          0x4
 
+enum txgbe_state_t {
+	__TXGBE_TESTING,
+	__TXGBE_RESETTING,
+	__TXGBE_DOWN,
+	__TXGBE_HANGING,
+	__TXGBE_DISABLED,
+	__TXGBE_REMOVING,
+	__TXGBE_SERVICE_SCHED,
+	__TXGBE_SERVICE_INITED,
+};
+
 /* board specific private data structure */
 struct txgbe_adapter {
 	u8 __iomem *io_addr;    /* Mainly for iounmap use */
@@ -31,6 +42,10 @@  struct txgbe_adapter {
 	struct net_device *netdev;
 	struct pci_dev *pdev;
 
+	unsigned long state;
+	struct timer_list service_timer;
+	struct work_struct service_task;
+
 	/* structs defined in txgbe_type.h */
 	struct txgbe_hw hw;
 	u16 msg_enable;
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 1c00ecbc1c6a..cb86c001baa6 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -35,6 +35,8 @@  static const struct pci_device_id txgbe_pci_tbl[] = {
 
 #define DEFAULT_DEBUG_LEVEL_SHIFT 3
 
+static struct workqueue_struct *txgbe_wq;
+
 static void txgbe_check_minimum_link(struct txgbe_adapter *adapter)
 {
 	struct pci_dev *pdev;
@@ -73,6 +75,50 @@  static int txgbe_enumerate_functions(struct txgbe_adapter *adapter)
 	return physfns;
 }
 
+static void txgbe_service_event_schedule(struct txgbe_adapter *adapter)
+{
+	if (!test_bit(__TXGBE_DOWN, &adapter->state) &&
+	    !test_bit(__TXGBE_REMOVING, &adapter->state) &&
+	    !test_and_set_bit(__TXGBE_SERVICE_SCHED, &adapter->state))
+		queue_work(txgbe_wq, &adapter->service_task);
+}
+
+static void txgbe_service_event_complete(struct txgbe_adapter *adapter)
+{
+	if (WARN_ON(!test_bit(__TXGBE_SERVICE_SCHED, &adapter->state)))
+		return;
+
+	/* flush memory to make sure state is correct before next watchdog */
+	smp_mb__before_atomic();
+	clear_bit(__TXGBE_SERVICE_SCHED, &adapter->state);
+}
+
+static void txgbe_service_timer(struct timer_list *t)
+{
+	struct txgbe_adapter *adapter = from_timer(adapter, t, service_timer);
+	unsigned long next_event_offset;
+
+	next_event_offset = HZ * 2;
+
+	/* Reset the timer */
+	mod_timer(&adapter->service_timer, next_event_offset + jiffies);
+
+	txgbe_service_event_schedule(adapter);
+}
+
+/**
+ * txgbe_service_task - manages and runs subtasks
+ * @work: pointer to work_struct containing our data
+ **/
+static void txgbe_service_task(struct work_struct *work)
+{
+	struct txgbe_adapter *adapter = container_of(work,
+						     struct txgbe_adapter,
+						     service_task);
+
+	txgbe_service_event_complete(adapter);
+}
+
 static void txgbe_sync_mac_table(struct txgbe_adapter *adapter)
 {
 	struct txgbe_hw *hw = &adapter->hw;
@@ -190,6 +236,10 @@  static void txgbe_disable_device(struct txgbe_adapter *adapter)
 	struct net_device *netdev = adapter->netdev;
 	struct wx_hw *wxhw = &adapter->hw.wxhw;
 
+	/* signal that we are down to the interrupt handler */
+	if (test_and_set_bit(__TXGBE_DOWN, &adapter->state))
+		return; /* do nothing if already down */
+
 	wx_disable_pcie_master(wxhw);
 	/* disable receives */
 	wx_disable_rx(wxhw);
@@ -197,6 +247,8 @@  static void txgbe_disable_device(struct txgbe_adapter *adapter)
 	netif_carrier_off(netdev);
 	netif_tx_disable(netdev);
 
+	del_timer_sync(&adapter->service_timer);
+
 	if (wxhw->bus.func < 2)
 		wr32m(wxhw, TXGBE_MIS_PRB_CTL, TXGBE_MIS_PRB_CTL_LAN_UP(wxhw->bus.func), 0);
 	else
@@ -266,6 +318,8 @@  static int txgbe_sw_init(struct txgbe_adapter *adapter)
 		return -ENOMEM;
 	}
 
+	set_bit(__TXGBE_DOWN, &adapter->state);
+
 	return 0;
 }
 
@@ -336,7 +390,8 @@  static void txgbe_dev_shutdown(struct pci_dev *pdev, bool *enable_wake)
 
 	wx_control_hw(wxhw, false);
 
-	pci_disable_device(pdev);
+	if (!test_and_set_bit(__TXGBE_DISABLED, &adapter->state))
+		pci_disable_device(pdev);
 }
 
 static void txgbe_shutdown(struct pci_dev *pdev)
@@ -410,6 +465,7 @@  static int txgbe_probe(struct pci_dev *pdev,
 	struct txgbe_hw *hw = NULL;
 	struct wx_hw *wxhw = NULL;
 	struct net_device *netdev;
+	bool disable_dev = false;
 	int err, expected_gts;
 
 	u16 eeprom_verh = 0, eeprom_verl = 0, offset = 0;
@@ -468,6 +524,7 @@  static int txgbe_probe(struct pci_dev *pdev,
 	}
 
 	netdev->netdev_ops = &txgbe_netdev_ops;
+	netdev->watchdog_timeo = 5 * HZ;
 
 	/* setup the private structure */
 	err = txgbe_sw_init(adapter);
@@ -518,6 +575,11 @@  static int txgbe_probe(struct pci_dev *pdev,
 	eth_hw_addr_set(netdev, wxhw->mac.perm_addr);
 	txgbe_mac_set_default_filter(adapter, wxhw->mac.perm_addr);
 
+	timer_setup(&adapter->service_timer, txgbe_service_timer, 0);
+	INIT_WORK(&adapter->service_task, txgbe_service_task);
+	set_bit(__TXGBE_SERVICE_INITED, &adapter->state);
+	clear_bit(__TXGBE_SERVICE_SCHED, &adapter->state);
+
 	/* Save off EEPROM version number and Option Rom version which
 	 * together make a unique identify for the eeprom
 	 */
@@ -599,11 +661,13 @@  static int txgbe_probe(struct pci_dev *pdev,
 err_free_mac_table:
 	kfree(adapter->mac_table);
 err_pci_release_regions:
+	disable_dev = !test_and_set_bit(__TXGBE_DISABLED, &adapter->state);
 	pci_disable_pcie_error_reporting(pdev);
 	pci_release_selected_regions(pdev,
 				     pci_select_bars(pdev, IORESOURCE_MEM));
 err_pci_disable_dev:
-	pci_disable_device(pdev);
+	if (!adapter || disable_dev)
+		pci_disable_device(pdev);
 	return err;
 }
 
@@ -620,18 +684,25 @@  static void txgbe_remove(struct pci_dev *pdev)
 {
 	struct txgbe_adapter *adapter = pci_get_drvdata(pdev);
 	struct net_device *netdev;
+	bool disable_dev;
 
 	netdev = adapter->netdev;
+
+	set_bit(__TXGBE_REMOVING, &adapter->state);
+	cancel_work_sync(&adapter->service_task);
+
 	unregister_netdev(netdev);
 
 	pci_release_selected_regions(pdev,
 				     pci_select_bars(pdev, IORESOURCE_MEM));
 
 	kfree(adapter->mac_table);
+	disable_dev = !test_and_set_bit(__TXGBE_DISABLED, &adapter->state);
 
 	pci_disable_pcie_error_reporting(pdev);
 
-	pci_disable_device(pdev);
+	if (disable_dev)
+		pci_disable_device(pdev);
 }
 
 static struct pci_driver txgbe_driver = {
@@ -642,7 +713,41 @@  static struct pci_driver txgbe_driver = {
 	.shutdown = txgbe_shutdown,
 };
 
-module_pci_driver(txgbe_driver);
+/**
+ * txgbe_init_module - Driver Registration Routine
+ *
+ * txgbe_init_module is the first routine called when the driver is
+ * loaded. All it does is register with the PCI subsystem.
+ **/
+static int __init txgbe_init_module(void)
+{
+	int ret;
+
+	txgbe_wq = create_singlethread_workqueue(txgbe_driver_name);
+	if (!txgbe_wq) {
+		pr_err("%s: Failed to create workqueue\n", txgbe_driver_name);
+		return -ENOMEM;
+	}
+
+	ret = pci_register_driver(&txgbe_driver);
+	return ret;
+}
+
+module_init(txgbe_init_module);
+
+/**
+ * txgbe_exit_module - Driver Exit Cleanup Routine
+ *
+ * txgbe_exit_module is called just before the driver is removed
+ * from memory.
+ **/
+static void __exit txgbe_exit_module(void)
+{
+	pci_unregister_driver(&txgbe_driver);
+	destroy_workqueue(txgbe_wq);
+}
+
+module_exit(txgbe_exit_module);
 
 MODULE_DEVICE_TABLE(pci, txgbe_pci_tbl);
 MODULE_AUTHOR("Beijing WangXun Technology Co., Ltd, <software@trustnetic.com>");