| Message ID | 20211030005408.13932-5-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 635096a86edb067d55a1e04b4a918f5c6dac0c51 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mana: some misc patches | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: sthemmin@microsoft.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 322 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
> -----Original Message----- > From: Dexuan Cui <decui@microsoft.com> > Sent: Friday, October 29, 2021 8:54 PM > To: davem@davemloft.net; kuba@kernel.org; gustavoars@kernel.org; Haiyang > Zhang <haiyangz@microsoft.com>; netdev@vger.kernel.org > Cc: KY Srinivasan <kys@microsoft.com>; stephen@networkplumber.org; > wei.liu@kernel.org; linux-kernel@vger.kernel.org; linux- > hyperv@vger.kernel.org; Shachar Raindel <shacharr@microsoft.com>; Paul > Rosswurm <paulros@microsoft.com>; olaf@aepfle.de; vkuznets > <vkuznets@redhat.com>; Dexuan Cui <decui@microsoft.com> > Subject: [PATCH net-next 4/4] net: mana: Support hibernation and kexec > > Implement the suspend/resume/shutdown callbacks for hibernation/kexec. > > Add mana_gd_setup() and mana_gd_cleanup() for some common code, and > use them in the mand_gd_* callbacks. > > Reuse mana_probe/remove() for the hibernation path. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > .../net/ethernet/microsoft/mana/gdma_main.c | 140 +++++++++++++----- > drivers/net/ethernet/microsoft/mana/mana.h | 4 +- > drivers/net/ethernet/microsoft/mana/mana_en.c | 72 +++++++-- > 3 files changed, 164 insertions(+), 52 deletions(-) > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index 599dfd5e6090..c96ac81212f7 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > @@ -1258,6 +1258,52 @@ static void mana_gd_remove_irqs(struct pci_dev > *pdev) > gc->irq_contexts = NULL; > } > > +static int mana_gd_setup(struct pci_dev *pdev) > +{ > + struct gdma_context *gc = pci_get_drvdata(pdev); > + int err; > + > + mana_gd_init_registers(pdev); > + mana_smc_init(&gc->shm_channel, gc->dev, gc->shm_base); > + > + err = mana_gd_setup_irqs(pdev); > + if (err) > + return err; > + > + err = mana_hwc_create_channel(gc); > + if (err) > + goto remove_irq; > + > + err = mana_gd_verify_vf_version(pdev); > + if (err) > + goto destroy_hwc; > + > + err = mana_gd_query_max_resources(pdev); > + if (err) > + goto destroy_hwc; > + > + err = mana_gd_detect_devices(pdev); > + if (err) > + goto destroy_hwc; > + > + return 0; > + > +destroy_hwc: > + mana_hwc_destroy_channel(gc); > +remove_irq: > + mana_gd_remove_irqs(pdev); > + return err; > +} > + > +static void mana_gd_cleanup(struct pci_dev *pdev) > +{ > + struct gdma_context *gc = pci_get_drvdata(pdev); > + > + mana_hwc_destroy_channel(gc); > + > + mana_gd_remove_irqs(pdev); > +} > + > static int mana_gd_probe(struct pci_dev *pdev, const struct > pci_device_id *ent) > { > struct gdma_context *gc; > @@ -1287,6 +1333,9 @@ static int mana_gd_probe(struct pci_dev *pdev, > const struct pci_device_id *ent) > if (!gc) > goto release_region; > > + mutex_init(&gc->eq_test_event_mutex); > + pci_set_drvdata(pdev, gc); > + > bar0_va = pci_iomap(pdev, bar, 0); > if (!bar0_va) > goto free_gc; > @@ -1294,47 +1343,23 @@ static int mana_gd_probe(struct pci_dev *pdev, > const struct pci_device_id *ent) > gc->bar0_va = bar0_va; > gc->dev = &pdev->dev; > > - pci_set_drvdata(pdev, gc); > - > - mana_gd_init_registers(pdev); > > - mana_smc_init(&gc->shm_channel, gc->dev, gc->shm_base); > - > - err = mana_gd_setup_irqs(pdev); > + err = mana_gd_setup(pdev); > if (err) > goto unmap_bar; > > - mutex_init(&gc->eq_test_event_mutex); > - > - err = mana_hwc_create_channel(gc); > + err = mana_probe(&gc->mana, false); > if (err) > - goto remove_irq; > - > - err = mana_gd_verify_vf_version(pdev); > - if (err) > - goto remove_irq; > - > - err = mana_gd_query_max_resources(pdev); > - if (err) > - goto remove_irq; > - > - err = mana_gd_detect_devices(pdev); > - if (err) > - goto remove_irq; > - > - err = mana_probe(&gc->mana); > - if (err) > - goto clean_up_gdma; > + goto cleanup_gd; > > return 0; > > -clean_up_gdma: > - mana_hwc_destroy_channel(gc); > -remove_irq: > - mana_gd_remove_irqs(pdev); > +cleanup_gd: > + mana_gd_cleanup(pdev); > unmap_bar: > pci_iounmap(pdev, bar0_va); > free_gc: > + pci_set_drvdata(pdev, NULL); > vfree(gc); > release_region: > pci_release_regions(pdev); > @@ -1349,11 +1374,9 @@ static void mana_gd_remove(struct pci_dev *pdev) > { > struct gdma_context *gc = pci_get_drvdata(pdev); > > - mana_remove(&gc->mana); > + mana_remove(&gc->mana, false); > > - mana_hwc_destroy_channel(gc); > - > - mana_gd_remove_irqs(pdev); > + mana_gd_cleanup(pdev); > > pci_iounmap(pdev, gc->bar0_va); > > @@ -1364,6 +1387,52 @@ static void mana_gd_remove(struct pci_dev *pdev) > pci_disable_device(pdev); > } > > +/* The 'state' parameter is not used. */ > +static int mana_gd_suspend(struct pci_dev *pdev, pm_message_t state) > +{ > + struct gdma_context *gc = pci_get_drvdata(pdev); > + > + mana_remove(&gc->mana, true); > + > + mana_gd_cleanup(pdev); > + > + return 0; > +} > + > +/* In case the NIC hardware stops working, the suspend and resume > callbacks will > + * fail -- if this happens, it's safer to just report an error than try > to undo > + * what has been done. > + */ > +static int mana_gd_resume(struct pci_dev *pdev) > +{ > + struct gdma_context *gc = pci_get_drvdata(pdev); > + int err; > + > + err = mana_gd_setup(pdev); > + if (err) > + return err; > + > + err = mana_probe(&gc->mana, true); > + if (err) > + return err; > + > + return 0; > +} > + > +/* Quiesce the device for kexec. This is also called upon > reboot/shutdown. */ > +static void mana_gd_shutdown(struct pci_dev *pdev) > +{ > + struct gdma_context *gc = pci_get_drvdata(pdev); > + > + dev_info(&pdev->dev, "Shutdown was calledd\n"); > + > + mana_remove(&gc->mana, true); > + > + mana_gd_cleanup(pdev); > + > + pci_disable_device(pdev); > +} > + > #ifndef PCI_VENDOR_ID_MICROSOFT > #define PCI_VENDOR_ID_MICROSOFT 0x1414 > #endif > @@ -1378,6 +1447,9 @@ static struct pci_driver mana_driver = { > .id_table = mana_id_table, > .probe = mana_gd_probe, > .remove = mana_gd_remove, > + .suspend = mana_gd_suspend, > + .resume = mana_gd_resume, > + .shutdown = mana_gd_shutdown, > }; > > module_pci_driver(mana_driver); > diff --git a/drivers/net/ethernet/microsoft/mana/mana.h > b/drivers/net/ethernet/microsoft/mana/mana.h > index fc98a5ba5ed0..d047ee876f12 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana.h > +++ b/drivers/net/ethernet/microsoft/mana/mana.h > @@ -374,8 +374,8 @@ int mana_alloc_queues(struct net_device *ndev); > int mana_attach(struct net_device *ndev); > int mana_detach(struct net_device *ndev, bool from_close); > > -int mana_probe(struct gdma_dev *gd); > -void mana_remove(struct gdma_dev *gd); > +int mana_probe(struct gdma_dev *gd, bool resuming); > +void mana_remove(struct gdma_dev *gd, bool suspending); > > extern const struct ethtool_ops mana_ethtool_ops; > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index 4ff5a1fc506f..820585d45a61 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > @@ -1828,11 +1828,12 @@ static int mana_probe_port(struct mana_context > *ac, int port_idx, > return err; > } > > -int mana_probe(struct gdma_dev *gd) > +int mana_probe(struct gdma_dev *gd, bool resuming) > { > struct gdma_context *gc = gd->gdma_context; > + struct mana_context *ac = gd->driver_data; > struct device *dev = gc->dev; > - struct mana_context *ac; > + u16 num_ports = 0; > int err; > int i; > > @@ -1844,44 +1845,70 @@ int mana_probe(struct gdma_dev *gd) > if (err) > return err; > > - ac = kzalloc(sizeof(*ac), GFP_KERNEL); > - if (!ac) > - return -ENOMEM; > + if (!resuming) { > + ac = kzalloc(sizeof(*ac), GFP_KERNEL); > + if (!ac) > + return -ENOMEM; > > - ac->gdma_dev = gd; > - ac->num_ports = 1; > - gd->driver_data = ac; > + ac->gdma_dev = gd; > + gd->driver_data = ac; > + } > > err = mana_create_eq(ac); > if (err) > goto out; > > err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION, > MANA_MINOR_VERSION, > - MANA_MICRO_VERSION, &ac->num_ports); > + MANA_MICRO_VERSION, &num_ports); > if (err) > goto out; > > + if (!resuming) { > + ac->num_ports = num_ports; > + } else { > + if (ac->num_ports != num_ports) { > + dev_err(dev, "The number of vPorts changed: %d->%d\n", > + ac->num_ports, num_ports); > + err = -EPROTO; > + goto out; > + } > + } > + > + if (ac->num_ports == 0) > + dev_err(dev, "Failed to detect any vPort\n"); > + > if (ac->num_ports > MAX_PORTS_IN_MANA_DEV) > ac->num_ports = MAX_PORTS_IN_MANA_DEV; > > - for (i = 0; i < ac->num_ports; i++) { > - err = mana_probe_port(ac, i, &ac->ports[i]); > - if (err) > - break; > + if (!resuming) { > + for (i = 0; i < ac->num_ports; i++) { > + err = mana_probe_port(ac, i, &ac->ports[i]); > + if (err) > + break; > + } > + } else { > + for (i = 0; i < ac->num_ports; i++) { > + rtnl_lock(); > + err = mana_attach(ac->ports[i]); > + rtnl_unlock(); > + if (err) > + break; > + } > } > out: > if (err) > - mana_remove(gd); > + mana_remove(gd, false); The "goto out" can happen in both resuming true/false cases, should the error handling path deal with the two cases differently? Other parts look good. Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > > return err; > } > > -void mana_remove(struct gdma_dev *gd) > +void mana_remove(struct gdma_dev *gd, bool suspending) > { > struct gdma_context *gc = gd->gdma_context; > struct mana_context *ac = gd->driver_data; > struct device *dev = gc->dev; > struct net_device *ndev; > + int err; > int i; > > for (i = 0; i < ac->num_ports; i++) { > @@ -1897,7 +1924,16 @@ void mana_remove(struct gdma_dev *gd) > */ > rtnl_lock(); > > - mana_detach(ndev, false); > + err = mana_detach(ndev, false); > + if (err) > + netdev_err(ndev, "Failed to detach vPort %d: %d\n", > + i, err); > + > + if (suspending) { > + /* No need to unregister the ndev. */ > + rtnl_unlock(); > + continue; > + } > > unregister_netdevice(ndev); > > @@ -1910,6 +1946,10 @@ void mana_remove(struct gdma_dev *gd) > > out: > mana_gd_deregister_device(gd); > + > + if (suspending) > + return; > + > gd->driver_data = NULL; > gd->gdma_context = NULL; > kfree(ac); > -- > 2.17.1
> From: Haiyang Zhang <haiyangz@microsoft.com> > Sent: Saturday, October 30, 2021 8:55 AM > > > > @@ -1844,44 +1845,70 @@ int mana_probe(struct gdma_dev *gd) > > if (err) > > return err; > > > > - ac = kzalloc(sizeof(*ac), GFP_KERNEL); > > - if (!ac) > > - return -ENOMEM; > > + if (!resuming) { > > + ac = kzalloc(sizeof(*ac), GFP_KERNEL); > > + if (!ac) > > + return -ENOMEM; > > > > - ac->gdma_dev = gd; > > - ac->num_ports = 1; > > - gd->driver_data = ac; > > + ac->gdma_dev = gd; > > + gd->driver_data = ac; > > + } > > > > err = mana_create_eq(ac); > > if (err) > > goto out; > > > > err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION, > > MANA_MINOR_VERSION, > > - MANA_MICRO_VERSION, &ac->num_ports); > > + MANA_MICRO_VERSION, &num_ports); > > if (err) > > goto out; > > > > + if (!resuming) { > > + ac->num_ports = num_ports; > > + } else { > > + if (ac->num_ports != num_ports) { > > + dev_err(dev, "The number of vPorts changed: %d->%d\n", > > + ac->num_ports, num_ports); > > + err = -EPROTO; > > + goto out; > > + } > > + } > > + > > + if (ac->num_ports == 0) > > + dev_err(dev, "Failed to detect any vPort\n"); > > + > > if (ac->num_ports > MAX_PORTS_IN_MANA_DEV) > > ac->num_ports = MAX_PORTS_IN_MANA_DEV; > > > > - for (i = 0; i < ac->num_ports; i++) { > > - err = mana_probe_port(ac, i, &ac->ports[i]); > > - if (err) > > - break; > > + if (!resuming) { > > + for (i = 0; i < ac->num_ports; i++) { > > + err = mana_probe_port(ac, i, &ac->ports[i]); > > + if (err) > > + break; > > + } > > + } else { > > + for (i = 0; i < ac->num_ports; i++) { > > + rtnl_lock(); > > + err = mana_attach(ac->ports[i]); > > + rtnl_unlock(); > > + if (err) > > + break; > > + } > > } > > out: > > if (err) > > - mana_remove(gd); > > + mana_remove(gd, false); > > The "goto out" can happen in both resuming true/false cases, > should the error handling path deal with the two cases > differently? Let me make the below change in v2. Please let me know if any further change is needed: --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -1850,8 +1850,10 @@ int mana_probe(struct gdma_dev *gd, bool resuming) if (!resuming) { ac = kzalloc(sizeof(*ac), GFP_KERNEL); - if (!ac) - return -ENOMEM; + if (!ac) { + err = -ENOMEM; + goto out; + } ac->gdma_dev = gd; gd->driver_data = ac; @@ -1872,8 +1874,8 @@ int mana_probe(struct gdma_dev *gd, bool resuming) if (ac->num_ports != num_ports) { dev_err(dev, "The number of vPorts changed: %d->%d\n", ac->num_ports, num_ports); - err = -EPROTO; - goto out; + /* It's unsafe to proceed. */ + return -EPROTO; } } @@ -1886,22 +1888,36 @@ int mana_probe(struct gdma_dev *gd, bool resuming) if (!resuming) { for (i = 0; i < ac->num_ports; i++) { err = mana_probe_port(ac, i, &ac->ports[i]); - if (err) - break; + if (err) { + dev_err(dev, "Failed to probe vPort %u: %d\n", + i, err); + goto out; + } } } else { for (i = 0; i < ac->num_ports; i++) { rtnl_lock(); err = mana_attach(ac->ports[i]); rtnl_unlock(); - if (err) - break; + + if (err) { + netdev_err(ac->ports[i], + "Failed to resume vPort %u: %d\n", + i, err); + return err; + } } } + + return 0; out: - if (err) - mana_remove(gd, false); + /* In the resuming path, it's safer to leave the device in the failed + * state than try to invoke mana_detach(). + */ + if (resuming) + return err; + mana_remove(gd, false); return err; } @@ -1919,7 +1935,7 @@ void mana_remove(struct gdma_dev *gd, bool suspending) if (!ndev) { if (i == 0) dev_err(dev, "No net device to remove\n"); - goto out; + break; } /* All cleanup actions should stay after rtnl_lock(), otherwise For your easy reviewing, the new code of the function in v2 will be: int mana_probe(struct gdma_dev *gd, bool resuming) { struct gdma_context *gc = gd->gdma_context; struct mana_context *ac = gd->driver_data; struct device *dev = gc->dev; u16 num_ports = 0; int err; int i; dev_info(dev, "Microsoft Azure Network Adapter protocol version: %d.%d.%d\n", MANA_MAJOR_VERSION, MANA_MINOR_VERSION, MANA_MICRO_VERSION); err = mana_gd_register_device(gd); if (err) return err; if (!resuming) { ac = kzalloc(sizeof(*ac), GFP_KERNEL); if (!ac) { err = -ENOMEM; goto out; } ac->gdma_dev = gd; gd->driver_data = ac; } err = mana_create_eq(ac); if (err) goto out; err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION, MANA_MINOR_VERSION, MANA_MICRO_VERSION, &num_ports); if (err) goto out; if (!resuming) { ac->num_ports = num_ports; } else { if (ac->num_ports != num_ports) { dev_err(dev, "The number of vPorts changed: %d->%d\n", ac->num_ports, num_ports); /* It's unsafe to proceed. */ return -EPROTO; } } if (ac->num_ports == 0) dev_err(dev, "Failed to detect any vPort\n"); if (ac->num_ports > MAX_PORTS_IN_MANA_DEV) ac->num_ports = MAX_PORTS_IN_MANA_DEV; if (!resuming) { for (i = 0; i < ac->num_ports; i++) { err = mana_probe_port(ac, i, &ac->ports[i]); if (err) { dev_err(dev, "Failed to probe vPort %u: %d\n", i, err); goto out; } } } else { for (i = 0; i < ac->num_ports; i++) { rtnl_lock(); err = mana_attach(ac->ports[i]); rtnl_unlock(); if (err) { netdev_err(ac->ports[i], "Failed to resume vPort %u: %d\n", i, err); return err; } } } return 0; out: /* In the resuming path, it's safer to leave the device in the failed * state than try to invoke mana_detach(). */ if (resuming) return err; mana_remove(gd, false); return err; }
> -----Original Message----- > From: Dexuan Cui <decui@microsoft.com> > Sent: Monday, November 1, 2021 3:03 AM > To: Haiyang Zhang <haiyangz@microsoft.com>; davem@davemloft.net; > kuba@kernel.org; gustavoars@kernel.org; netdev@vger.kernel.org > Cc: KY Srinivasan <kys@microsoft.com>; stephen@networkplumber.org; > wei.liu@kernel.org; linux-kernel@vger.kernel.org; linux- > hyperv@vger.kernel.org; Shachar Raindel <shacharr@microsoft.com>; Paul > Rosswurm <paulros@microsoft.com>; olaf@aepfle.de; vkuznets > <vkuznets@redhat.com> > Subject: RE: [PATCH net-next 4/4] net: mana: Support hibernation and > kexec > > > From: Haiyang Zhang <haiyangz@microsoft.com> > > Sent: Saturday, October 30, 2021 8:55 AM > > > > > > @@ -1844,44 +1845,70 @@ int mana_probe(struct gdma_dev *gd) > > > if (err) > > > return err; > > > > > > - ac = kzalloc(sizeof(*ac), GFP_KERNEL); > > > - if (!ac) > > > - return -ENOMEM; > > > + if (!resuming) { > > > + ac = kzalloc(sizeof(*ac), GFP_KERNEL); > > > + if (!ac) > > > + return -ENOMEM; > > > > > > - ac->gdma_dev = gd; > > > - ac->num_ports = 1; > > > - gd->driver_data = ac; > > > + ac->gdma_dev = gd; > > > + gd->driver_data = ac; > > > + } > > > > > > err = mana_create_eq(ac); > > > if (err) > > > goto out; > > > > > > err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION, > > > MANA_MINOR_VERSION, > > > - MANA_MICRO_VERSION, &ac->num_ports); > > > + MANA_MICRO_VERSION, &num_ports); > > > if (err) > > > goto out; > > > > > > + if (!resuming) { > > > + ac->num_ports = num_ports; > > > + } else { > > > + if (ac->num_ports != num_ports) { > > > + dev_err(dev, "The number of vPorts changed: %d->%d\n", > > > + ac->num_ports, num_ports); > > > + err = -EPROTO; > > > + goto out; > > > + } > > > + } > > > + > > > + if (ac->num_ports == 0) > > > + dev_err(dev, "Failed to detect any vPort\n"); > > > + > > > if (ac->num_ports > MAX_PORTS_IN_MANA_DEV) > > > ac->num_ports = MAX_PORTS_IN_MANA_DEV; > > > > > > - for (i = 0; i < ac->num_ports; i++) { > > > - err = mana_probe_port(ac, i, &ac->ports[i]); > > > - if (err) > > > - break; > > > + if (!resuming) { > > > + for (i = 0; i < ac->num_ports; i++) { > > > + err = mana_probe_port(ac, i, &ac->ports[i]); > > > + if (err) > > > + break; > > > + } > > > + } else { > > > + for (i = 0; i < ac->num_ports; i++) { > > > + rtnl_lock(); > > > + err = mana_attach(ac->ports[i]); > > > + rtnl_unlock(); > > > + if (err) > > > + break; > > > + } > > > } > > > out: > > > if (err) > > > - mana_remove(gd); > > > + mana_remove(gd, false); > > > > The "goto out" can happen in both resuming true/false cases, > > should the error handling path deal with the two cases > > differently? > > Let me make the below change in v2. Please let me know > if any further change is needed: > > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > @@ -1850,8 +1850,10 @@ int mana_probe(struct gdma_dev *gd, bool resuming) > > if (!resuming) { > ac = kzalloc(sizeof(*ac), GFP_KERNEL); > - if (!ac) > - return -ENOMEM; > + if (!ac) { > + err = -ENOMEM; > + goto out; > + } > > ac->gdma_dev = gd; > gd->driver_data = ac; > @@ -1872,8 +1874,8 @@ int mana_probe(struct gdma_dev *gd, bool resuming) > if (ac->num_ports != num_ports) { > dev_err(dev, "The number of vPorts changed: %d- > >%d\n", > ac->num_ports, num_ports); > - err = -EPROTO; > - goto out; > + /* It's unsafe to proceed. */ > + return -EPROTO; > } > } > > @@ -1886,22 +1888,36 @@ int mana_probe(struct gdma_dev *gd, bool > resuming) > if (!resuming) { > for (i = 0; i < ac->num_ports; i++) { > err = mana_probe_port(ac, i, &ac->ports[i]); > - if (err) > - break; > + if (err) { > + dev_err(dev, "Failed to probe > vPort %u: %d\n", > + i, err); > + goto out; > + } > } > } else { > for (i = 0; i < ac->num_ports; i++) { > rtnl_lock(); > err = mana_attach(ac->ports[i]); > rtnl_unlock(); > - if (err) > - break; > + > + if (err) { > + netdev_err(ac->ports[i], > + "Failed to resume > vPort %u: %d\n", > + i, err); > + return err; > + } > } > } > + > + return 0; > out: > - if (err) > - mana_remove(gd, false); > + /* In the resuming path, it's safer to leave the device in the > failed > + * state than try to invoke mana_detach(). > + */ > + if (resuming) > + return err; > > + mana_remove(gd, false); > return err; > } > > @@ -1919,7 +1935,7 @@ void mana_remove(struct gdma_dev *gd, bool > suspending) > if (!ndev) { > if (i == 0) > dev_err(dev, "No net device to > remove\n"); > - goto out; > + break; > } > > /* All cleanup actions should stay after rtnl_lock(), > otherwise > > For your easy reviewing, the new code of the function in v2 will be: > > int mana_probe(struct gdma_dev *gd, bool resuming) > { > struct gdma_context *gc = gd->gdma_context; > struct mana_context *ac = gd->driver_data; > struct device *dev = gc->dev; > u16 num_ports = 0; > int err; > int i; > > dev_info(dev, > "Microsoft Azure Network Adapter protocol > version: %d.%d.%d\n", > MANA_MAJOR_VERSION, MANA_MINOR_VERSION, > MANA_MICRO_VERSION); > > err = mana_gd_register_device(gd); > if (err) > return err; > > if (!resuming) { > ac = kzalloc(sizeof(*ac), GFP_KERNEL); > if (!ac) { > err = -ENOMEM; > goto out; > } > > ac->gdma_dev = gd; > gd->driver_data = ac; > } > > err = mana_create_eq(ac); > if (err) > goto out; > > err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION, > MANA_MINOR_VERSION, > MANA_MICRO_VERSION, &num_ports); > if (err) > goto out; > > if (!resuming) { > ac->num_ports = num_ports; > } else { > if (ac->num_ports != num_ports) { > dev_err(dev, "The number of vPorts changed: %d- > >%d\n", > ac->num_ports, num_ports); > /* It's unsafe to proceed. */ > return -EPROTO; > } > } > > if (ac->num_ports == 0) > dev_err(dev, "Failed to detect any vPort\n"); > > if (ac->num_ports > MAX_PORTS_IN_MANA_DEV) > ac->num_ports = MAX_PORTS_IN_MANA_DEV; > > if (!resuming) { > for (i = 0; i < ac->num_ports; i++) { > err = mana_probe_port(ac, i, &ac->ports[i]); > if (err) { > dev_err(dev, "Failed to probe > vPort %u: %d\n", > i, err); > goto out; > } > } > } else { > for (i = 0; i < ac->num_ports; i++) { > rtnl_lock(); > err = mana_attach(ac->ports[i]); > rtnl_unlock(); > > if (err) { > netdev_err(ac->ports[i], > "Failed to resume > vPort %u: %d\n", > i, err); > return err; > } > } > } > > return 0; > out: > /* In the resuming path, it's safer to leave the device in the > failed > * state than try to invoke mana_detach(). > */ > if (resuming) > return err; > > mana_remove(gd, false); > return err; > } The new code looks good! Thanks, - Haiyang
> From: Haiyang Zhang <haiyangz@microsoft.com> > Sent: Monday, November 1, 2021 8:12 AM > ... > The new code looks good! > > Thanks, > - Haiyang The v1 patchset has been merged into net-next.git by patchwork-bot+netdevbpf@kernel.org ... I'll post an incremental patchset. Thanks, Dexuan
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c index 599dfd5e6090..c96ac81212f7 100644 --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c @@ -1258,6 +1258,52 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev) gc->irq_contexts = NULL; } +static int mana_gd_setup(struct pci_dev *pdev) +{ + struct gdma_context *gc = pci_get_drvdata(pdev); + int err; + + mana_gd_init_registers(pdev); + mana_smc_init(&gc->shm_channel, gc->dev, gc->shm_base); + + err = mana_gd_setup_irqs(pdev); + if (err) + return err; + + err = mana_hwc_create_channel(gc); + if (err) + goto remove_irq; + + err = mana_gd_verify_vf_version(pdev); + if (err) + goto destroy_hwc; + + err = mana_gd_query_max_resources(pdev); + if (err) + goto destroy_hwc; + + err = mana_gd_detect_devices(pdev); + if (err) + goto destroy_hwc; + + return 0; + +destroy_hwc: + mana_hwc_destroy_channel(gc); +remove_irq: + mana_gd_remove_irqs(pdev); + return err; +} + +static void mana_gd_cleanup(struct pci_dev *pdev) +{ + struct gdma_context *gc = pci_get_drvdata(pdev); + + mana_hwc_destroy_channel(gc); + + mana_gd_remove_irqs(pdev); +} + static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { struct gdma_context *gc; @@ -1287,6 +1333,9 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (!gc) goto release_region; + mutex_init(&gc->eq_test_event_mutex); + pci_set_drvdata(pdev, gc); + bar0_va = pci_iomap(pdev, bar, 0); if (!bar0_va) goto free_gc; @@ -1294,47 +1343,23 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent) gc->bar0_va = bar0_va; gc->dev = &pdev->dev; - pci_set_drvdata(pdev, gc); - - mana_gd_init_registers(pdev); - mana_smc_init(&gc->shm_channel, gc->dev, gc->shm_base); - - err = mana_gd_setup_irqs(pdev); + err = mana_gd_setup(pdev); if (err) goto unmap_bar; - mutex_init(&gc->eq_test_event_mutex); - - err = mana_hwc_create_channel(gc); + err = mana_probe(&gc->mana, false); if (err) - goto remove_irq; - - err = mana_gd_verify_vf_version(pdev); - if (err) - goto remove_irq; - - err = mana_gd_query_max_resources(pdev); - if (err) - goto remove_irq; - - err = mana_gd_detect_devices(pdev); - if (err) - goto remove_irq; - - err = mana_probe(&gc->mana); - if (err) - goto clean_up_gdma; + goto cleanup_gd; return 0; -clean_up_gdma: - mana_hwc_destroy_channel(gc); -remove_irq: - mana_gd_remove_irqs(pdev); +cleanup_gd: + mana_gd_cleanup(pdev); unmap_bar: pci_iounmap(pdev, bar0_va); free_gc: + pci_set_drvdata(pdev, NULL); vfree(gc); release_region: pci_release_regions(pdev); @@ -1349,11 +1374,9 @@ static void mana_gd_remove(struct pci_dev *pdev) { struct gdma_context *gc = pci_get_drvdata(pdev); - mana_remove(&gc->mana); + mana_remove(&gc->mana, false); - mana_hwc_destroy_channel(gc); - - mana_gd_remove_irqs(pdev); + mana_gd_cleanup(pdev); pci_iounmap(pdev, gc->bar0_va); @@ -1364,6 +1387,52 @@ static void mana_gd_remove(struct pci_dev *pdev) pci_disable_device(pdev); } +/* The 'state' parameter is not used. */ +static int mana_gd_suspend(struct pci_dev *pdev, pm_message_t state) +{ + struct gdma_context *gc = pci_get_drvdata(pdev); + + mana_remove(&gc->mana, true); + + mana_gd_cleanup(pdev); + + return 0; +} + +/* In case the NIC hardware stops working, the suspend and resume callbacks will + * fail -- if this happens, it's safer to just report an error than try to undo + * what has been done. + */ +static int mana_gd_resume(struct pci_dev *pdev) +{ + struct gdma_context *gc = pci_get_drvdata(pdev); + int err; + + err = mana_gd_setup(pdev); + if (err) + return err; + + err = mana_probe(&gc->mana, true); + if (err) + return err; + + return 0; +} + +/* Quiesce the device for kexec. This is also called upon reboot/shutdown. */ +static void mana_gd_shutdown(struct pci_dev *pdev) +{ + struct gdma_context *gc = pci_get_drvdata(pdev); + + dev_info(&pdev->dev, "Shutdown was calledd\n"); + + mana_remove(&gc->mana, true); + + mana_gd_cleanup(pdev); + + pci_disable_device(pdev); +} + #ifndef PCI_VENDOR_ID_MICROSOFT #define PCI_VENDOR_ID_MICROSOFT 0x1414 #endif @@ -1378,6 +1447,9 @@ static struct pci_driver mana_driver = { .id_table = mana_id_table, .probe = mana_gd_probe, .remove = mana_gd_remove, + .suspend = mana_gd_suspend, + .resume = mana_gd_resume, + .shutdown = mana_gd_shutdown, }; module_pci_driver(mana_driver); diff --git a/drivers/net/ethernet/microsoft/mana/mana.h b/drivers/net/ethernet/microsoft/mana/mana.h index fc98a5ba5ed0..d047ee876f12 100644 --- a/drivers/net/ethernet/microsoft/mana/mana.h +++ b/drivers/net/ethernet/microsoft/mana/mana.h @@ -374,8 +374,8 @@ int mana_alloc_queues(struct net_device *ndev); int mana_attach(struct net_device *ndev); int mana_detach(struct net_device *ndev, bool from_close); -int mana_probe(struct gdma_dev *gd); -void mana_remove(struct gdma_dev *gd); +int mana_probe(struct gdma_dev *gd, bool resuming); +void mana_remove(struct gdma_dev *gd, bool suspending); extern const struct ethtool_ops mana_ethtool_ops; diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c index 4ff5a1fc506f..820585d45a61 100644 --- a/drivers/net/ethernet/microsoft/mana/mana_en.c +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c @@ -1828,11 +1828,12 @@ static int mana_probe_port(struct mana_context *ac, int port_idx, return err; } -int mana_probe(struct gdma_dev *gd) +int mana_probe(struct gdma_dev *gd, bool resuming) { struct gdma_context *gc = gd->gdma_context; + struct mana_context *ac = gd->driver_data; struct device *dev = gc->dev; - struct mana_context *ac; + u16 num_ports = 0; int err; int i; @@ -1844,44 +1845,70 @@ int mana_probe(struct gdma_dev *gd) if (err) return err; - ac = kzalloc(sizeof(*ac), GFP_KERNEL); - if (!ac) - return -ENOMEM; + if (!resuming) { + ac = kzalloc(sizeof(*ac), GFP_KERNEL); + if (!ac) + return -ENOMEM; - ac->gdma_dev = gd; - ac->num_ports = 1; - gd->driver_data = ac; + ac->gdma_dev = gd; + gd->driver_data = ac; + } err = mana_create_eq(ac); if (err) goto out; err = mana_query_device_cfg(ac, MANA_MAJOR_VERSION, MANA_MINOR_VERSION, - MANA_MICRO_VERSION, &ac->num_ports); + MANA_MICRO_VERSION, &num_ports); if (err) goto out; + if (!resuming) { + ac->num_ports = num_ports; + } else { + if (ac->num_ports != num_ports) { + dev_err(dev, "The number of vPorts changed: %d->%d\n", + ac->num_ports, num_ports); + err = -EPROTO; + goto out; + } + } + + if (ac->num_ports == 0) + dev_err(dev, "Failed to detect any vPort\n"); + if (ac->num_ports > MAX_PORTS_IN_MANA_DEV) ac->num_ports = MAX_PORTS_IN_MANA_DEV; - for (i = 0; i < ac->num_ports; i++) { - err = mana_probe_port(ac, i, &ac->ports[i]); - if (err) - break; + if (!resuming) { + for (i = 0; i < ac->num_ports; i++) { + err = mana_probe_port(ac, i, &ac->ports[i]); + if (err) + break; + } + } else { + for (i = 0; i < ac->num_ports; i++) { + rtnl_lock(); + err = mana_attach(ac->ports[i]); + rtnl_unlock(); + if (err) + break; + } } out: if (err) - mana_remove(gd); + mana_remove(gd, false); return err; } -void mana_remove(struct gdma_dev *gd) +void mana_remove(struct gdma_dev *gd, bool suspending) { struct gdma_context *gc = gd->gdma_context; struct mana_context *ac = gd->driver_data; struct device *dev = gc->dev; struct net_device *ndev; + int err; int i; for (i = 0; i < ac->num_ports; i++) { @@ -1897,7 +1924,16 @@ void mana_remove(struct gdma_dev *gd) */ rtnl_lock(); - mana_detach(ndev, false); + err = mana_detach(ndev, false); + if (err) + netdev_err(ndev, "Failed to detach vPort %d: %d\n", + i, err); + + if (suspending) { + /* No need to unregister the ndev. */ + rtnl_unlock(); + continue; + } unregister_netdevice(ndev); @@ -1910,6 +1946,10 @@ void mana_remove(struct gdma_dev *gd) out: mana_gd_deregister_device(gd); + + if (suspending) + return; + gd->driver_data = NULL; gd->gdma_context = NULL; kfree(ac);
Implement the suspend/resume/shutdown callbacks for hibernation/kexec. Add mana_gd_setup() and mana_gd_cleanup() for some common code, and use them in the mand_gd_* callbacks. Reuse mana_probe/remove() for the hibernation path. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- .../net/ethernet/microsoft/mana/gdma_main.c | 140 +++++++++++++----- drivers/net/ethernet/microsoft/mana/mana.h | 4 +- drivers/net/ethernet/microsoft/mana/mana_en.c | 72 +++++++-- 3 files changed, 164 insertions(+), 52 deletions(-)