Re: [PATCH V2 net-next 1/3] net: hns3: add command queue trace for hns3

From: Simon Horman
Date: Thu Dec 14 2023 - 16:04:55 EST

Next message: Mark Hasemeyer: "Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source"
Previous message: Sergey Shtylyov: "Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API"
In reply to: Jijie Shao: "[PATCH V2 net-next 1/3] net: hns3: add command queue trace for hns3"
Next in thread: Jijie Shao: "Re: [PATCH V2 net-next 1/3] net: hns3: add command queue trace for hns3"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Dec 14, 2023 at 10:11:33PM +0800, Jijie Shao wrote:
> From: Hao Lan <lanhao@xxxxxxxxxx>
>
> Currently, the hns3 driver does not have the trace
> of the command queue. As a result, it is difficult to
> locate the communication between the driver and firmware.
> Therefore, the trace function of the command queue is
> added in this test case to facilitate the locating of
> communication problems between the driver and firmware.
>
> Signed-off-by: Hao Lan <lanhao@xxxxxxxxxx>
> Signed-off-by: Jijie Shao <shaojijie@xxxxxxxxxx>
> ---
> .../hns3/hns3_common/hclge_comm_cmd.c | 18 ++++
> .../hns3/hns3_common/hclge_comm_cmd.h | 14 ++-
> .../hisilicon/hns3/hns3pf/hclge_main.c | 45 +++++++++
> .../hisilicon/hns3/hns3pf/hclge_trace.h | 94 +++++++++++++++++++
> .../hisilicon/hns3/hns3vf/hclgevf_main.c | 40 ++++++++
> .../hisilicon/hns3/hns3vf/hclgevf_trace.h | 50 ++++++++++
> 6 files changed, 260 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c
> index d92ad6082d8e..424555e00e05 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.c
> @@ -470,10 +470,14 @@ static int hclge_comm_cmd_check_result(struct hclge_comm_hw *hw,
> int hclge_comm_cmd_send(struct hclge_comm_hw *hw, struct hclge_desc *desc,
> int num)
> {
> + bool is_special = hclge_comm_is_special_opcode(desc->opcode);
> struct hclge_comm_cmq_ring *csq = &hw->cmq.csq;
> int ret;
> int ntc;

hclge_comm_is_special_opcode takes a u16 value as it's argument,
but the type of desc->opcode is __le16. So perhaps this should be
(completely untested!):

struct hclge_comm_cmq_ring *csq = &hw->cmq.csq;
bool is_special;
...

is_special = hclge_comm_is_special_opcode(le16_to_cpu(desc->opcode));

Flagged by Sparse.

>
> + if (hw->cmq.ops.trace_cmd_send)
> + hw->cmq.ops.trace_cmd_send(hw, desc, num, is_special);
> +
> spin_lock_bh(&hw->cmq.csq.lock);
>
> if (test_bit(HCLGE_COMM_STATE_CMD_DISABLE, &hw->comm_state)) {
> @@ -507,6 +511,9 @@ int hclge_comm_cmd_send(struct hclge_comm_hw *hw, struct hclge_desc *desc,
>
> spin_unlock_bh(&hw->cmq.csq.lock);
>
> + if (hw->cmq.ops.trace_cmd_get)
> + hw->cmq.ops.trace_cmd_get(hw, desc, num, is_special);
> +
> return ret;
> }
>
> @@ -584,6 +591,17 @@ int hclge_comm_cmd_queue_init(struct pci_dev *pdev, struct hclge_comm_hw *hw)
> return ret;
> }
>
> +void hclge_comm_cmd_init_ops(struct hclge_comm_hw *hw,
> + const struct hclge_comm_cmq_ops *ops)
> +{
> + struct hclge_comm_cmq *cmdq = &hw->cmq;
> +
> + if (ops) {
> + cmdq->ops.trace_cmd_send = ops->trace_cmd_send;
> + cmdq->ops.trace_cmd_get = ops->trace_cmd_get;
> + }
> +}
> +
> int hclge_comm_cmd_init(struct hnae3_ae_dev *ae_dev, struct hclge_comm_hw *hw,
> u32 *fw_version, bool is_pf,
> unsigned long reset_pending)
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h
> index 533c19d25e4f..387807b47d9a 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_common/hclge_comm_cmd.h
> @@ -423,11 +423,22 @@ enum hclge_comm_cmd_status {
> HCLGE_COMM_ERR_CSQ_ERROR = -3,
> };
>
> +struct hclge_comm_hw;
> +struct hclge_comm_cmq_ops {
> + void (*trace_cmd_send)(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int num, bool is_special);
> + void (*trace_cmd_get)(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int num, bool is_special);
> +};
> +
> struct hclge_comm_cmq {
> struct hclge_comm_cmq_ring csq;
> struct hclge_comm_cmq_ring crq;
> u16 tx_timeout;
> enum hclge_comm_cmd_status last_status;
> + struct hclge_comm_cmq_ops ops;
> };
>
> struct hclge_comm_hw {
> @@ -474,5 +485,6 @@ int hclge_comm_cmd_queue_init(struct pci_dev *pdev, struct hclge_comm_hw *hw);
> int hclge_comm_cmd_init(struct hnae3_ae_dev *ae_dev, struct hclge_comm_hw *hw,
> u32 *fw_version, bool is_pf,
> unsigned long reset_pending);
> -
> +void hclge_comm_cmd_init_ops(struct hclge_comm_hw *hw,
> + const struct hclge_comm_cmq_ops *ops);
> #endif
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index 5ea9e59569ef..8c2562ae3c69 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -27,6 +27,8 @@
> #include "hclge_devlink.h"
> #include "hclge_comm_cmd.h"
>
> +#include "hclge_trace.h"
> +
> #define HCLGE_NAME "hclge"
>
> #define HCLGE_BUF_SIZE_UNIT 256U
> @@ -391,6 +393,48 @@ int hclge_cmd_send(struct hclge_hw *hw, struct hclge_desc *desc, int num)
> return hclge_comm_cmd_send(&hw->hw, desc, num);
> }
>
> +static void hclge_trace_cmd_send(struct hclge_comm_hw *hw, struct hclge_desc *desc,
> + int num, bool is_special)
> +{
> + int i;
> +
> + trace_hclge_pf_cmd_send(hw, desc, 0, num);
> +
> + if (!is_special) {
> + for (i = 1; i < num; i++)
> + trace_hclge_pf_cmd_send(hw, &desc[i], i, num);
> + } else {
> + for (i = 1; i < num; i++)
> + trace_hclge_pf_special_cmd_send(hw, (u32 *)&desc[i],
> + i, num);
> + }
> +}
> +
> +static void hclge_trace_cmd_get(struct hclge_comm_hw *hw, struct hclge_desc *desc,
> + int num, bool is_special)
> +{
> + int i;
> +
> + if (!HCLGE_COMM_SEND_SYNC(le16_to_cpu(desc->flag)))
> + return;
> +
> + trace_hclge_pf_cmd_get(hw, desc, 0, num);
> +
> + if (!is_special) {
> + for (i = 1; i < num; i++)
> + trace_hclge_pf_cmd_get(hw, &desc[i], i, num);
> + } else {
> + for (i = 1; i < num; i++)
> + trace_hclge_pf_special_cmd_get(hw, (u32 *)&desc[i],
> + i, num);
> + }
> +}
> +
> +static const struct hclge_comm_cmq_ops hclge_cmq_ops = {
> + .trace_cmd_send = hclge_trace_cmd_send,
> + .trace_cmd_get = hclge_trace_cmd_get,
> +};
> +
> static int hclge_mac_update_stats_defective(struct hclge_dev *hdev)
> {
> #define HCLGE_MAC_CMD_NUM 21
> @@ -11616,6 +11660,7 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
> goto err_devlink_uninit;
>
> /* Firmware command initialize */
> + hclge_comm_cmd_init_ops(&hdev->hw.hw, &hclge_cmq_ops);
> ret = hclge_comm_cmd_init(hdev->ae_dev, &hdev->hw.hw, &hdev->fw_version,
> true, hdev->reset_pending);
> if (ret)
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_trace.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_trace.h
> index 8510b88d4982..116ac3a5ffc5 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_trace.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_trace.h
> @@ -10,6 +10,7 @@
>
> #include <linux/tracepoint.h>
>
> +#define PF_DESC_LEN (sizeof(struct hclge_desc) / sizeof(u32))
> #define PF_GET_MBX_LEN (sizeof(struct hclge_mbx_vf_to_pf_cmd) / sizeof(u32))
> #define PF_SEND_MBX_LEN (sizeof(struct hclge_mbx_pf_to_vf_cmd) / sizeof(u32))
>
> @@ -77,6 +78,99 @@ TRACE_EVENT(hclge_pf_mbx_send,
> )
> );
>
> +DECLARE_EVENT_CLASS(hclge_pf_cmd_template,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int index,
> + int num),
> + TP_ARGS(hw, desc, index, num),
> +
> + TP_STRUCT__entry(__field(u16, opcode)
> + __field(u16, flag)
> + __field(u16, retval)
> + __field(u16, rsv)
> + __field(int, index)
> + __field(int, num)
> + __string(pciname, pci_name(hw->cmq.csq.pdev))
> + __array(u32, data, HCLGE_DESC_DATA_LEN)),
> +
> + TP_fast_assign(int i;
> + __entry->opcode = le16_to_cpu(desc->opcode);
> + __entry->flag = le16_to_cpu(desc->flag);
> + __entry->retval = le16_to_cpu(desc->retval);
> + __entry->rsv = le16_to_cpu(desc->rsv);
> + __entry->index = index;
> + __entry->num = num;
> + __assign_str(pciname, pci_name(hw->cmq.csq.pdev));
> + for (i = 0; i < HCLGE_DESC_DATA_LEN; i++)
> + __entry->data[i] = le32_to_cpu(desc->data[i]);),
> +
> + TP_printk("%s opcode:0x%04x %d-%d flag:0x%04x retval:0x%04x rsv:0x%04x data:%s",
> + __get_str(pciname), __entry->opcode,
> + __entry->index, __entry->num,
> + __entry->flag, __entry->retval, __entry->rsv,
> + __print_array(__entry->data,
> + HCLGE_DESC_DATA_LEN, sizeof(u32)))
> +);
> +
> +DEFINE_EVENT(hclge_pf_cmd_template, hclge_pf_cmd_send,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int index,
> + int num),
> + TP_ARGS(hw, desc, index, num)
> +);
> +
> +DEFINE_EVENT(hclge_pf_cmd_template, hclge_pf_cmd_get,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int index,
> + int num),
> + TP_ARGS(hw, desc, index, num)
> +);
> +
> +DECLARE_EVENT_CLASS(hclge_pf_special_cmd_template,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + u32 *data,
> + int index,
> + int num),
> + TP_ARGS(hw, data, index, num),
> +
> + TP_STRUCT__entry(__field(int, index)
> + __field(int, num)
> + __string(pciname, pci_name(hw->cmq.csq.pdev))
> + __array(u32, data, PF_DESC_LEN)),
> +
> + TP_fast_assign(int i;
> + __entry->index = index;
> + __entry->num = num;
> + __assign_str(pciname, pci_name(hw->cmq.csq.pdev));
> + for (i = 0; i < PF_DESC_LEN; i++)
> + __entry->data[i] = le32_to_cpu(data[i]);
> + ),
> +
> + TP_printk("%s %d-%d data:%s",
> + __get_str(pciname),
> + __entry->index, __entry->num,
> + __print_array(__entry->data,
> + PF_DESC_LEN, sizeof(u32)))
> +);
> +
> +DEFINE_EVENT(hclge_pf_special_cmd_template, hclge_pf_special_cmd_send,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + u32 *desc,
> + int index,
> + int num),
> + TP_ARGS(hw, desc, index, num));
> +
> +DEFINE_EVENT(hclge_pf_special_cmd_template, hclge_pf_special_cmd_get,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + u32 *desc,
> + int index,
> + int num),
> + TP_ARGS(hw, desc, index, num)
> +);
> +
> #endif /* _HCLGE_TRACE_H_ */
>
> /* This must be outside ifdef _HCLGE_TRACE_H */
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> index 0aa9beefd1c7..ecc092555362 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> @@ -11,6 +11,7 @@
> #include "hnae3.h"
> #include "hclgevf_devlink.h"
> #include "hclge_comm_rss.h"
> +#include "hclgevf_trace.h"
>
> #define HCLGEVF_NAME "hclgevf"
>
> @@ -47,6 +48,42 @@ int hclgevf_cmd_send(struct hclgevf_hw *hw, struct hclge_desc *desc, int num)
> return hclge_comm_cmd_send(&hw->hw, desc, num);
> }
>
> +static void hclgevf_trace_cmd_send(struct hclge_comm_hw *hw, struct hclge_desc *desc,
> + int num, bool is_special)
> +{
> + int i;
> +
> + trace_hclge_vf_cmd_send(hw, desc, 0, num);
> +
> + if (is_special)
> + return;
> +
> + for (i = 1; i < num; i++)
> + trace_hclge_vf_cmd_send(hw, &desc[i], i, num);
> +}
> +
> +static void hclgevf_trace_cmd_get(struct hclge_comm_hw *hw, struct hclge_desc *desc,
> + int num, bool is_special)
> +{
> + int i;
> +
> + if (!HCLGE_COMM_SEND_SYNC(le16_to_cpu(desc->flag)))
> + return;
> +
> + trace_hclge_vf_cmd_get(hw, desc, 0, num);
> +
> + if (is_special)
> + return;
> +
> + for (i = 1; i < num; i++)
> + trace_hclge_vf_cmd_get(hw, &desc[i], i, num);
> +}
> +
> +static const struct hclge_comm_cmq_ops hclgevf_cmq_ops = {
> + .trace_cmd_send = hclgevf_trace_cmd_send,
> + .trace_cmd_get = hclgevf_trace_cmd_get,
> +};
> +
> void hclgevf_arq_init(struct hclgevf_dev *hdev)
> {
> struct hclge_comm_cmq *cmdq = &hdev->hw.hw.cmq;
> @@ -2796,6 +2833,7 @@ static int hclgevf_reset_hdev(struct hclgevf_dev *hdev)
> }
>
> hclgevf_arq_init(hdev);
> +
> ret = hclge_comm_cmd_init(hdev->ae_dev, &hdev->hw.hw,
> &hdev->fw_version, false,
> hdev->reset_pending);
> @@ -2854,6 +2892,8 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
> goto err_cmd_queue_init;
>
> hclgevf_arq_init(hdev);
> +
> + hclge_comm_cmd_init_ops(&hdev->hw.hw, &hclgevf_cmq_ops);
> ret = hclge_comm_cmd_init(hdev->ae_dev, &hdev->hw.hw,
> &hdev->fw_version, false,
> hdev->reset_pending);
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_trace.h b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_trace.h
> index 5d4895bb57a1..82263cb5e47b 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_trace.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_trace.h
> @@ -77,6 +77,56 @@ TRACE_EVENT(hclge_vf_mbx_send,
> )
> );
>
> +DECLARE_EVENT_CLASS(hclge_vf_cmd_template,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int index,
> + int num),
> +
> + TP_ARGS(hw, desc, index, num),
> +
> + TP_STRUCT__entry(__field(u16, opcode)
> + __field(u16, flag)
> + __field(u16, retval)
> + __field(u16, rsv)
> + __field(int, index)
> + __field(int, num)
> + __string(pciname, pci_name(hw->cmq.csq.pdev))
> + __array(u32, data, HCLGE_DESC_DATA_LEN)),
> +
> + TP_fast_assign(int i;
> + __entry->opcode = le16_to_cpu(desc->opcode);
> + __entry->flag = le16_to_cpu(desc->flag);
> + __entry->retval = le16_to_cpu(desc->retval);
> + __entry->rsv = le16_to_cpu(desc->rsv);
> + __entry->index = index;
> + __entry->num = num;
> + __assign_str(pciname, pci_name(hw->cmq.csq.pdev));
> + for (i = 0; i < HCLGE_DESC_DATA_LEN; i++)
> + __entry->data[i] = le32_to_cpu(desc->data[i]);),
> +
> + TP_printk("%s opcode:0x%04x %d-%d flag:0x%04x retval:0x%04x rsv:0x%04x data:%s",
> + __get_str(pciname), __entry->opcode,
> + __entry->index, __entry->num,
> + __entry->flag, __entry->retval, __entry->rsv,
> + __print_array(__entry->data,
> + HCLGE_DESC_DATA_LEN, sizeof(u32)))
> +);
> +
> +DEFINE_EVENT(hclge_vf_cmd_template, hclge_vf_cmd_send,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int index,
> + int num),
> + TP_ARGS(hw, desc, index, num));
> +
> +DEFINE_EVENT(hclge_vf_cmd_template, hclge_vf_cmd_get,
> + TP_PROTO(struct hclge_comm_hw *hw,
> + struct hclge_desc *desc,
> + int index,
> + int num),
> + TP_ARGS(hw, desc, index, num));
> +
> #endif /* _HCLGEVF_TRACE_H_ */
>
> /* This must be outside ifdef _HCLGEVF_TRACE_H */
> --
> 2.30.0
>

Next message: Mark Hasemeyer: "Re: [PATCH v1 2/6] arm: arm64: dts: Enable cros-ec-spi as wake source"
Previous message: Sergey Shtylyov: "Re: [PATCH net-next v2 11/21] net: ravb: Move DBAT configuration to the driver's ndo_open API"
In reply to: Jijie Shao: "[PATCH V2 net-next 1/3] net: hns3: add command queue trace for hns3"
Next in thread: Jijie Shao: "Re: [PATCH V2 net-next 1/3] net: hns3: add command queue trace for hns3"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]