Message ID | 1339004108-7356-3-git-send-email-anirban.chakraborty@qlogic.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, 2012-06-06 at 13:35 -0400, Anirban Chakraborty wrote: > From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> > > Add debug messages for FW CDRP command failure. trivia: Please be consistent with the use of (preferably _no_) periods at the end of logging messages. $ git grep -E "[^\.]\\\\n\"" drivers/net/ethernet/qlogic/qlcnic/ | wc -l 187 $ git grep -E "\.\\\\n\"" drivers/net/ethernet/qlogic/qlcnic/ | wc -l 22 [] > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c [] > @@ -53,12 +53,39 @@ qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *cmd) > rsp = qlcnic_poll_rsp(adapter); > > if (rsp == QLCNIC_CDRP_RSP_TIMEOUT) { > - dev_err(&pdev->dev, "card response timeout.\n"); > + dev_err(&pdev->dev, "CDRP response timeout.\n"); ie: no period necessary. CDRP is kind of an odd acronym. Is it for CarD ResPonse? If it is, then I think a lot of the below messages are not particularly sensible and the CDRP should be dropped. > cmd->rsp.cmd = QLCNIC_RCODE_TIMEOUT; > } else if (rsp == QLCNIC_CDRP_RSP_FAIL) { > cmd->rsp.cmd = QLCRD32(adapter, QLCNIC_ARG1_CRB_OFFSET); > - dev_err(&pdev->dev, "failed card response code:0x%x\n", > + switch (cmd->rsp.cmd) { > + case QLCNIC_RCODE_INVALID_ARGS: > + dev_err(&pdev->dev, "CDRP invalid args: 0x%x.\n", > cmd->rsp.cmd); > + break; > + case QLCNIC_RCODE_NOT_SUPPORTED: > + case QLCNIC_RCODE_NOT_IMPL: > + dev_err(&pdev->dev, > + "CDRP command not supported: 0x%x.\n", > + cmd->rsp.cmd); > + break; > + case QLCNIC_RCODE_NOT_PERMITTED: > + dev_err(&pdev->dev, > + "CDRP requested action not permitted: 0x%x.\n", > + cmd->rsp.cmd); > + break; > + case QLCNIC_RCODE_INVALID: > + dev_err(&pdev->dev, > + "CDRP invalid or unknown cmd received: 0x%x.\n", > + cmd->rsp.cmd); > + break; > + case QLCNIC_RCODE_TIMEOUT: > + dev_err(&pdev->dev, "CDRP command timeout: 0x%x.\n", > + cmd->rsp.cmd); > + break; > + default: > + dev_err(&pdev->dev, "CDRP command failed: 0x%x.\n", > + cmd->rsp.cmd); > + } > } else if (rsp == QLCNIC_CDRP_RSP_OK) { > cmd->rsp.cmd = QLCNIC_RCODE_SUCCESS; > if (cmd->rsp.arg2) > @@ -957,9 +984,6 @@ int qlcnic_get_mac_stats(struct qlcnic_adapter *adapter, > mac_stats->mac_rx_jabber = le64_to_cpu(stats->mac_rx_jabber); > mac_stats->mac_rx_dropped = le64_to_cpu(stats->mac_rx_dropped); > mac_stats->mac_rx_crc_error = le64_to_cpu(stats->mac_rx_crc_error); > - } else { > - dev_info(&adapter->pdev->dev, > - "%s: Get mac stats failed =%d.\n", __func__, err); > } > > dma_free_coherent(&adapter->pdev->dev, stats_size, stats_addr, -- 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
On 6/6/12 11:14 AM, "Joe Perches" <joe@perches.com> wrote: >On Wed, 2012-06-06 at 13:35 -0400, Anirban Chakraborty wrote: >> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> >> >> Add debug messages for FW CDRP command failure. > >trivia: > >Please be consistent with the use of (preferably _no_) periods >at the end of logging messages. > >$ git grep -E "[^\.]\\\\n\"" drivers/net/ethernet/qlogic/qlcnic/ | wc -l >187 >$ git grep -E "\.\\\\n\"" drivers/net/ethernet/qlogic/qlcnic/ | wc -l >22 > >[] >> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c >>b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c >[] >> @@ -53,12 +53,39 @@ qlcnic_issue_cmd(struct qlcnic_adapter *adapter, >>struct qlcnic_cmd_args *cmd) >> rsp = qlcnic_poll_rsp(adapter); >> >> if (rsp == QLCNIC_CDRP_RSP_TIMEOUT) { >> - dev_err(&pdev->dev, "card response timeout.\n"); >> + dev_err(&pdev->dev, "CDRP response timeout.\n"); > >ie: no period necessary. Thanks for pointing it out. We will not add that period in commit messages from next time on. > >CDRP is kind of an odd acronym. >Is it for CarD ResPonse? It stands for FW CommanD ResPonse. > >If it is, then I think a lot of the below messages are >not particularly sensible and the CDRP should be dropped. Having CDRRP in the message string helps us identify the source of error. This works well for us in debugging issues. -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 --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h index 520ff03..df4552f 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h @@ -612,7 +612,11 @@ struct qlcnic_recv_context { #define QLCNIC_CDRP_CMD_GET_MAC_STATS 0x00000037 #define QLCNIC_RCODE_SUCCESS 0 +#define QLCNIC_RCODE_INVALID_ARGS 6 #define QLCNIC_RCODE_NOT_SUPPORTED 9 +#define QLCNIC_RCODE_NOT_PERMITTED 10 +#define QLCNIC_RCODE_NOT_IMPL 15 +#define QLCNIC_RCODE_INVALID 16 #define QLCNIC_RCODE_TIMEOUT 17 #define QLCNIC_DESTROY_CTX_RESET 0 diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c index cfa174d..b8ead69 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c @@ -53,12 +53,39 @@ qlcnic_issue_cmd(struct qlcnic_adapter *adapter, struct qlcnic_cmd_args *cmd) rsp = qlcnic_poll_rsp(adapter); if (rsp == QLCNIC_CDRP_RSP_TIMEOUT) { - dev_err(&pdev->dev, "card response timeout.\n"); + dev_err(&pdev->dev, "CDRP response timeout.\n"); cmd->rsp.cmd = QLCNIC_RCODE_TIMEOUT; } else if (rsp == QLCNIC_CDRP_RSP_FAIL) { cmd->rsp.cmd = QLCRD32(adapter, QLCNIC_ARG1_CRB_OFFSET); - dev_err(&pdev->dev, "failed card response code:0x%x\n", + switch (cmd->rsp.cmd) { + case QLCNIC_RCODE_INVALID_ARGS: + dev_err(&pdev->dev, "CDRP invalid args: 0x%x.\n", cmd->rsp.cmd); + break; + case QLCNIC_RCODE_NOT_SUPPORTED: + case QLCNIC_RCODE_NOT_IMPL: + dev_err(&pdev->dev, + "CDRP command not supported: 0x%x.\n", + cmd->rsp.cmd); + break; + case QLCNIC_RCODE_NOT_PERMITTED: + dev_err(&pdev->dev, + "CDRP requested action not permitted: 0x%x.\n", + cmd->rsp.cmd); + break; + case QLCNIC_RCODE_INVALID: + dev_err(&pdev->dev, + "CDRP invalid or unknown cmd received: 0x%x.\n", + cmd->rsp.cmd); + break; + case QLCNIC_RCODE_TIMEOUT: + dev_err(&pdev->dev, "CDRP command timeout: 0x%x.\n", + cmd->rsp.cmd); + break; + default: + dev_err(&pdev->dev, "CDRP command failed: 0x%x.\n", + cmd->rsp.cmd); + } } else if (rsp == QLCNIC_CDRP_RSP_OK) { cmd->rsp.cmd = QLCNIC_RCODE_SUCCESS; if (cmd->rsp.arg2) @@ -957,9 +984,6 @@ int qlcnic_get_mac_stats(struct qlcnic_adapter *adapter, mac_stats->mac_rx_jabber = le64_to_cpu(stats->mac_rx_jabber); mac_stats->mac_rx_dropped = le64_to_cpu(stats->mac_rx_dropped); mac_stats->mac_rx_crc_error = le64_to_cpu(stats->mac_rx_crc_error); - } else { - dev_info(&adapter->pdev->dev, - "%s: Get mac stats failed =%d.\n", __func__, err); } dma_free_coherent(&adapter->pdev->dev, stats_size, stats_addr,