| Message ID | 20250105113822.108706-1-demonihin@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,RESEND,net-next] e1000e: makes e1000_watchdog_task use queue_delayed_work | expand |
On 1/5/2025 1:38 PM, Dmitrii Ermakov wrote: > Replaces watchdog timer with delayed_work as advised > in the driver's TODO comment. > > Signed-off-by: Dmitrii Ermakov <demonihin@gmail.com> > --- > V1 -> V2: Removed redundant line wraps, renamed e1000_watchdog to e1000_watchdog_work > > drivers/net/ethernet/intel/e1000e/e1000.h | 4 +-- > drivers/net/ethernet/intel/e1000e/netdev.c | 42 ++++++++-------------- > 2 files changed, 16 insertions(+), 30 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h > index ba9c19e6994c..5a60372d2158 100644 > --- a/drivers/net/ethernet/intel/e1000e/e1000.h > +++ b/drivers/net/ethernet/intel/e1000e/e1000.h > @@ -189,12 +189,12 @@ struct e1000_phy_regs { > > /* board specific private data structure */ > struct e1000_adapter { > - struct timer_list watchdog_timer; > struct timer_list phy_info_timer; > struct timer_list blink_timer; > > + struct delayed_work watchdog_work; > + > struct work_struct reset_task; > - struct work_struct watchdog_task; > > const struct e1000_info *ei; > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index 286155efcedf..cb68662cdc3a 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -1778,7 +1778,7 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data) > } > /* guard against interrupt when we're going down */ > if (!test_bit(__E1000_DOWN, &adapter->state)) > - mod_timer(&adapter->watchdog_timer, jiffies + 1); > + queue_delayed_work(system_wq, &adapter->watchdog_work, 1); > } > > /* Reset on uncorrectable ECC error */ > @@ -1857,7 +1857,7 @@ static irqreturn_t e1000_intr(int __always_unused irq, void *data) > } > /* guard against interrupt when we're going down */ > if (!test_bit(__E1000_DOWN, &adapter->state)) > - mod_timer(&adapter->watchdog_timer, jiffies + 1); > + queue_delayed_work(system_wq, &adapter->watchdog_work, 1); > } > > /* Reset on uncorrectable ECC error */ > @@ -1901,7 +1901,7 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data) > hw->mac.get_link_status = true; > /* guard against interrupt when we're going down */ > if (!test_bit(__E1000_DOWN, &adapter->state)) > - mod_timer(&adapter->watchdog_timer, jiffies + 1); > + queue_delayed_work(system_wq, &adapter->watchdog_work, 1); > } > > if (!test_bit(__E1000_DOWN, &adapter->state)) > @@ -4287,7 +4287,8 @@ void e1000e_down(struct e1000_adapter *adapter, bool reset) > > napi_synchronize(&adapter->napi); > > - del_timer_sync(&adapter->watchdog_timer); > + cancel_delayed_work_sync(&adapter->watchdog_work); > + > del_timer_sync(&adapter->phy_info_timer); > > spin_lock(&adapter->stats64_lock); > @@ -5169,25 +5170,12 @@ static void e1000e_check_82574_phy_workaround(struct e1000_adapter *adapter) > } > } > > -/** > - * e1000_watchdog - Timer Call-back > - * @t: pointer to timer_list containing private info adapter > - **/ > -static void e1000_watchdog(struct timer_list *t) > +static void e1000_watchdog_work(struct work_struct *work) > { > - struct e1000_adapter *adapter = from_timer(adapter, t, watchdog_timer); > - > - /* Do the rest outside of interrupt context */ > - schedule_work(&adapter->watchdog_task); > - > - /* TODO: make this use queue_delayed_work() */ > -} > - > -static void e1000_watchdog_task(struct work_struct *work) > -{ > - struct e1000_adapter *adapter = container_of(work, > - struct e1000_adapter, > - watchdog_task); > + struct delayed_work *dwork = > + container_of(work, struct delayed_work, work); > + struct e1000_adapter *adapter = > + container_of(dwork, struct e1000_adapter, watchdog_work); > struct net_device *netdev = adapter->netdev; > struct e1000_mac_info *mac = &adapter->hw.mac; > struct e1000_phy_info *phy = &adapter->hw.phy; > @@ -5416,8 +5404,8 @@ static void e1000_watchdog_task(struct work_struct *work) > > /* Reset the timer */ > if (!test_bit(__E1000_DOWN, &adapter->state)) > - mod_timer(&adapter->watchdog_timer, > - round_jiffies(jiffies + 2 * HZ)); > + queue_delayed_work(system_wq, &adapter->watchdog_work, > + round_jiffies(2 * HZ)); > } > > #define E1000_TX_FLAGS_CSUM 0x00000001 > @@ -7596,11 +7584,10 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > goto err_eeprom; > } > > - timer_setup(&adapter->watchdog_timer, e1000_watchdog, 0); > timer_setup(&adapter->phy_info_timer, e1000_update_phy_info, 0); > + INIT_DELAYED_WORK(&adapter->watchdog_work, e1000_watchdog_work); > > INIT_WORK(&adapter->reset_task, e1000_reset_task); > - INIT_WORK(&adapter->watchdog_task, e1000_watchdog_task); > INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround); > INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task); > INIT_WORK(&adapter->print_hang_task, e1000_print_hw_hang); > @@ -7741,11 +7728,10 @@ static void e1000_remove(struct pci_dev *pdev) > * from being rescheduled. > */ > set_bit(__E1000_DOWN, &adapter->state); > - del_timer_sync(&adapter->watchdog_timer); > + cancel_delayed_work_sync(&adapter->watchdog_work); > del_timer_sync(&adapter->phy_info_timer); > > cancel_work_sync(&adapter->reset_task); > - cancel_work_sync(&adapter->watchdog_task); > cancel_work_sync(&adapter->downshift_task); > cancel_work_sync(&adapter->update_phy_task); > cancel_work_sync(&adapter->print_hang_task); Hi Dmitrii, I have found that in the past someone has already tried to change delayed work instead of watchdog task (59653e6497d1: e1000e: Make watchdog use delayed work). This resulted in driver crashes and connections to be reset unexpectedly (d5ad7a6a7f3c8: e1000e: Revert "e1000e: Make watchdog use delayed work"). Because of that, and unless there is a clear benefit to using delayed_work, I recommend to reject this patch, as the risk of regression is high.
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h index ba9c19e6994c..5a60372d2158 100644 --- a/drivers/net/ethernet/intel/e1000e/e1000.h +++ b/drivers/net/ethernet/intel/e1000e/e1000.h @@ -189,12 +189,12 @@ struct e1000_phy_regs { /* board specific private data structure */ struct e1000_adapter { - struct timer_list watchdog_timer; struct timer_list phy_info_timer; struct timer_list blink_timer; + struct delayed_work watchdog_work; + struct work_struct reset_task; - struct work_struct watchdog_task; const struct e1000_info *ei; diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index 286155efcedf..cb68662cdc3a 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -1778,7 +1778,7 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data) } /* guard against interrupt when we're going down */ if (!test_bit(__E1000_DOWN, &adapter->state)) - mod_timer(&adapter->watchdog_timer, jiffies + 1); + queue_delayed_work(system_wq, &adapter->watchdog_work, 1); } /* Reset on uncorrectable ECC error */ @@ -1857,7 +1857,7 @@ static irqreturn_t e1000_intr(int __always_unused irq, void *data) } /* guard against interrupt when we're going down */ if (!test_bit(__E1000_DOWN, &adapter->state)) - mod_timer(&adapter->watchdog_timer, jiffies + 1); + queue_delayed_work(system_wq, &adapter->watchdog_work, 1); } /* Reset on uncorrectable ECC error */ @@ -1901,7 +1901,7 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data) hw->mac.get_link_status = true; /* guard against interrupt when we're going down */ if (!test_bit(__E1000_DOWN, &adapter->state)) - mod_timer(&adapter->watchdog_timer, jiffies + 1); + queue_delayed_work(system_wq, &adapter->watchdog_work, 1); } if (!test_bit(__E1000_DOWN, &adapter->state)) @@ -4287,7 +4287,8 @@ void e1000e_down(struct e1000_adapter *adapter, bool reset) napi_synchronize(&adapter->napi); - del_timer_sync(&adapter->watchdog_timer); + cancel_delayed_work_sync(&adapter->watchdog_work); + del_timer_sync(&adapter->phy_info_timer); spin_lock(&adapter->stats64_lock); @@ -5169,25 +5170,12 @@ static void e1000e_check_82574_phy_workaround(struct e1000_adapter *adapter) } } -/** - * e1000_watchdog - Timer Call-back - * @t: pointer to timer_list containing private info adapter - **/ -static void e1000_watchdog(struct timer_list *t) +static void e1000_watchdog_work(struct work_struct *work) { - struct e1000_adapter *adapter = from_timer(adapter, t, watchdog_timer); - - /* Do the rest outside of interrupt context */ - schedule_work(&adapter->watchdog_task); - - /* TODO: make this use queue_delayed_work() */ -} - -static void e1000_watchdog_task(struct work_struct *work) -{ - struct e1000_adapter *adapter = container_of(work, - struct e1000_adapter, - watchdog_task); + struct delayed_work *dwork = + container_of(work, struct delayed_work, work); + struct e1000_adapter *adapter = + container_of(dwork, struct e1000_adapter, watchdog_work); struct net_device *netdev = adapter->netdev; struct e1000_mac_info *mac = &adapter->hw.mac; struct e1000_phy_info *phy = &adapter->hw.phy; @@ -5416,8 +5404,8 @@ static void e1000_watchdog_task(struct work_struct *work) /* Reset the timer */ if (!test_bit(__E1000_DOWN, &adapter->state)) - mod_timer(&adapter->watchdog_timer, - round_jiffies(jiffies + 2 * HZ)); + queue_delayed_work(system_wq, &adapter->watchdog_work, + round_jiffies(2 * HZ)); } #define E1000_TX_FLAGS_CSUM 0x00000001 @@ -7596,11 +7584,10 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) goto err_eeprom; } - timer_setup(&adapter->watchdog_timer, e1000_watchdog, 0); timer_setup(&adapter->phy_info_timer, e1000_update_phy_info, 0); + INIT_DELAYED_WORK(&adapter->watchdog_work, e1000_watchdog_work); INIT_WORK(&adapter->reset_task, e1000_reset_task); - INIT_WORK(&adapter->watchdog_task, e1000_watchdog_task); INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround); INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task); INIT_WORK(&adapter->print_hang_task, e1000_print_hw_hang); @@ -7741,11 +7728,10 @@ static void e1000_remove(struct pci_dev *pdev) * from being rescheduled. */ set_bit(__E1000_DOWN, &adapter->state); - del_timer_sync(&adapter->watchdog_timer); + cancel_delayed_work_sync(&adapter->watchdog_work); del_timer_sync(&adapter->phy_info_timer); cancel_work_sync(&adapter->reset_task); - cancel_work_sync(&adapter->watchdog_task); cancel_work_sync(&adapter->downshift_task); cancel_work_sync(&adapter->update_phy_task); cancel_work_sync(&adapter->print_hang_task);
Replaces watchdog timer with delayed_work as advised in the driver's TODO comment. Signed-off-by: Dmitrii Ermakov <demonihin@gmail.com> --- V1 -> V2: Removed redundant line wraps, renamed e1000_watchdog to e1000_watchdog_work drivers/net/ethernet/intel/e1000e/e1000.h | 4 +-- drivers/net/ethernet/intel/e1000e/netdev.c | 42 ++++++++-------------- 2 files changed, 16 insertions(+), 30 deletions(-)