| Message ID | 20231019071346.55949-1-mschmidt@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] iavf: initialize waitqueues before starting watchdog_task | expand |
Dear Michal, Am 19.10.23 um 09:13 schrieb Michal Schmidt: > It is not safe to initialize the waitqueues after queueing the > watchdog_task. It will be using them. > > The chance of this causing a real problem is very small, because > there will be some sleeping before any of the waitqueues get used. > I got a crash only after inserting an artificial sleep in iavf_probe. > > Queue the watchdog_task as the last step in iavf_probe. Add a comment to > prevent repeating the mistake. > > Fixes: fe2647ab0c99 ("i40evf: prevent VF close returning before state transitions to DOWN") > Signed-off-by: Michal Schmidt <mschmidt@redhat.com> > --- > drivers/net/ethernet/intel/iavf/iavf_main.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 6a2e6d64bc3a..5b5c0525aa13 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c > @@ -4982,8 +4982,6 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > INIT_WORK(&adapter->finish_config, iavf_finish_config); > INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task); > INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task); > - queue_delayed_work(adapter->wq, &adapter->watchdog_task, > - msecs_to_jiffies(5 * (pdev->devfn & 0x07))); > > /* Setup the wait queue for indicating transition to down status */ > init_waitqueue_head(&adapter->down_waitqueue); > @@ -4994,6 +4992,9 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > /* Setup the wait queue for indicating virtchannel events */ > init_waitqueue_head(&adapter->vc_waitqueue); > > + queue_delayed_work(adapter->wq, &adapter->watchdog_task, > + msecs_to_jiffies(5 * (pdev->devfn & 0x07))); > + /* Initialization goes on in the work. Do not add more of it below. */ > return 0; > > err_ioremap: Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul
On 10/19/23 09:13, Michal Schmidt wrote: > It is not safe to initialize the waitqueues after queueing the > watchdog_task. It will be using them. > > The chance of this causing a real problem is very small, because > there will be some sleeping before any of the waitqueues get used. > I got a crash only after inserting an artificial sleep in iavf_probe. > > Queue the watchdog_task as the last step in iavf_probe. Add a comment to > prevent repeating the mistake. > > Fixes: fe2647ab0c99 ("i40evf: prevent VF close returning before state transitions to DOWN") > Signed-off-by: Michal Schmidt <mschmidt@redhat.com> > --- > drivers/net/ethernet/intel/iavf/iavf_main.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 6a2e6d64bc3a..5b5c0525aa13 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c > @@ -4982,8 +4982,6 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > INIT_WORK(&adapter->finish_config, iavf_finish_config); > INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task); > INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task); > - queue_delayed_work(adapter->wq, &adapter->watchdog_task, > - msecs_to_jiffies(5 * (pdev->devfn & 0x07))); > > /* Setup the wait queue for indicating transition to down status */ > init_waitqueue_head(&adapter->down_waitqueue); > @@ -4994,6 +4992,9 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > /* Setup the wait queue for indicating virtchannel events */ > init_waitqueue_head(&adapter->vc_waitqueue); > > + queue_delayed_work(adapter->wq, &adapter->watchdog_task, > + msecs_to_jiffies(5 * (pdev->devfn & 0x07))); > + /* Initialization goes on in the work. Do not add more of it below. */ > return 0; > > err_ioremap: Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
On Thu, 19 Oct 2023 09:13:46 +0200 Michal Schmidt wrote: > It is not safe to initialize the waitqueues after queueing the > watchdog_task. It will be using them. > > The chance of this causing a real problem is very small, because > there will be some sleeping before any of the waitqueues get used. > I got a crash only after inserting an artificial sleep in iavf_probe. > > Queue the watchdog_task as the last step in iavf_probe. Add a comment to > prevent repeating the mistake. Applied as 7db31110438 in net, thanks!
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c index 6a2e6d64bc3a..5b5c0525aa13 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_main.c +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c @@ -4982,8 +4982,6 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) INIT_WORK(&adapter->finish_config, iavf_finish_config); INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task); INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task); - queue_delayed_work(adapter->wq, &adapter->watchdog_task, - msecs_to_jiffies(5 * (pdev->devfn & 0x07))); /* Setup the wait queue for indicating transition to down status */ init_waitqueue_head(&adapter->down_waitqueue); @@ -4994,6 +4992,9 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) /* Setup the wait queue for indicating virtchannel events */ init_waitqueue_head(&adapter->vc_waitqueue); + queue_delayed_work(adapter->wq, &adapter->watchdog_task, + msecs_to_jiffies(5 * (pdev->devfn & 0x07))); + /* Initialization goes on in the work. Do not add more of it below. */ return 0; err_ioremap:
It is not safe to initialize the waitqueues after queueing the watchdog_task. It will be using them. The chance of this causing a real problem is very small, because there will be some sleeping before any of the waitqueues get used. I got a crash only after inserting an artificial sleep in iavf_probe. Queue the watchdog_task as the last step in iavf_probe. Add a comment to prevent repeating the mistake. Fixes: fe2647ab0c99 ("i40evf: prevent VF close returning before state transitions to DOWN") Signed-off-by: Michal Schmidt <mschmidt@redhat.com> --- drivers/net/ethernet/intel/iavf/iavf_main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)