* [PATCH V2] nvme-core: add async event trace helper
@ 2018-08-29 22:19 Chaitanya Kulkarni
2018-09-11 7:19 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Chaitanya Kulkarni @ 2018-08-29 22:19 UTC (permalink / raw)
This patch adds a new event for nvme async event notification.
We print the async event in the decoded format when we recognize
the event otherwise we just dump the result.
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
Changes since V1:-
1. __stringify() all the known NVME_AER* status codes.
---
drivers/nvme/host/core.c | 24 ++++++++++++++++++++++++
drivers/nvme/host/trace.h | 22 ++++++++++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dd8ec1dd9219..80b5b0451e95 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3411,16 +3411,24 @@ static void nvme_fw_act_work(struct work_struct *work)
static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
{
+ const char *str;
+
switch ((result & 0xff00) >> 8) {
case NVME_AER_NOTICE_NS_CHANGED:
+ str = __stringify(NVME_AER_NOTICE_NS_CHANGED);
+ trace_nvme_async_event(ctrl, (result & 0xff00) >> 8, str);
set_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events);
nvme_queue_scan(ctrl);
break;
case NVME_AER_NOTICE_FW_ACT_STARTING:
+ str = __stringify(NVME_AER_NOTICE_FW_ACT_STARTING);
+ trace_nvme_async_event(ctrl, (result & 0xff00) >> 8, str);
queue_work(nvme_wq, &ctrl->fw_act_work);
break;
#ifdef CONFIG_NVME_MULTIPATH
case NVME_AER_NOTICE_ANA:
+ str = __stringify(NVME_AER_NOTICE_ANA)
+ trace_nvme_async_event(ctrl, (result & 0xff00) >> 8, str);
if (!ctrl->ana_log_buf)
break;
queue_work(nvme_wq, &ctrl->ana_work);
@@ -3434,6 +3442,7 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
volatile union nvme_result *res)
{
+ const char *str;
u32 result = le32_to_cpu(res->u32);
if (le16_to_cpu(status) >> 1 != NVME_SC_SUCCESS)
@@ -3444,12 +3453,27 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
nvme_handle_aen_notice(ctrl, result);
break;
case NVME_AER_ERROR:
+ str = __stringify(NVME_AER_ERROR);
+ trace_nvme_async_event(ctrl, result & 0x7, str);
+ ctrl->aen_result = result;
+ break;
case NVME_AER_SMART:
+ str = __stringify(NVME_AER_SMART);
+ trace_nvme_async_event(ctrl, result & 0x7, str);
+ ctrl->aen_result = result;
+ break;
case NVME_AER_CSS:
+ str = __stringify(NVME_AER_CSS);
+ trace_nvme_async_event(ctrl, result & 0x7, str);
+ ctrl->aen_result = result;
+ break;
case NVME_AER_VS:
+ str = __stringify(NVME_AER_VS);
+ trace_nvme_async_event(ctrl, result & 0x7, str);
ctrl->aen_result = result;
break;
default:
+ trace_nvme_async_event(ctrl, result, "unknown");
break;
}
queue_work(nvme_wq, &ctrl->async_event_work);
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index a490790d6691..a8c502cb0281 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -156,6 +156,28 @@ TRACE_EVENT(nvme_complete_rq,
);
+TRACE_EVENT(nvme_async_event,
+ TP_PROTO(struct nvme_ctrl *ctrl,
+ u32 result,
+ const char *result_str),
+ TP_ARGS(ctrl,
+ result,
+ result_str),
+ TP_STRUCT__entry(
+ __field(int, ctrl_id)
+ __field(u32, result)
+ __field(const char *, result_str)
+ ),
+ TP_fast_assign(
+ __entry->ctrl_id = ctrl->instance;
+ __entry->result = result;
+ __entry->result_str = result_str;
+ ),
+ TP_printk("nvme%d: NVME_AEN=%#08x [%s]",
+ __entry->ctrl_id, __entry->result, __entry->result_str)
+
+ );
+
#endif /* _TRACE_NVME_H */
#undef TRACE_INCLUDE_PATH
--
2.17.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH V2] nvme-core: add async event trace helper
2018-08-29 22:19 [PATCH V2] nvme-core: add async event trace helper Chaitanya Kulkarni
@ 2018-09-11 7:19 ` Christoph Hellwig
2018-09-12 5:51 ` Chaitanya Kulkarni
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2018-09-11 7:19 UTC (permalink / raw)
On Wed, Aug 29, 2018@03:19:22PM -0700, Chaitanya Kulkarni wrote:
> This patch adds a new event for nvme async event notification.
> We print the async event in the decoded format when we recognize
> the event otherwise we just dump the result.
I don't like the reason_str handling. The normal way to do this
would be to but the binary reason (i.e. what we switch on) into
the trace buffer, and then use __print_symbolic in the TP_printk
statement.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH V2] nvme-core: add async event trace helper
2018-09-11 7:19 ` Christoph Hellwig
@ 2018-09-12 5:51 ` Chaitanya Kulkarni
2018-09-14 10:03 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Chaitanya Kulkarni @ 2018-09-12 5:51 UTC (permalink / raw)
What do you think of the following implementation?
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ef6748f4ff47..d6adb3b50926 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3413,14 +3413,17 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
{
switch ((result & 0xff00) >> 8) {
case NVME_AER_NOTICE_NS_CHANGED:
+ trace_nvme_async_event(ctrl, (result & 0xff00) >> 8);
set_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events);
nvme_queue_scan(ctrl);
break;
case NVME_AER_NOTICE_FW_ACT_STARTING:
+ trace_nvme_async_event(ctrl, (result & 0xff00) >> 8);
queue_work(nvme_wq, &ctrl->fw_act_work);
break;
#ifdef CONFIG_NVME_MULTIPATH
case NVME_AER_NOTICE_ANA:
+ trace_nvme_async_event(ctrl, (result & 0xff00) >> 8);
if (!ctrl->ana_log_buf)
break;
queue_work(nvme_wq, &ctrl->ana_work);
@@ -3447,6 +3450,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
case NVME_AER_SMART:
case NVME_AER_CSS:
case NVME_AER_VS:
+ trace_nvme_async_event(ctrl, result & 0x7);
ctrl->aen_result = result;
break;
default:
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index a490790d6691..9c07fea357b8 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -156,6 +156,36 @@ TRACE_EVENT(nvme_complete_rq,
);
+#define aer_name(aer) { aer, #aer }
+
+TRACE_EVENT(nvme_async_event,
+ TP_PROTO(struct nvme_ctrl *ctrl,
+ u32 result),
+ TP_ARGS(ctrl,
+ result),
+ TP_STRUCT__entry(
+ __field(int, ctrl_id)
+ __field(u32, result)
+ ),
+ TP_fast_assign(
+ __entry->ctrl_id = ctrl->instance;
+ __entry->result = result;
+ ),
+ TP_printk("nvme%d: NVME_AEN=%#08x [%s]",
+ __entry->ctrl_id, __entry->result,
+ __print_symbolic(__entry->result,
+ aer_name(NVME_AER_NOTICE_NS_CHANGED),
+#ifdef CONFIG_NVME_MULTIPATH
+ aer_name(NVME_AER_NOTICE_ANA)
+#endif
+ aer_name(NVME_AER_NOTICE_FW_ACT_STARTING),
+ aer_name(NVME_AER_ERROR),
+ aer_name(NVME_AER_SMART),
+ aer_name(NVME_AER_CSS),
+ aer_name(NVME_AER_VS))
+)
+ );
+
#endif /* _TRACE_NVME_H */
#undef TRACE_INCLUDE_PATH
Regards,
Chaitanya
From: Christoph Hellwig <hch@infradead.org>
Sent: Tuesday, September 11, 2018 12:19 AM
To: Chaitanya Kulkarni
Cc: linux-nvme at lists.infradead.org
Subject: Re: [PATCH V2] nvme-core: add async event trace helper
?
On Wed, Aug 29, 2018@03:19:22PM -0700, Chaitanya Kulkarni wrote:
> This patch adds a new event for nvme async event notification.
> We print the async event in the decoded format when we recognize
> the event otherwise we just dump the result.
I don't like the reason_str handling.? The normal way to do this
would be to but the binary reason (i.e. what we switch on) into
the trace buffer, and then use __print_symbolic in the TP_printk
statement.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH V2] nvme-core: add async event trace helper
2018-09-12 5:51 ` Chaitanya Kulkarni
@ 2018-09-14 10:03 ` Christoph Hellwig
0 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2018-09-14 10:03 UTC (permalink / raw)
On Wed, Sep 12, 2018@05:51:23AM +0000, Chaitanya Kulkarni wrote:
> switch ((result & 0xff00) >> 8) {
> case NVME_AER_NOTICE_NS_CHANGED:
> + trace_nvme_async_event(ctrl, (result & 0xff00) >> 8);
> set_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events);
> nvme_queue_scan(ctrl);
> break;
> case NVME_AER_NOTICE_FW_ACT_STARTING:
> + trace_nvme_async_event(ctrl, (result & 0xff00) >> 8);
> queue_work(nvme_wq, &ctrl->fw_act_work);
> break;
> #ifdef CONFIG_NVME_MULTIPATH
> case NVME_AER_NOTICE_ANA:
> + trace_nvme_async_event(ctrl, (result & 0xff00) >> 8);
I think we want a local variable so that we only do the mask and shift
once.
> +#ifdef CONFIG_NVME_MULTIPATH
> + aer_name(NVME_AER_NOTICE_ANA)
> +#endif
I don't think we need the ifdef as the symbol is always defined.
Otherwise this looks good to me.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-14 10:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-29 22:19 [PATCH V2] nvme-core: add async event trace helper Chaitanya Kulkarni
2018-09-11 7:19 ` Christoph Hellwig
2018-09-12 5:51 ` Chaitanya Kulkarni
2018-09-14 10:03 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).