[PATCH net-next 0/4] net: mana: some misc patches

[PATCH net-next 0/4] net: mana: some misc patches

[Dexuan Cui]

Hi all,

Patch 1 is a small fix.

Patch 2 reports OS info to the PF driver.
Before the patch, the req fields were all zeros.

Patch 3 fixes and cleans up the error handling of HWC creation failure.

Patch 4 adds the callbacks for hibernation/kexec. It's based on patch 3.

Please review. Thanks!

Thanks,
Dexuan

Dexuan Cui (4):
  net: mana: Fix the netdev_err()'s vPort argument in mana_init_port()
  net: mana: Report OS info to the PF driver
  net: mana: Improve the HWC error handling
  net: mana: Support hibernation and kexec

 .../net/ethernet/microsoft/mana/gdma_main.c   | 155 +++++++++++++-----
 .../net/ethernet/microsoft/mana/hw_channel.c  |  71 ++++----
 drivers/net/ethernet/microsoft/mana/mana.h    |   4 +-
 drivers/net/ethernet/microsoft/mana/mana_en.c |  75 +++++++--
 4 files changed, 208 insertions(+), 97 deletions(-)

-- 
2.17.1

[PATCH net-next 1/4] net: mana: Fix the netdev_err()'s vPort argument in mana_init_port()

[Dexuan Cui]

Use the correct port index rather than 0.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 1417d1e72b7b..4ff5a1fc506f 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1599,7 +1599,8 @@ static int mana_init_port(struct net_device *ndev)
 	err = mana_query_vport_cfg(apc, port_idx, &max_txq, &max_rxq,
 				   &num_indirect_entries);
 	if (err) {
-		netdev_err(ndev, "Failed to query info for vPort 0\n");
+		netdev_err(ndev, "Failed to query info for vPort %d\n",
+			   port_idx);
 		goto reset_apc;
 	}
 
-- 
2.17.1

RE: [PATCH net-next 1/4] net: mana: Fix the netdev_err()'s vPort argument in mana_init_port()

[Haiyang Zhang]

> -----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 1/4] net: mana: Fix the netdev_err()'s vPort
> argument in mana_init_port()
> 
> Use the correct port index rather than 0.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 1417d1e72b7b..4ff5a1fc506f 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1599,7 +1599,8 @@ static int mana_init_port(struct net_device *ndev)
>  	err = mana_query_vport_cfg(apc, port_idx, &max_txq, &max_rxq,
>  				   &num_indirect_entries);
>  	if (err) {
> -		netdev_err(ndev, "Failed to query info for vPort 0\n");
> +		netdev_err(ndev, "Failed to query info for vPort %d\n",
> +			   port_idx);

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

Re: [PATCH net-next 1/4] net: mana: Fix the netdev_err()'s vPort argument in mana_init_port()

[Stephen Hemminger]

On Fri, 29 Oct 2021 17:54:05 -0700
Dexuan Cui <decui@microsoft.com> wrote:

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 1417d1e72b7b..4ff5a1fc506f 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1599,7 +1599,8 @@ static int mana_init_port(struct net_device *ndev)
>  	err = mana_query_vport_cfg(apc, port_idx, &max_txq, &max_rxq,
>  				   &num_indirect_entries);
>  	if (err) {
> -		netdev_err(ndev, "Failed to query info for vPort 0\n");
> +		netdev_err(ndev, "Failed to query info for vPort %d\n",
> +			   port_idx);

Shouldn't port_idx have been unsigned or u16?
It is u16 in mana_port_context.

RE: [PATCH net-next 1/4] net: mana: Fix the netdev_err()'s vPort argument in mana_init_port()

[Dexuan Cui]

> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Saturday, October 30, 2021 1:07 PM
> 
> On Fri, 29 Oct 2021 17:54:05 -0700
> Dexuan Cui <decui@microsoft.com> wrote:
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 1417d1e72b7b..4ff5a1fc506f 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -1599,7 +1599,8 @@ static int mana_init_port(struct net_device *ndev)
> >  	err = mana_query_vport_cfg(apc, port_idx, &max_txq, &max_rxq,
> >  				   &num_indirect_entries);
> >  	if (err) {
> > -		netdev_err(ndev, "Failed to query info for vPort 0\n");
> > +		netdev_err(ndev, "Failed to query info for vPort %d\n",
> > +			   port_idx);
> 
> Shouldn't port_idx have been unsigned or u16?
> It is u16 in mana_port_context.

Thanks! I'll use "u32" and "%u" here.

I'll post v2 like the below. Please let me know if any further change is needed.

diff --git a/drivers/net/ethernet/microsoft/mana/mana.h b/drivers/net/ethernet/microsoft/mana/mana.h
index fc98a5ba5ed0..0a4246646447 100644
--- a/drivers/net/ethernet/microsoft/mana/mana.h
+++ b/drivers/net/ethernet/microsoft/mana/mana.h
@@ -359,6 +359,9 @@ struct mana_port_context {

        mana_handle_t port_handle;

+       /* This doesn't have to be u32, because the max_num_vports is u16:
+        * see mana_query_device_cfg().
+        */
        u16 port_idx;

        bool port_is_up;
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 1417d1e72b7b..b495e9a20324 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -479,6 +479,9 @@ static int mana_query_device_cfg(struct mana_context *ac, u32 proto_major_ver,
        return 0;
 }

+/* The range of the 'vport_index' parameter is actually only u16, but since
+ * the PF driver defines req.vport_index as u32, we also use u32 here.
+ */
 static int mana_query_vport_cfg(struct mana_port_context *apc, u32 vport_index,
                                u32 *max_sq, u32 *max_rq, u32 *num_indir_entry)
 {
@@ -1588,7 +1591,7 @@ static int mana_init_port(struct net_device *ndev)
 {
        struct mana_port_context *apc = netdev_priv(ndev);
        u32 max_txq, max_rxq, max_queues;
-       int port_idx = apc->port_idx;
+       u32 port_idx = apc->port_idx;
        u32 num_indirect_entries;
        int err;

@@ -1599,7 +1602,8 @@ static int mana_init_port(struct net_device *ndev)
        err = mana_query_vport_cfg(apc, port_idx, &max_txq, &max_rxq,
                                   &num_indirect_entries);
        if (err) {
-               netdev_err(ndev, "Failed to query info for vPort 0\n");
+               netdev_err(ndev, "Failed to query info for vPort %u\n",
+                          port_idx);
                goto reset_apc;
        }

[PATCH net-next 2/4] net: mana: Report OS info to the PF driver

[Dexuan Cui]

The PF driver might use the OS info for statistical purposes.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/gdma_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index cee75b561f59..8a9ee2885f8c 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -3,6 +3,8 @@
 
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/utsname.h>
+#include <linux/version.h>
 
 #include "mana.h"
 
@@ -848,6 +850,15 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
 	req.gd_drv_cap_flags3 = GDMA_DRV_CAP_FLAGS3;
 	req.gd_drv_cap_flags4 = GDMA_DRV_CAP_FLAGS4;
 
+	req.drv_ver = 0;	/* Unused*/
+	req.os_type = 0x10;	/* Linux */
+	req.os_ver_major = LINUX_VERSION_MAJOR;
+	req.os_ver_minor = LINUX_VERSION_PATCHLEVEL;
+	req.os_ver_build = LINUX_VERSION_SUBLEVEL;
+	strscpy(req.os_ver_str1, utsname()->sysname, sizeof(req.os_ver_str1));
+	strscpy(req.os_ver_str2, utsname()->release, sizeof(req.os_ver_str2));
+	strscpy(req.os_ver_str3, utsname()->version, sizeof(req.os_ver_str3));
+
 	err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
 	if (err || resp.hdr.status) {
 		dev_err(gc->dev, "VfVerifyVersionOutput: %d, status=0x%x\n",
-- 
2.17.1

RE: [PATCH net-next 2/4] net: mana: Report OS info to the PF driver

[Haiyang Zhang]

> -----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 2/4] net: mana: Report OS info to the PF driver
> 
> The PF driver might use the OS info for statistical purposes.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/net/ethernet/microsoft/mana/gdma_main.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index cee75b561f59..8a9ee2885f8c 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -3,6 +3,8 @@
> 
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/utsname.h>
> +#include <linux/version.h>
> 
>  #include "mana.h"
> 
> @@ -848,6 +850,15 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
>  	req.gd_drv_cap_flags3 = GDMA_DRV_CAP_FLAGS3;
>  	req.gd_drv_cap_flags4 = GDMA_DRV_CAP_FLAGS4;
> 
> +	req.drv_ver = 0;	/* Unused*/
> +	req.os_type = 0x10;	/* Linux */

Instead of a magic number, could you define it as a macro?

Other parts look fine.

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

> +	req.os_ver_major = LINUX_VERSION_MAJOR;
> +	req.os_ver_minor = LINUX_VERSION_PATCHLEVEL;
> +	req.os_ver_build = LINUX_VERSION_SUBLEVEL;
> +	strscpy(req.os_ver_str1, utsname()->sysname,
> sizeof(req.os_ver_str1));
> +	strscpy(req.os_ver_str2, utsname()->release,
> sizeof(req.os_ver_str2));
> +	strscpy(req.os_ver_str3, utsname()->version,
> sizeof(req.os_ver_str3));
> +
>  	err = mana_gd_send_request(gc, sizeof(req), &req, sizeof(resp),
> &resp);
>  	if (err || resp.hdr.status) {
>  		dev_err(gc->dev, "VfVerifyVersionOutput: %d, status=0x%x\n",
> --
> 2.17.1

RE: [PATCH net-next 2/4] net: mana: Report OS info to the PF driver

[Dexuan Cui]

> From: Haiyang Zhang <haiyangz@microsoft.com>
> Sent: Saturday, October 30, 2021 8:36 AM
> > @@ -848,6 +850,15 @@ int mana_gd_verify_vf_version(struct pci_dev
> *pdev)
> >  	req.gd_drv_cap_flags3 = GDMA_DRV_CAP_FLAGS3;
> >  	req.gd_drv_cap_flags4 = GDMA_DRV_CAP_FLAGS4;
> >
> > +	req.drv_ver = 0;	/* Unused*/
> > +	req.os_type = 0x10;	/* Linux */
> 
> Instead of a magic number, could you define it as a macro?
> 
> Other parts look fine.
> 
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

Ok, I'll apply the below change in v2.

@@ -497,6 +497,7 @@ enum {

 #define GDMA_DRV_CAP_FLAGS4 0

+#define GDMA_OS_TYPE_LINUX      0x10
 struct gdma_verify_ver_req {
        struct gdma_req_hdr hdr;

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index c96ac81212f7..bea218c5c043 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -851,7 +851,7 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
        req.gd_drv_cap_flags4 = GDMA_DRV_CAP_FLAGS4;

        req.drv_ver = 0;        /* Unused*/
-       req.os_type = 0x10;     /* Linux */
+       req.os_type = GDMA_OS_TYPE_LINUX;
        req.os_ver_major = LINUX_VERSION_MAJOR;
        req.os_ver_minor = LINUX_VERSION_PATCHLEVEL;
        req.os_ver_build = LINUX_VERSION_SUBLEVEL;

[PATCH net-next 3/4] net: mana: Improve the HWC error handling

[Dexuan Cui]

Currently when the HWC creation fails, the error handling is flawed,
e.g. if mana_hwc_create_channel() -> mana_hwc_establish_channel() fails,
the resources acquired in mana_hwc_init_queues() is not released.

Enhance mana_hwc_destroy_channel() to do the proper cleanup work and
call it accordingly.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 .../net/ethernet/microsoft/mana/gdma_main.c   |  4 --
 .../net/ethernet/microsoft/mana/hw_channel.c  | 71 ++++++++-----------
 2 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 8a9ee2885f8c..599dfd5e6090 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -1330,8 +1330,6 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 clean_up_gdma:
 	mana_hwc_destroy_channel(gc);
-	vfree(gc->cq_table);
-	gc->cq_table = NULL;
 remove_irq:
 	mana_gd_remove_irqs(pdev);
 unmap_bar:
@@ -1354,8 +1352,6 @@ static void mana_gd_remove(struct pci_dev *pdev)
 	mana_remove(&gc->mana);
 
 	mana_hwc_destroy_channel(gc);
-	vfree(gc->cq_table);
-	gc->cq_table = NULL;
 
 	mana_gd_remove_irqs(pdev);
 
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index c1310ea1c216..851de2b81fa4 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -309,9 +309,6 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
 
 static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
 {
-	if (!hwc_cq)
-		return;
-
 	kfree(hwc_cq->comp_buf);
 
 	if (hwc_cq->gdma_cq)
@@ -448,9 +445,6 @@ static void mana_hwc_dealloc_dma_buf(struct hw_channel_context *hwc,
 static void mana_hwc_destroy_wq(struct hw_channel_context *hwc,
 				struct hwc_wq *hwc_wq)
 {
-	if (!hwc_wq)
-		return;
-
 	mana_hwc_dealloc_dma_buf(hwc, hwc_wq->msg_buf);
 
 	if (hwc_wq->gdma_wq)
@@ -623,6 +617,7 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
 	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
 
+	/* Both were set in mana_hwc_init_event_handler(). */
 	if (WARN_ON(cq->id >= gc->max_num_cqs))
 		return -EPROTO;
 
@@ -638,9 +633,6 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 				u32 max_req_msg_size, u32 max_resp_msg_size)
 {
-	struct hwc_wq *hwc_rxq = NULL;
-	struct hwc_wq *hwc_txq = NULL;
-	struct hwc_cq *hwc_cq = NULL;
 	int err;
 
 	err = mana_hwc_init_inflight_msg(hwc, q_depth);
@@ -653,44 +645,32 @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16 q_depth,
 	err = mana_hwc_create_cq(hwc, q_depth * 2,
 				 mana_hwc_init_event_handler, hwc,
 				 mana_hwc_rx_event_handler, hwc,
-				 mana_hwc_tx_event_handler, hwc, &hwc_cq);
+				 mana_hwc_tx_event_handler, hwc, &hwc->cq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC CQ: %d\n", err);
 		goto out;
 	}
-	hwc->cq = hwc_cq;
 
 	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
-				 hwc_cq, &hwc_rxq);
+				 hwc->cq, &hwc->rxq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
 		goto out;
 	}
-	hwc->rxq = hwc_rxq;
 
 	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
-				 hwc_cq, &hwc_txq);
+				 hwc->cq, &hwc->txq);
 	if (err) {
 		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
 		goto out;
 	}
-	hwc->txq = hwc_txq;
 
 	hwc->num_inflight_msg = q_depth;
 	hwc->max_req_msg_size = max_req_msg_size;
 
 	return 0;
 out:
-	if (hwc_txq)
-		mana_hwc_destroy_wq(hwc, hwc_txq);
-
-	if (hwc_rxq)
-		mana_hwc_destroy_wq(hwc, hwc_rxq);
-
-	if (hwc_cq)
-		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc_cq);
-
-	mana_gd_free_res_map(&hwc->inflight_msg_res);
+	/* mana_hwc_create_channel() will do the cleanup.*/
 	return err;
 }
 
@@ -718,6 +698,9 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 	gd->pdid = INVALID_PDID;
 	gd->doorbell = INVALID_DOORBELL;
 
+	/* mana_hwc_init_queues() only creates the required data structures,
+	 * and doesn't touch the HWC device.
+	 */
 	err = mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
 				   HW_CHANNEL_MAX_REQUEST_SIZE,
 				   HW_CHANNEL_MAX_RESPONSE_SIZE);
@@ -743,42 +726,50 @@ int mana_hwc_create_channel(struct gdma_context *gc)
 
 	return 0;
 out:
-	kfree(hwc);
+	mana_hwc_destroy_channel(gc);
 	return err;
 }
 
 void mana_hwc_destroy_channel(struct gdma_context *gc)
 {
 	struct hw_channel_context *hwc = gc->hwc.driver_data;
-	struct hwc_caller_ctx *ctx;
 
-	mana_smc_teardown_hwc(&gc->shm_channel, false);
+	if (!hwc)
+		return;
+
+	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
+	 * non-zero, the HWC worked and we should tear down the HWC here.
+	 */
+	if (gc->max_num_cqs > 0) {
+		mana_smc_teardown_hwc(&gc->shm_channel, false);
+		gc->max_num_cqs = 0;
+	}
 
-	ctx = hwc->caller_ctx;
-	kfree(ctx);
+	kfree(hwc->caller_ctx);
 	hwc->caller_ctx = NULL;
 
-	mana_hwc_destroy_wq(hwc, hwc->txq);
-	hwc->txq = NULL;
+	if (hwc->txq)
+		mana_hwc_destroy_wq(hwc, hwc->txq);
 
-	mana_hwc_destroy_wq(hwc, hwc->rxq);
-	hwc->rxq = NULL;
+	if (hwc->rxq)
+		mana_hwc_destroy_wq(hwc, hwc->rxq);
 
-	mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
-	hwc->cq = NULL;
+	if (hwc->cq)
+		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
 
 	mana_gd_free_res_map(&hwc->inflight_msg_res);
 
 	hwc->num_inflight_msg = 0;
 
-	if (hwc->gdma_dev->pdid != INVALID_PDID) {
-		hwc->gdma_dev->doorbell = INVALID_DOORBELL;
-		hwc->gdma_dev->pdid = INVALID_PDID;
-	}
+	hwc->gdma_dev->doorbell = INVALID_DOORBELL;
+	hwc->gdma_dev->pdid = INVALID_PDID;
 
 	kfree(hwc);
 	gc->hwc.driver_data = NULL;
 	gc->hwc.gdma_context = NULL;
+
+	vfree(gc->cq_table);
+	gc->cq_table = NULL;
 }
 
 int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
-- 
2.17.1

RE: [PATCH net-next 3/4] net: mana: Improve the HWC error handling

[Haiyang Zhang]

> -----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 3/4] net: mana: Improve the HWC error handling
> 
> Currently when the HWC creation fails, the error handling is flawed, e.g.
> if mana_hwc_create_channel() -> mana_hwc_establish_channel() fails, the
> resources acquired in mana_hwc_init_queues() is not released.
> 
> Enhance mana_hwc_destroy_channel() to do the proper cleanup work and
> call it accordingly.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  .../net/ethernet/microsoft/mana/gdma_main.c   |  4 --
>  .../net/ethernet/microsoft/mana/hw_channel.c  | 71 ++++++++-----------
>  2 files changed, 31 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8a9ee2885f8c..599dfd5e6090 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1330,8 +1330,6 @@ static int mana_gd_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> 
>  clean_up_gdma:
>  	mana_hwc_destroy_channel(gc);
> -	vfree(gc->cq_table);
> -	gc->cq_table = NULL;
>  remove_irq:
>  	mana_gd_remove_irqs(pdev);
>  unmap_bar:
> @@ -1354,8 +1352,6 @@ static void mana_gd_remove(struct pci_dev *pdev)
>  	mana_remove(&gc->mana);
> 
>  	mana_hwc_destroy_channel(gc);
> -	vfree(gc->cq_table);
> -	gc->cq_table = NULL;
> 
>  	mana_gd_remove_irqs(pdev);
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index c1310ea1c216..851de2b81fa4 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -309,9 +309,6 @@ static void mana_hwc_comp_event(void *ctx, struct
> gdma_queue *q_self)
> 
>  static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq
> *hwc_cq)  {
> -	if (!hwc_cq)
> -		return;
> -
>  	kfree(hwc_cq->comp_buf);
> 
>  	if (hwc_cq->gdma_cq)
> @@ -448,9 +445,6 @@ static void mana_hwc_dealloc_dma_buf(struct
> hw_channel_context *hwc,  static void mana_hwc_destroy_wq(struct
> hw_channel_context *hwc,
>  				struct hwc_wq *hwc_wq)
>  {
> -	if (!hwc_wq)
> -		return;
> -
>  	mana_hwc_dealloc_dma_buf(hwc, hwc_wq->msg_buf);
> 
>  	if (hwc_wq->gdma_wq)
> @@ -623,6 +617,7 @@ static int mana_hwc_establish_channel(struct
> gdma_context *gc, u16 *q_depth,
>  	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
>  	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
> 
> +	/* Both were set in mana_hwc_init_event_handler(). */
>  	if (WARN_ON(cq->id >= gc->max_num_cqs))
>  		return -EPROTO;
> 
> @@ -638,9 +633,6 @@ static int mana_hwc_establish_channel(struct
> gdma_context *gc, u16 *q_depth,  static int mana_hwc_init_queues(struct
> hw_channel_context *hwc, u16 q_depth,
>  				u32 max_req_msg_size, u32 max_resp_msg_size)  {
> -	struct hwc_wq *hwc_rxq = NULL;
> -	struct hwc_wq *hwc_txq = NULL;
> -	struct hwc_cq *hwc_cq = NULL;
>  	int err;
> 
>  	err = mana_hwc_init_inflight_msg(hwc, q_depth); @@ -653,44 +645,32
> @@ static int mana_hwc_init_queues(struct hw_channel_context *hwc, u16
> q_depth,
>  	err = mana_hwc_create_cq(hwc, q_depth * 2,
>  				 mana_hwc_init_event_handler, hwc,
>  				 mana_hwc_rx_event_handler, hwc,
> -				 mana_hwc_tx_event_handler, hwc, &hwc_cq);
> +				 mana_hwc_tx_event_handler, hwc, &hwc->cq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC CQ: %d\n", err);
>  		goto out;
>  	}
> -	hwc->cq = hwc_cq;
> 
>  	err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size,
> -				 hwc_cq, &hwc_rxq);
> +				 hwc->cq, &hwc->rxq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err);
>  		goto out;
>  	}
> -	hwc->rxq = hwc_rxq;
> 
>  	err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size,
> -				 hwc_cq, &hwc_txq);
> +				 hwc->cq, &hwc->txq);
>  	if (err) {
>  		dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err);
>  		goto out;
>  	}
> -	hwc->txq = hwc_txq;
> 
>  	hwc->num_inflight_msg = q_depth;
>  	hwc->max_req_msg_size = max_req_msg_size;
> 
>  	return 0;
>  out:
> -	if (hwc_txq)
> -		mana_hwc_destroy_wq(hwc, hwc_txq);
> -
> -	if (hwc_rxq)
> -		mana_hwc_destroy_wq(hwc, hwc_rxq);
> -
> -	if (hwc_cq)
> -		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc_cq);
> -
> -	mana_gd_free_res_map(&hwc->inflight_msg_res);
> +	/* mana_hwc_create_channel() will do the cleanup.*/
>  	return err;
>  }
> 
> @@ -718,6 +698,9 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  	gd->pdid = INVALID_PDID;
>  	gd->doorbell = INVALID_DOORBELL;
> 
> +	/* mana_hwc_init_queues() only creates the required data structures,
> +	 * and doesn't touch the HWC device.
> +	 */
>  	err = mana_hwc_init_queues(hwc, HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
>  				   HW_CHANNEL_MAX_REQUEST_SIZE,
>  				   HW_CHANNEL_MAX_RESPONSE_SIZE);
> @@ -743,42 +726,50 @@ int mana_hwc_create_channel(struct gdma_context
> *gc)
> 
>  	return 0;
>  out:
> -	kfree(hwc);
> +	mana_hwc_destroy_channel(gc);
>  	return err;
>  }
> 
>  void mana_hwc_destroy_channel(struct gdma_context *gc)  {
>  	struct hw_channel_context *hwc = gc->hwc.driver_data;
> -	struct hwc_caller_ctx *ctx;
> 
> -	mana_smc_teardown_hwc(&gc->shm_channel, false);
> +	if (!hwc)
> +		return;
> +
> +	/* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
> +	 * non-zero, the HWC worked and we should tear down the HWC here.
> +	 */
> +	if (gc->max_num_cqs > 0) {
> +		mana_smc_teardown_hwc(&gc->shm_channel, false);
> +		gc->max_num_cqs = 0;
> +	}
> 
> -	ctx = hwc->caller_ctx;
> -	kfree(ctx);
> +	kfree(hwc->caller_ctx);
>  	hwc->caller_ctx = NULL;
> 
> -	mana_hwc_destroy_wq(hwc, hwc->txq);
> -	hwc->txq = NULL;
> +	if (hwc->txq)
> +		mana_hwc_destroy_wq(hwc, hwc->txq);
> 
> -	mana_hwc_destroy_wq(hwc, hwc->rxq);
> -	hwc->rxq = NULL;
> +	if (hwc->rxq)
> +		mana_hwc_destroy_wq(hwc, hwc->rxq);
> 
> -	mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> -	hwc->cq = NULL;
> +	if (hwc->cq)
> +		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> 
>  	mana_gd_free_res_map(&hwc->inflight_msg_res);
> 
>  	hwc->num_inflight_msg = 0;
> 
> -	if (hwc->gdma_dev->pdid != INVALID_PDID) {
> -		hwc->gdma_dev->doorbell = INVALID_DOORBELL;
> -		hwc->gdma_dev->pdid = INVALID_PDID;
> -	}
> +	hwc->gdma_dev->doorbell = INVALID_DOORBELL;
> +	hwc->gdma_dev->pdid = INVALID_PDID;
> 
>  	kfree(hwc);
>  	gc->hwc.driver_data = NULL;
>  	gc->hwc.gdma_context = NULL;
> +
> +	vfree(gc->cq_table);
> +	gc->cq_table = NULL;
>  }
> 
>  int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
> --
> 2.17.1

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

[PATCH net-next 4/4] net: mana: Support hibernation and kexec

[Dexuan Cui]

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);
 
 	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

RE: [PATCH net-next 4/4] net: mana: Support hibernation and kexec

[Haiyang Zhang]

> -----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

RE: [PATCH net-next 4/4] net: mana: Support hibernation and kexec

[Dexuan Cui]

> 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;
}

RE: [PATCH net-next 4/4] net: mana: Support hibernation and kexec

[Haiyang Zhang]

> -----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

RE: [PATCH net-next 4/4] net: mana: Support hibernation and kexec

[Dexuan Cui]

> 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

Re: [PATCH net-next 0/4] net: mana: some misc patches

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 29 Oct 2021 17:54:04 -0700 you wrote:
> Hi all,
> 
> Patch 1 is a small fix.
> 
> Patch 2 reports OS info to the PF driver.
> Before the patch, the req fields were all zeros.
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] net: mana: Fix the netdev_err()'s vPort argument in mana_init_port()
    https://git.kernel.org/netdev/net-next/c/6c7ea69653e4
  - [net-next,2/4] net: mana: Report OS info to the PF driver
    https://git.kernel.org/netdev/net-next/c/3c37f3573508
  - [net-next,3/4] net: mana: Improve the HWC error handling
    https://git.kernel.org/netdev/net-next/c/62ea8b77ed3b
  - [net-next,4/4] net: mana: Support hibernation and kexec
    https://git.kernel.org/netdev/net-next/c/635096a86edb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html