diff mbox

[net-next,2/2] qlcnic: Change CDRP function

Message ID 1315937179-22383-2-git-send-email-anirban.chakraborty@qlogic.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Anirban Chakraborty Sept. 13, 2011, 6:06 p.m. UTC 
Argument list to CDRP function has become unmanageably long. Fix it by properly
declaring a struct that encompasses all the input and output parameters.

Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic.h        |   21 +-
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c    |  374 +++++++++-----------
 .../net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c    |   12 +-
 3 files changed, 185 insertions(+), 222 deletions(-)

Comments

Joe Perches Sept. 13, 2011, 6:22 p.m. UTC | #1 
On Tue, 2011-09-13 at 11:06 -0700, Anirban Chakraborty wrote:
> Argument list to CDRP function has become unmanageably long. Fix it by properly
> declaring a struct that encompasses all the input and output parameters.

I don't think this is better.

I think you've overloaded the issue_cmd function and should
use separate issue_cmd_<type> functions instead.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anirban Chakraborty Sept. 13, 2011, 8:28 p.m. UTC | #2 
On Sep 13, 2011, at 11:22 AM, Joe Perches wrote:

> On Tue, 2011-09-13 at 11:06 -0700, Anirban Chakraborty wrote:
>> Argument list to CDRP function has become unmanageably long. Fix it by properly
>> declaring a struct that encompasses all the input and output parameters.
> 
> I don't think this is better.
> 
> I think you've overloaded the issue_cmd function and should
> use separate issue_cmd_<type> functions instead.

I did not overload issue_cmd(), instead I changed the function signature. 

-u32
-qlcnic_issue_cmd(struct qlcnic_adapter *adapter,
-       u32 pci_fn, u32 version, u32 arg1, u32 arg2, u32 arg3, u32 cmd,
-               u32 *rd_args[3])
+void
+qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *cmd)

Instead of having a long list of arguments, I put them into a struct and passed it around.

-Anirban






--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Sept. 13, 2011, 8:56 p.m. UTC | #3 
On Tue, 2011-09-13 at 13:28 -0700, Anirban Chakraborty wrote:
> On Sep 13, 2011, at 11:22 AM, Joe Perches wrote:
> > On Tue, 2011-09-13 at 11:06 -0700, Anirban Chakraborty wrote:
> >> Argument list to CDRP function has become unmanageably long. Fix it by properly
> >> declaring a struct that encompasses all the input and output parameters.
> > I don't think this is better.
> > I think you've overloaded the issue_cmd function and should
> > use separate issue_cmd_<type> functions instead.
> I did not overload issue_cmd(), instead I changed the function signature. 
> -u32
> -qlcnic_issue_cmd(struct qlcnic_adapter *adapter,
> -       u32 pci_fn, u32 version, u32 arg1, u32 arg2, u32 arg3, u32 cmd,
> -               u32 *rd_args[3])
> +void
> +qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *cmd)
> Instead of having a long list of arguments, I put them into a struct and passed it around.

Yes, I saw that.

I think that if restructuring is done, perhaps there are
better alternatives to bundling all the arguments into a
single struct.

I think it might be more sensible to have multiple
issue_cmd_<cmd>(adapter, [appropriate_args]) functions
instead so that the arguments and returns are appropriate
for the cmd performed.

or maybe use

issue_cmd(adapter, cmd, struct)

so you make cmd explicit and it's a bit easier to read
if you really must use some struct where cmd doesn't
use all the elements of struct.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anirban Chakraborty Sept. 13, 2011, 9:15 p.m. UTC | #4 
On Sep 13, 2011, at 1:56 PM, Joe Perches wrote:

>     On Tue, 2011-09-13 at 13:28 -0700, Anirban Chakraborty wrote:
>> On Sep 13, 2011, at 11:22 AM, Joe Perches wrote:
>>> On Tue, 2011-09-13 at 11:06 -0700, Anirban Chakraborty wrote:
>>>> Argument list to CDRP function has become unmanageably long. Fix it by properly
>>>> declaring a struct that encompasses all the input and output parameters.
>>> I don't think this is better.
>>> I think you've overloaded the issue_cmd function and should
>>> use separate issue_cmd_<type> functions instead.
>> I did not overload issue_cmd(), instead I changed the function signature. 
>> -u32
>> -qlcnic_issue_cmd(struct qlcnic_adapter *adapter,
>> -       u32 pci_fn, u32 version, u32 arg1, u32 arg2, u32 arg3, u32 cmd,
>> -               u32 *rd_args[3])
>> +void
>> +qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *cmd)
>> Instead of having a long list of arguments, I put them into a struct and passed it around.
> 
> Yes, I saw that.
> 
> I think that if restructuring is done, perhaps there are
> better alternatives to bundling all the arguments into a
> single struct.
> 
> I think it might be more sensible to have multiple
> issue_cmd_<cmd>(adapter, [appropriate_args]) functions
> instead so that the arguments and returns are appropriate
> for the cmd performed.
> 
> or maybe use
> 
> issue_cmd(adapter, cmd, struct)
> 
> so you make cmd explicit and it's a bit easier to read
> if you really must use some struct where cmd doesn't
> use all the elements of struct.

The usage of issue_cmd() here is to send a message to the FW. In some cases, the caller
of the function may want to read additional data right after the FW cmd is executed and in some
cases it doesn't.  As you mentioned, one way to do is to use two different issue_cmd()s. However,
we find it easier to use a single issue_cmd(), where the caller indicates whether it needs additional
data by setting the parameters in the argument. 

Thank you for your feedback.

-Anirban


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
index 04c7015..2fd1ba8 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
@@ -36,8 +36,8 @@ 
 
 #define _QLCNIC_LINUX_MAJOR 5
 #define _QLCNIC_LINUX_MINOR 0
-#define _QLCNIC_LINUX_SUBVERSION 23
-#define QLCNIC_LINUX_VERSIONID  "5.0.23"
+#define _QLCNIC_LINUX_SUBVERSION 24
+#define QLCNIC_LINUX_VERSIONID  "5.0.24"
 #define QLCNIC_DRV_IDC_VER  0x01
 #define QLCNIC_DRIVER_VERSION  ((_QLCNIC_LINUX_MAJOR << 16) |\
 		 (_QLCNIC_LINUX_MINOR << 8) | (_QLCNIC_LINUX_SUBVERSION))
@@ -583,6 +583,7 @@  struct qlcnic_recv_context {
 #define QLCNIC_CDRP_CMD_DESTROY_RX_CTX          0x00000008
 #define QLCNIC_CDRP_CMD_CREATE_TX_CTX           0x00000009
 #define QLCNIC_CDRP_CMD_DESTROY_TX_CTX          0x0000000a
+#define QLCNIC_CDRP_CMD_INTRPT_TEST		0x00000011
 #define QLCNIC_CDRP_CMD_SET_MTU                 0x00000012
 #define QLCNIC_CDRP_CMD_READ_PHY		0x00000013
 #define QLCNIC_CDRP_CMD_WRITE_PHY		0x00000014
@@ -1358,6 +1359,18 @@  struct qlcnic_dump_operations {
 			struct qlcnic_dump_entry *, u32 *);
 };
 
+struct _cdrp_cmd {
+	u32 cmd;
+	u32 arg1;
+	u32 arg2;
+	u32 arg3;
+};
+
+struct qlcnic_cmd_args {
+	struct _cdrp_cmd req;
+	struct _cdrp_cmd rsp;
+};
+
 int qlcnic_fw_cmd_get_minidump_temp(struct qlcnic_adapter *adapter);
 int qlcnic_fw_cmd_set_port(struct qlcnic_adapter *adapter, u32 config);
 
@@ -1470,9 +1483,7 @@  int qlcnic_check_loopback_buff(unsigned char *data, u8 mac[]);
 
 /* Functions from qlcnic_main.c */
 int qlcnic_reset_context(struct qlcnic_adapter *);
-u32 qlcnic_issue_cmd(struct qlcnic_adapter *adapter,
-	u32 pci_fn, u32 version, u32 arg1, u32 arg2, u32 arg3, u32 cmd,
-		u32 *rd_args[3]);
+void qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *);
 void qlcnic_diag_free_res(struct net_device *netdev, int max_sds_rings);
 int qlcnic_diag_alloc_res(struct net_device *netdev, int test);
 netdev_tx_t qlcnic_xmit_frame(struct sk_buff *skb, struct net_device *netdev);
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
index e0c85a5..569a837 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
@@ -26,50 +26,54 @@  qlcnic_poll_rsp(struct qlcnic_adapter *adapter)
 	return rsp;
 }
 
-u32
-qlcnic_issue_cmd(struct qlcnic_adapter *adapter,
-	u32 pci_fn, u32 version, u32 arg1, u32 arg2, u32 arg3, u32 cmd,
-		u32 *rd_args[3])
+void
+qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *cmd)
 {
 	u32 rsp;
 	u32 signature;
-	u32 rcode = QLCNIC_RCODE_SUCCESS;
 	struct pci_dev *pdev = adapter->pdev;
+	struct qlcnic_hardware_context *ahw = adapter->ahw;
 
-	signature = QLCNIC_CDRP_SIGNATURE_MAKE(pci_fn, version);
+	signature = QLCNIC_CDRP_SIGNATURE_MAKE(ahw->pci_func,
+		adapter->fw_hal_version);
 
 	/* Acquire semaphore before accessing CRB */
-	if (qlcnic_api_lock(adapter))
-		return QLCNIC_RCODE_TIMEOUT;
+	if (qlcnic_api_lock(adapter)) {
+		cmd->rsp.cmd = QLCNIC_RCODE_TIMEOUT;
+		return;
+	}
 
 	QLCWR32(adapter, QLCNIC_SIGN_CRB_OFFSET, signature);
-	QLCWR32(adapter, QLCNIC_ARG1_CRB_OFFSET, arg1);
-	QLCWR32(adapter, QLCNIC_ARG2_CRB_OFFSET, arg2);
-	QLCWR32(adapter, QLCNIC_ARG3_CRB_OFFSET, arg3);
-	QLCWR32(adapter, QLCNIC_CDRP_CRB_OFFSET, QLCNIC_CDRP_FORM_CMD(cmd));
+	QLCWR32(adapter, QLCNIC_ARG1_CRB_OFFSET, cmd->req.arg1);
+	QLCWR32(adapter, QLCNIC_ARG2_CRB_OFFSET, cmd->req.arg2);
+	QLCWR32(adapter, QLCNIC_ARG3_CRB_OFFSET, cmd->req.arg3);
+	QLCWR32(adapter, QLCNIC_CDRP_CRB_OFFSET,
+		QLCNIC_CDRP_FORM_CMD(cmd->req.cmd));
 
 	rsp = qlcnic_poll_rsp(adapter);
 
 	if (rsp == QLCNIC_CDRP_RSP_TIMEOUT) {
 		dev_err(&pdev->dev, "card response timeout.\n");
-		rcode = QLCNIC_RCODE_TIMEOUT;
+		cmd->rsp.cmd = QLCNIC_RCODE_TIMEOUT;
 	} else if (rsp == QLCNIC_CDRP_RSP_FAIL) {
-		rcode = QLCRD32(adapter, QLCNIC_ARG1_CRB_OFFSET);
+		cmd->rsp.cmd = QLCRD32(adapter, QLCNIC_ARG1_CRB_OFFSET);
 		dev_err(&pdev->dev, "failed card response code:0x%x\n",
-				rcode);
+				cmd->rsp.cmd);
 	} else if (rsp == QLCNIC_CDRP_RSP_OK) {
-		if (rd_args[1])
-			*rd_args[1] = QLCRD32(adapter, QLCNIC_ARG2_CRB_OFFSET);
-		if (rd_args[2])
-			*rd_args[2] = QLCRD32(adapter, QLCNIC_ARG3_CRB_OFFSET);
+		cmd->rsp.cmd = QLCNIC_RCODE_SUCCESS;
+		if (cmd->rsp.arg2)
+			cmd->rsp.arg2 = QLCRD32(adapter,
+				QLCNIC_ARG2_CRB_OFFSET);
+		if (cmd->rsp.arg3)
+			cmd->rsp.arg3 = QLCRD32(adapter,
+				QLCNIC_ARG3_CRB_OFFSET);
 	}
-	if (rd_args[0])
-		*rd_args[0] = QLCRD32(adapter, QLCNIC_ARG1_CRB_OFFSET);
+	if (cmd->rsp.arg1)
+		cmd->rsp.arg1 = QLCRD32(adapter, QLCNIC_ARG1_CRB_OFFSET);
 
 	/* Release semaphore */
 	qlcnic_api_unlock(adapter);
 
-	return rcode;
 }
 
 static uint32_t qlcnic_temp_checksum(uint32_t *temp_buffer, u16 temp_size)
@@ -88,30 +92,25 @@  int qlcnic_fw_cmd_get_minidump_temp(struct qlcnic_adapter *adapter)
 	int err, i;
 	u16 temp_size;
 	void *tmp_addr;
-	u32 version, csum, *template, *tmp_buf, rsp;
-	u32 *rd_args[3];
+	u32 version, csum, *template, *tmp_buf;
+	struct qlcnic_cmd_args cmd;
 	struct qlcnic_hardware_context *ahw;
 	struct qlcnic_dump_template_hdr *tmpl_hdr, *tmp_tmpl;
 	dma_addr_t tmp_addr_t = 0;
 
 	ahw = adapter->ahw;
-	rd_args[0] = &rsp;
-	rd_args[1] = (u32 *) &temp_size;
-	rd_args[2] = &version;
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			0,
-			0,
-			0,
-			QLCNIC_CDRP_CMD_TEMP_SIZE,
-			rd_args);
-	if (err != QLCNIC_RCODE_SUCCESS) {
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_TEMP_SIZE;
+	memset(&cmd.rsp, 1, sizeof(struct _cdrp_cmd));
+	qlcnic_issue_cmd(adapter, &cmd);
+	if (cmd.rsp.cmd != QLCNIC_RCODE_SUCCESS) {
 		dev_info(&adapter->pdev->dev,
-			"Can't get template size %d\n", rsp);
+			"Can't get template size %d\n", cmd.rsp.cmd);
 		err = -EIO;
 		return err;
 	}
+	temp_size = cmd.rsp.arg2;
+	version = cmd.rsp.arg3;
 	if (!temp_size)
 		return -EIO;
 
@@ -122,16 +121,14 @@  int qlcnic_fw_cmd_get_minidump_temp(struct qlcnic_adapter *adapter)
 			"Can't get memory for FW dump template\n");
 		return -ENOMEM;
 	}
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-		adapter->ahw->pci_func,
-		adapter->fw_hal_version,
-		LSD(tmp_addr_t),
-		MSD(tmp_addr_t),
-		temp_size,
-		QLCNIC_CDRP_CMD_GET_TEMP_HDR,
-		rd_args);
-
+	memset(&cmd.rsp, 0, sizeof(struct _cdrp_cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_GET_TEMP_HDR;
+	cmd.req.arg1 = LSD(tmp_addr_t);
+	cmd.req.arg2 = MSD(tmp_addr_t);
+	cmd.req.arg3 = temp_size;
+	qlcnic_issue_cmd(adapter, &cmd);
+
+	err = cmd.rsp.cmd;
 	if (err != QLCNIC_RCODE_SUCCESS) {
 		dev_err(&adapter->pdev->dev,
 			"Failed to get mini dump template header %d\n", err);
@@ -167,20 +164,17 @@  error:
 int
 qlcnic_fw_cmd_set_mtu(struct qlcnic_adapter *adapter, int mtu)
 {
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	struct qlcnic_recv_context *recv_ctx = adapter->recv_ctx;
 
-	memset(rd_args, 0, sizeof(rd_args));
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_SET_MTU;
+	cmd.req.arg1 = recv_ctx->context_id;
+	cmd.req.arg2 = mtu;
+	cmd.req.arg3 = 0;
 	if (recv_ctx->state == QLCNIC_HOST_CTX_STATE_ACTIVE) {
-		if (qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			recv_ctx->context_id,
-			mtu,
-			0,
-			QLCNIC_CDRP_CMD_SET_MTU,
-			rd_args)) {
-
+		qlcnic_issue_cmd(adapter, &cmd);
+		if (cmd.rsp.cmd) {
 			dev_err(&adapter->pdev->dev, "Failed to set mtu\n");
 			return -EIO;
 		}
@@ -201,6 +195,7 @@  qlcnic_fw_cmd_create_rx_ctx(struct qlcnic_adapter *adapter)
 	struct qlcnic_cardrsp_sds_ring *prsp_sds;
 	struct qlcnic_host_rds_ring *rds_ring;
 	struct qlcnic_host_sds_ring *sds_ring;
+	struct qlcnic_cmd_args cmd;
 
 	dma_addr_t hostrq_phys_addr, cardrsp_phys_addr;
 	u64 phys_addr;
@@ -208,7 +203,6 @@  qlcnic_fw_cmd_create_rx_ctx(struct qlcnic_adapter *adapter)
 	u8 i, nrds_rings, nsds_rings;
 	size_t rq_size, rsp_size;
 	u32 cap, reg, val, reg2;
-	u32 *rd_args[3];
 	int err;
 
 	struct qlcnic_recv_context *recv_ctx = adapter->recv_ctx;
@@ -290,15 +284,13 @@  qlcnic_fw_cmd_create_rx_ctx(struct qlcnic_adapter *adapter)
 	}
 
 	phys_addr = hostrq_phys_addr;
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			(u32)(phys_addr >> 32),
-			(u32)(phys_addr & 0xffffffff),
-			rq_size,
-			QLCNIC_CDRP_CMD_CREATE_RX_CTX,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.arg1 = (u32) (phys_addr >> 32);
+	cmd.req.arg2 = (u32) (phys_addr & 0xffffffff);
+	cmd.req.arg3 = rq_size;
+	cmd.req.cmd = QLCNIC_CDRP_CMD_CREATE_RX_CTX;
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 	if (err) {
 		dev_err(&adapter->pdev->dev,
 			"Failed to create rx ctx in firmware%d\n", err);
@@ -344,22 +336,18 @@  out_free_rq:
 static void
 qlcnic_fw_cmd_destroy_rx_ctx(struct qlcnic_adapter *adapter)
 {
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	struct qlcnic_recv_context *recv_ctx = adapter->recv_ctx;
 
-	memset(rd_args, 0, sizeof(rd_args));
-	if (qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			recv_ctx->context_id,
-			QLCNIC_DESTROY_CTX_RESET,
-			0,
-			QLCNIC_CDRP_CMD_DESTROY_RX_CTX,
-			rd_args)) {
-
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.arg1 = recv_ctx->context_id;
+	cmd.req.arg2 = QLCNIC_DESTROY_CTX_RESET;
+	cmd.req.arg3 = 0;
+	cmd.req.cmd = QLCNIC_CDRP_CMD_DESTROY_RX_CTX;
+	qlcnic_issue_cmd(adapter, &cmd);
+	if (cmd.rsp.cmd)
 		dev_err(&adapter->pdev->dev,
 			"Failed to destroy rx ctx in firmware\n");
-	}
 
 	recv_ctx->state = QLCNIC_HOST_CTX_STATE_FREED;
 }
@@ -373,7 +361,7 @@  qlcnic_fw_cmd_create_tx_ctx(struct qlcnic_adapter *adapter)
 	void	*rq_addr, *rsp_addr;
 	size_t	rq_size, rsp_size;
 	u32	temp;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	int	err;
 	u64	phys_addr;
 	dma_addr_t	rq_phys_addr, rsp_phys_addr;
@@ -423,15 +411,13 @@  qlcnic_fw_cmd_create_tx_ctx(struct qlcnic_adapter *adapter)
 	prq_cds->ring_size = cpu_to_le32(tx_ring->num_desc);
 
 	phys_addr = rq_phys_addr;
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			(u32)(phys_addr >> 32),
-			((u32)phys_addr & 0xffffffff),
-			rq_size,
-			QLCNIC_CDRP_CMD_CREATE_TX_CTX,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.arg1 = (u32)(phys_addr >> 32);
+	cmd.req.arg2 = ((u32)phys_addr & 0xffffffff);
+	cmd.req.arg3 = rq_size;
+	cmd.req.cmd = QLCNIC_CDRP_CMD_CREATE_TX_CTX;
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 
 	if (err == QLCNIC_RCODE_SUCCESS) {
 		temp = le32_to_cpu(prsp->cds_ring.host_producer_crb);
@@ -457,37 +443,30 @@  out_free_rq:
 static void
 qlcnic_fw_cmd_destroy_tx_ctx(struct qlcnic_adapter *adapter)
 {
-	u32 *rd_args[3];
-
-	memset(rd_args, 0, sizeof(rd_args));
-	if (qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			adapter->tx_context_id,
-			QLCNIC_DESTROY_CTX_RESET,
-			0,
-			QLCNIC_CDRP_CMD_DESTROY_TX_CTX,
-			rd_args)) {
-
+	struct qlcnic_cmd_args cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.arg1 = adapter->tx_context_id;
+	cmd.req.arg2 = QLCNIC_DESTROY_CTX_RESET;
+	cmd.req.arg3 = 0;
+	cmd.req.cmd = QLCNIC_CDRP_CMD_DESTROY_TX_CTX;
+	qlcnic_issue_cmd(adapter, &cmd);
+	if (cmd.rsp.cmd)
 		dev_err(&adapter->pdev->dev,
 			"Failed to destroy tx ctx in firmware\n");
-	}
 }
 
 int
 qlcnic_fw_cmd_set_port(struct qlcnic_adapter *adapter, u32 config)
 {
-	u32 *rd_args[3];
-
-	memset(rd_args, 0, sizeof(rd_args));
-	return qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			config,
-			0,
-			0,
-			QLCNIC_CDRP_CMD_CONFIG_PORT,
-			rd_args);
+	struct qlcnic_cmd_args cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.arg1 = config;
+	cmd.req.cmd = QLCNIC_CDRP_CMD_CONFIG_PORT;
+	qlcnic_issue_cmd(adapter, &cmd);
+
+	return cmd.rsp.cmd;
 }
 
 int qlcnic_alloc_hw_resources(struct qlcnic_adapter *adapter)
@@ -652,24 +631,17 @@  void qlcnic_free_hw_resources(struct qlcnic_adapter *adapter)
 int qlcnic_get_mac_address(struct qlcnic_adapter *adapter, u8 *mac)
 {
 	int err;
-	u32 arg1, rd_arg1, rd_arg2;
-	u32 *rd_args[3];
-
-	arg1 = adapter->ahw->pci_func | BIT_8;
-	rd_args[0] = &rd_arg1;
-	rd_args[1] = &rd_arg2;
-	rd_args[2] = 0;
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			arg1,
-			0,
-			0,
-			QLCNIC_CDRP_CMD_MAC_ADDRESS,
-			rd_args);
+	struct qlcnic_cmd_args cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.arg1 = adapter->ahw->pci_func | BIT_8;
+	cmd.req.cmd = QLCNIC_CDRP_CMD_MAC_ADDRESS;
+	cmd.rsp.arg1 = cmd.rsp.arg2 = 1;
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 
 	if (err == QLCNIC_RCODE_SUCCESS)
-		qlcnic_fetch_mac(adapter, rd_arg1, rd_arg2, 0, mac);
+		qlcnic_fetch_mac(adapter, cmd.rsp.arg1, cmd.rsp.arg2, 0, mac);
 	else {
 		dev_err(&adapter->pdev->dev,
 			"Failed to get mac address%d\n", err);
@@ -687,7 +659,7 @@  int qlcnic_get_nic_info(struct qlcnic_adapter *adapter,
 	dma_addr_t nic_dma_t;
 	struct qlcnic_info *nic_info;
 	void *nic_info_addr;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	size_t	nic_size = sizeof(struct qlcnic_info);
 
 	nic_info_addr = dma_alloc_coherent(&adapter->pdev->dev, nic_size,
@@ -697,15 +669,13 @@  int qlcnic_get_nic_info(struct qlcnic_adapter *adapter,
 	memset(nic_info_addr, 0, nic_size);
 
 	nic_info = nic_info_addr;
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			MSD(nic_dma_t),
-			LSD(nic_dma_t),
-			(func_id << 16 | nic_size),
-			QLCNIC_CDRP_CMD_GET_NIC_INFO,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_GET_NIC_INFO;
+	cmd.req.arg1 = MSD(nic_dma_t);
+	cmd.req.arg2 = LSD(nic_dma_t);
+	cmd.req.arg3 = (func_id << 16 | nic_size);
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 
 	if (err == QLCNIC_RCODE_SUCCESS) {
 		npar_info->pci_func = le16_to_cpu(nic_info->pci_func);
@@ -744,7 +714,7 @@  int qlcnic_set_nic_info(struct qlcnic_adapter *adapter, struct qlcnic_info *nic)
 	int err = -EIO;
 	dma_addr_t nic_dma_t;
 	void *nic_info_addr;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	struct qlcnic_info *nic_info;
 	size_t nic_size = sizeof(struct qlcnic_info);
 
@@ -770,15 +740,13 @@  int qlcnic_set_nic_info(struct qlcnic_adapter *adapter, struct qlcnic_info *nic)
 	nic_info->min_tx_bw = cpu_to_le16(nic->min_tx_bw);
 	nic_info->max_tx_bw = cpu_to_le16(nic->max_tx_bw);
 
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			MSD(nic_dma_t),
-			LSD(nic_dma_t),
-			((nic->pci_func << 16) | nic_size),
-			QLCNIC_CDRP_CMD_SET_NIC_INFO,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_SET_NIC_INFO;
+	cmd.req.arg1 = MSD(nic_dma_t);
+	cmd.req.arg2 = LSD(nic_dma_t);
+	cmd.req.arg3 = ((nic->pci_func << 16) | nic_size);
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 
 	if (err != QLCNIC_RCODE_SUCCESS) {
 		dev_err(&adapter->pdev->dev,
@@ -796,7 +764,7 @@  int qlcnic_get_pci_info(struct qlcnic_adapter *adapter,
 				struct qlcnic_pci_info *pci_info)
 {
 	int err = 0, i;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	dma_addr_t pci_info_dma_t;
 	struct qlcnic_pci_info *npar;
 	void *pci_info_addr;
@@ -810,15 +778,13 @@  int qlcnic_get_pci_info(struct qlcnic_adapter *adapter,
 	memset(pci_info_addr, 0, pci_size);
 
 	npar = pci_info_addr;
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			MSD(pci_info_dma_t),
-			LSD(pci_info_dma_t),
-			pci_size,
-			QLCNIC_CDRP_CMD_GET_PCI_INFO,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_GET_PCI_INFO;
+	cmd.req.arg1 = MSD(pci_info_dma_t);
+	cmd.req.arg2 = LSD(pci_info_dma_t);
+	cmd.req.arg3 = pci_size;
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 
 	if (err == QLCNIC_RCODE_SUCCESS) {
 		for (i = 0; i < QLCNIC_MAX_PCI_FUNC; i++, npar++, pci_info++) {
@@ -850,7 +816,7 @@  int qlcnic_config_port_mirroring(struct qlcnic_adapter *adapter, u8 id,
 {
 	int err = -EIO;
 	u32 arg1;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 
 	if (adapter->op_mode != QLCNIC_MGMT_FUNC ||
 		!(adapter->eswitch[id].flags & QLCNIC_SWITCH_ENABLE))
@@ -859,15 +825,11 @@  int qlcnic_config_port_mirroring(struct qlcnic_adapter *adapter, u8 id,
 	arg1 = id | (enable_mirroring ? BIT_4 : 0);
 	arg1 |= pci_func << 8;
 
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			arg1,
-			0,
-			0,
-			QLCNIC_CDRP_CMD_SET_PORTMIRRORING,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_SET_PORTMIRRORING;
+	cmd.req.arg1 = arg1;
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 
 	if (err != QLCNIC_RCODE_SUCCESS) {
 		dev_err(&adapter->pdev->dev,
@@ -890,7 +852,7 @@  int qlcnic_get_port_stats(struct qlcnic_adapter *adapter, const u8 func,
 	dma_addr_t stats_dma_t;
 	void *stats_addr;
 	u32 arg1;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	int err;
 
 	if (esw_stats == NULL)
@@ -914,15 +876,13 @@  int qlcnic_get_port_stats(struct qlcnic_adapter *adapter, const u8 func,
 	arg1 = func | QLCNIC_STATS_VERSION << 8 | QLCNIC_STATS_PORT << 12;
 	arg1 |= rx_tx << 15 | stats_size << 16;
 
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			arg1,
-			MSD(stats_dma_t),
-			LSD(stats_dma_t),
-			QLCNIC_CDRP_CMD_GET_ESWITCH_STATS,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_GET_ESWITCH_STATS;
+	cmd.req.arg1 = arg1;
+	cmd.req.arg2 = MSD(stats_dma_t);
+	cmd.req.arg3 = LSD(stats_dma_t);
+	qlcnic_issue_cmd(adapter, &cmd);
+	err = cmd.rsp.cmd;
 
 	if (!err) {
 		stats = stats_addr;
@@ -1003,7 +963,7 @@  int qlcnic_clear_esw_stats(struct qlcnic_adapter *adapter, const u8 func_esw,
 {
 
 	u32 arg1;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 
 	if (adapter->op_mode != QLCNIC_MGMT_FUNC)
 		return -EIO;
@@ -1024,15 +984,11 @@  int qlcnic_clear_esw_stats(struct qlcnic_adapter *adapter, const u8 func_esw,
 	arg1 = port | QLCNIC_STATS_VERSION << 8 | func_esw << 12;
 	arg1 |= BIT_14 | rx_tx << 15;
 
-	memset(rd_args, 0, sizeof(rd_args));
-	return qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			arg1,
-			0,
-			0,
-			QLCNIC_CDRP_CMD_GET_ESWITCH_STATS,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_GET_ESWITCH_STATS;
+	cmd.req.arg1 = arg1;
+	qlcnic_issue_cmd(adapter, &cmd);
+	return cmd.rsp.cmd;
 
 err_ret:
 	dev_err(&adapter->pdev->dev, "Invalid argument func_esw=%d port=%d"
@@ -1045,20 +1001,17 @@  __qlcnic_get_eswitch_port_config(struct qlcnic_adapter *adapter,
 					u32 *arg1, u32 *arg2)
 {
 	int err = -EIO;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	u8 pci_func;
 	pci_func = (*arg1 >> 8);
-	rd_args[0] = arg1;
-	rd_args[1] = arg2;
-	rd_args[2] = 0;
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			*arg1,
-			0,
-			0,
-			QLCNIC_CDRP_CMD_GET_ESWITCH_PORT_CONFIG,
-			rd_args);
+
+	cmd.req.cmd = QLCNIC_CDRP_CMD_GET_ESWITCH_PORT_CONFIG;
+	cmd.req.arg1 = *arg1;
+	cmd.rsp.arg1 = cmd.rsp.arg2 = 1;
+	qlcnic_issue_cmd(adapter, &cmd);
+	*arg1 = cmd.rsp.arg1;
+	*arg2 = cmd.rsp.arg2;
+	err = cmd.rsp.cmd;
 
 	if (err == QLCNIC_RCODE_SUCCESS) {
 		dev_info(&adapter->pdev->dev,
@@ -1082,7 +1035,7 @@  int qlcnic_config_switch_port(struct qlcnic_adapter *adapter,
 {
 	int err = -EIO;
 	u32 arg1, arg2 = 0;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 	u8 pci_func;
 
 	if (adapter->op_mode != QLCNIC_MGMT_FUNC)
@@ -1129,16 +1082,13 @@  int qlcnic_config_switch_port(struct qlcnic_adapter *adapter,
 		return err;
 	}
 
-	memset(rd_args, 0, sizeof(rd_args));
-	err = qlcnic_issue_cmd(adapter,
-			adapter->ahw->pci_func,
-			adapter->fw_hal_version,
-			arg1,
-			arg2,
-			0,
-			QLCNIC_CDRP_CMD_CONFIGURE_ESWITCH,
-			rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_CONFIGURE_ESWITCH;
+	cmd.req.arg1 = arg1;
+	cmd.req.arg2 = arg2;
+	qlcnic_issue_cmd(adapter, &cmd);
 
+	err = cmd.rsp.cmd;
 	if (err != QLCNIC_RCODE_SUCCESS) {
 		dev_err(&adapter->pdev->dev,
 			"Failed to configure eswitch pci func %d\n", pci_func);
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
index b127f80..11f4df7 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
@@ -659,7 +659,7 @@  static int qlcnic_irq_test(struct net_device *netdev)
 	struct qlcnic_adapter *adapter = netdev_priv(netdev);
 	int max_sds_rings = adapter->max_sds_rings;
 	int ret;
-	u32 *rd_args[3];
+	struct qlcnic_cmd_args cmd;
 
 	if (test_and_set_bit(__QLCNIC_RESETTING, &adapter->state))
 		return -EIO;
@@ -669,10 +669,12 @@  static int qlcnic_irq_test(struct net_device *netdev)
 		goto clear_it;
 
 	adapter->diag_cnt = 0;
-	memset(rd_args, 0, sizeof(rd_args));
-	ret = qlcnic_issue_cmd(adapter, adapter->ahw->pci_func,
-			adapter->fw_hal_version, adapter->ahw->pci_func,
-			0, 0, 0x00000011, rd_args);
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.req.cmd = QLCNIC_CDRP_CMD_INTRPT_TEST;
+	cmd.req.arg1 = adapter->ahw->pci_func;
+	qlcnic_issue_cmd(adapter, &cmd);
+	ret = cmd.rsp.cmd;
+
 	if (ret)
 		goto done;