Message-ID: <BN8PR21MB1284785C320EFE09C75286B6CA889@BN8PR21MB1284.namprd21.prod.outlook.com>
Date:   Sat, 30 Oct 2021 15:54:55 +0000
From:   Haiyang Zhang <haiyangz@...rosoft.com>
To:     Dexuan Cui <decui@...rosoft.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "gustavoars@...nel.org" <gustavoars@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     KY Srinivasan <kys@...rosoft.com>,
        "stephen@...workplumber.org" <stephen@...workplumber.org>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        Shachar Raindel <shacharr@...rosoft.com>,
        Paul Rosswurm <paulros@...rosoft.com>,
        "olaf@...fle.de" <olaf@...fle.de>, vkuznets <vkuznets@...hat.com>
Subject: RE: [PATCH net-next 4/4] net: mana: Support hibernation and kexec



> -----Original Message-----
> From: Dexuan Cui <decui@...rosoft.com>
> Sent: Friday, October 29, 2021 8:54 PM
> To: davem@...emloft.net; kuba@...nel.org; gustavoars@...nel.org; Haiyang
> Zhang <haiyangz@...rosoft.com>; netdev@...r.kernel.org
> Cc: KY Srinivasan <kys@...rosoft.com>; stephen@...workplumber.org;
> wei.liu@...nel.org; linux-kernel@...r.kernel.org; linux-
> hyperv@...r.kernel.org; Shachar Raindel <shacharr@...rosoft.com>; Paul
> Rosswurm <paulros@...rosoft.com>; olaf@...fle.de; vkuznets
> <vkuznets@...hat.com>; Dexuan Cui <decui@...rosoft.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@...rosoft.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@...rosoft.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

Powered by blists - more mailing lists