[PATCH 0/5] firewire: core/ohci: add tracepoints events for bus-reset

[PATCH 0/5] firewire: core/ohci: add tracepoints events for bus-reset

[Takashi Sakamoto]

Hi,

IEEE 1394 bus is under bus-reset state when the physical state of bus is
changed; e.g. bus topology change by adding new nodes in the bus. It is
important to handle the state appropriately for the maintenance of bus.

This series of change adds some tracepoints events to trace the events
related to bus-reset. Some kernel log messages are obsoleted and
deleted. It also includes for 1394 OHCI driver so that bus-reset IRQ
event is recorded as much as possible, and obsoletes bus-resets bit
from debug parameter successfully.

Takashi Sakamoto (5):
  firewire: ohci: add bus-reset event for initial set of handled irq
  firewire: ohci: obsolete OHCI_PARAM_DEBUG_BUSRESETS from debug module
    parameter
  firewire: core: add tracepoints events for initiating bus reset
  Revert "firewire: core: option to log bus reset initiation"
  firewire: core: add tracepoint event for handling bus reset

 drivers/firewire/core-card.c        | 13 +++---
 drivers/firewire/core-topology.c    |  3 ++
 drivers/firewire/core-transaction.c |  7 ----
 drivers/firewire/core.h             |  4 --
 drivers/firewire/ohci.c             | 18 +++------
 include/trace/events/firewire.h     | 61 ++++++++++++++++++++++++++++-
 6 files changed, 73 insertions(+), 33 deletions(-)

-- 
2.43.0

[PATCH 1/5] firewire: ohci: add bus-reset event for initial set of handled irq

[Takashi Sakamoto]

In the former commits, the spurious interrupt events are suppressed as
possible, by unset bus-reset event from the set of handled irq. The change
was written with the less-intrusive style, thus it firstly works at the
second time to handle the event. But it is slightly inconvenient.

This commit adds the event for the initial set of irq to handle. As a
result, the event can be handled even if it is the first time. The change
has a benefit that the OHCI_PARAM_DEBUG_BUSRESETS bit in debug module
parameter is always effective.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/ohci.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 6116153f0ce6..d69629d8ba71 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -394,7 +394,7 @@ MODULE_PARM_DESC(quirks, "Chip quirks (default = 0"
 #define OHCI_PARAM_DEBUG_AT_AR		1
 #define OHCI_PARAM_DEBUG_SELFIDS	2
 #define OHCI_PARAM_DEBUG_IRQS		4
-#define OHCI_PARAM_DEBUG_BUSRESETS	8 /* only effective before chip init */
+#define OHCI_PARAM_DEBUG_BUSRESETS	8
 
 static int param_debug;
 module_param_named(debug, param_debug, int, 0644);
@@ -2062,8 +2062,7 @@ static void bus_reset_work(struct work_struct *work)
 
 	ohci->generation = generation;
 	reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset);
-	if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
-		reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset);
+	reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset);
 
 	if (ohci->quirks & QUIRK_RESET_PACKET)
 		ohci->request_generation = generation;
@@ -2135,6 +2134,7 @@ static irqreturn_t irq_handler(int irq, void *data)
 	reg_write(ohci, OHCI1394_IntEventClear,
 		  event & ~(OHCI1394_busReset | OHCI1394_postedWriteErr));
 	log_irqs(ohci, event);
+	// The flag is masked again at bus_reset_work() scheduled by selfID event.
 	if (event & OHCI1394_busReset)
 		reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset);
 
@@ -2474,9 +2474,8 @@ static int ohci_enable(struct fw_card *card,
 		OHCI1394_cycleInconsistent |
 		OHCI1394_unrecoverableError |
 		OHCI1394_cycleTooLong |
-		OHCI1394_masterIntEnable;
-	if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
-		irqs |= OHCI1394_busReset;
+		OHCI1394_masterIntEnable |
+		OHCI1394_busReset;
 	reg_write(ohci, OHCI1394_IntMaskSet, irqs);
 
 	reg_write(ohci, OHCI1394_HCControlSet,
-- 
2.43.0

[PATCH 2/5] firewire: ohci: obsolete OHCI_PARAM_DEBUG_BUSRESETS from debug module parameter

[Takashi Sakamoto]

The OHCI_PARAM_DEBUG_BUSRESETS bit of debug module parameter was added at
a commit a007bb857e0b ("firewire: fw-ohci: conditionally log busReset
interrupts").

At the former commit, the bit becomes less meaningful, just to skip
logging.

This commit obsoletes it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/ohci.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index d69629d8ba71..93dca3216e45 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -394,7 +394,6 @@ MODULE_PARM_DESC(quirks, "Chip quirks (default = 0"
 #define OHCI_PARAM_DEBUG_AT_AR		1
 #define OHCI_PARAM_DEBUG_SELFIDS	2
 #define OHCI_PARAM_DEBUG_IRQS		4
-#define OHCI_PARAM_DEBUG_BUSRESETS	8
 
 static int param_debug;
 module_param_named(debug, param_debug, int, 0644);
@@ -402,7 +401,6 @@ MODULE_PARM_DESC(debug, "Verbose logging (default = 0"
 	", AT/AR events = "	__stringify(OHCI_PARAM_DEBUG_AT_AR)
 	", self-IDs = "		__stringify(OHCI_PARAM_DEBUG_SELFIDS)
 	", IRQs = "		__stringify(OHCI_PARAM_DEBUG_IRQS)
-	", busReset events = "	__stringify(OHCI_PARAM_DEBUG_BUSRESETS)
 	", or a combination, or all = -1)");
 
 static bool param_remote_dma;
@@ -411,12 +409,7 @@ MODULE_PARM_DESC(remote_dma, "Enable unfiltered remote DMA (default = N)");
 
 static void log_irqs(struct fw_ohci *ohci, u32 evt)
 {
-	if (likely(!(param_debug &
-			(OHCI_PARAM_DEBUG_IRQS | OHCI_PARAM_DEBUG_BUSRESETS))))
-		return;
-
-	if (!(param_debug & OHCI_PARAM_DEBUG_IRQS) &&
-	    !(evt & OHCI1394_busReset))
+	if (likely(!(param_debug & OHCI_PARAM_DEBUG_IRQS)))
 		return;
 
 	ohci_notice(ohci, "IRQ %08x%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n", evt,
-- 
2.43.0

[PATCH 3/5] firewire: core: add tracepoints events for initiating bus reset

[Takashi Sakamoto]

At a commit 673249124304 ("firewire: core: option to log bus reset
initiation"), some kernel log messages were added to trace initiation of
bus reset. The kernel log messages are really helpful, while nowadays it
is not preferable just for debugging purpose. For the purpose, Linux
kernel tracepoints is more preferable.

This commit adds some alternative tracepoints events.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c    |  7 +++++++
 include/trace/events/firewire.h | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index dac1b0fc7a42..5d43acf45a7d 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -23,6 +23,7 @@
 #include <asm/byteorder.h>
 
 #include "core.h"
+#include <trace/events/firewire.h>
 
 #define define_fw_printk_level(func, kern_level)		\
 void func(const struct fw_card *card, const char *fmt, ...)	\
@@ -221,6 +222,8 @@ static int reset_bus(struct fw_card *card, bool short_reset)
 	int reg = short_reset ? 5 : 1;
 	int bit = short_reset ? PHY_BUS_SHORT_RESET : PHY_BUS_RESET;
 
+	trace_bus_reset_initiate(card->generation, short_reset);
+
 	if (unlikely(fw_core_param_debug & FW_CORE_PARAM_DEBUG_BUSRESETS))
 		fw_notice(card, "initiating %s bus reset\n",
 		          short_reset ? "short" : "long");
@@ -230,6 +233,8 @@ static int reset_bus(struct fw_card *card, bool short_reset)
 
 void fw_schedule_bus_reset(struct fw_card *card, bool delayed, bool short_reset)
 {
+	trace_bus_reset_schedule(card->generation, short_reset);
+
 	if (unlikely(fw_core_param_debug & FW_CORE_PARAM_DEBUG_BUSRESETS))
 		fw_notice(card, "scheduling %s bus reset\n",
 		          short_reset ? "short" : "long");
@@ -252,6 +257,8 @@ static void br_work(struct work_struct *work)
 	/* Delay for 2s after last reset per IEEE 1394 clause 8.2.1. */
 	if (card->reset_jiffies != 0 &&
 	    time_before64(get_jiffies_64(), card->reset_jiffies + 2 * HZ)) {
+		trace_bus_reset_postpone(card->generation, card->br_short);
+
 		if (unlikely(fw_core_param_debug & FW_CORE_PARAM_DEBUG_BUSRESETS))
 			fw_notice(card, "delaying bus reset\n");
 		if (!queue_delayed_work(fw_workqueue, &card->br_work, 2 * HZ))
diff --git a/include/trace/events/firewire.h b/include/trace/events/firewire.h
index db49b9828bd1..92bcbe69bb42 100644
--- a/include/trace/events/firewire.h
+++ b/include/trace/events/firewire.h
@@ -284,6 +284,39 @@ TRACE_EVENT(async_phy_inbound,
 	)
 );
 
+DECLARE_EVENT_CLASS(bus_reset_arrange_template,
+	TP_PROTO(unsigned int generation, bool short_reset),
+	TP_ARGS(generation, short_reset),
+	TP_STRUCT__entry(
+		__field(u8, generation)
+		__field(bool, short_reset)
+	),
+	TP_fast_assign(
+		__entry->generation = generation;
+		__entry->short_reset = short_reset;
+	),
+	TP_printk(
+		"generation=%u short_reset=%s",
+		__entry->generation,
+		__entry->short_reset ? "true" : "false"
+	)
+);
+
+DEFINE_EVENT(bus_reset_arrange_template, bus_reset_initiate,
+	TP_PROTO(unsigned int generation, bool short_reset),
+	TP_ARGS(generation, short_reset)
+);
+
+DEFINE_EVENT(bus_reset_arrange_template, bus_reset_schedule,
+	TP_PROTO(unsigned int generation, bool short_reset),
+	TP_ARGS(generation, short_reset)
+);
+
+DEFINE_EVENT(bus_reset_arrange_template, bus_reset_postpone,
+	TP_PROTO(unsigned int generation, bool short_reset),
+	TP_ARGS(generation, short_reset)
+);
+
 #endif // _FIREWIRE_TRACE_EVENT_H
 
 #include <trace/define_trace.h>
-- 
2.43.0

[PATCH 4/5] Revert "firewire: core: option to log bus reset initiation"

[Takashi Sakamoto]

This reverts commit 6732491243045f5a7e1995b4be5f3c964b579ebd.

The former commit adds some alternative tracepoints events to replace the
reverted kernel log messages.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c        | 10 ----------
 drivers/firewire/core-transaction.c |  7 -------
 drivers/firewire/core.h             |  4 ----
 3 files changed, 21 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 5d43acf45a7d..127d87e3a153 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -224,10 +224,6 @@ static int reset_bus(struct fw_card *card, bool short_reset)
 
 	trace_bus_reset_initiate(card->generation, short_reset);
 
-	if (unlikely(fw_core_param_debug & FW_CORE_PARAM_DEBUG_BUSRESETS))
-		fw_notice(card, "initiating %s bus reset\n",
-		          short_reset ? "short" : "long");
-
 	return card->driver->update_phy_reg(card, reg, 0, bit);
 }
 
@@ -235,10 +231,6 @@ void fw_schedule_bus_reset(struct fw_card *card, bool delayed, bool short_reset)
 {
 	trace_bus_reset_schedule(card->generation, short_reset);
 
-	if (unlikely(fw_core_param_debug & FW_CORE_PARAM_DEBUG_BUSRESETS))
-		fw_notice(card, "scheduling %s bus reset\n",
-		          short_reset ? "short" : "long");
-
 	/* We don't try hard to sort out requests of long vs. short resets. */
 	card->br_short = short_reset;
 
@@ -259,8 +251,6 @@ static void br_work(struct work_struct *work)
 	    time_before64(get_jiffies_64(), card->reset_jiffies + 2 * HZ)) {
 		trace_bus_reset_postpone(card->generation, card->br_short);
 
-		if (unlikely(fw_core_param_debug & FW_CORE_PARAM_DEBUG_BUSRESETS))
-			fw_notice(card, "delaying bus reset\n");
 		if (!queue_delayed_work(fw_workqueue, &card->br_work, 2 * HZ))
 			fw_card_put(card);
 		return;
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index d3eefbf23663..571fdff65c2b 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -1390,12 +1390,5 @@ static void __exit fw_core_cleanup(void)
 	idr_destroy(&fw_device_idr);
 }
 
-int fw_core_param_debug;
-module_param_named(debug, fw_core_param_debug, int, 0644);
-MODULE_PARM_DESC(debug, "Verbose logging (default = 0"
-	", bus resets = "	__stringify(FW_CORE_PARAM_DEBUG_BUSRESETS)
-	")");
-
-
 module_init(fw_core_init);
 module_exit(fw_core_cleanup);
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 5097c7a270b4..7c36d2628e37 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -245,10 +245,6 @@ static inline bool tcode_is_link_internal(unsigned int tcode)
 /* OHCI-1394's default upper bound for physical DMA: 4 GB */
 #define FW_MAX_PHYSICAL_RANGE		(1ULL << 32)
 
-#define FW_CORE_PARAM_DEBUG_BUSRESETS	1
-
-extern int fw_core_param_debug;
-
 void fw_core_handle_request(struct fw_card *card, struct fw_packet *request);
 void fw_core_handle_response(struct fw_card *card, struct fw_packet *packet);
 int fw_get_response_length(struct fw_request *request);
-- 
2.43.0

[PATCH 5/5] firewire: core: add tracepoint event for handling bus reset

[Takashi Sakamoto]

The core function expects hardware drivers to call
fw_core_handle_bus_reset() when changing bus topology. The 1394 OHCI
driver calls it when handling selfID event as a result of any bus-reset.

This commit adds a tracepoints event for it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-topology.c |  3 +++
 include/trace/events/firewire.h  | 28 +++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index f40c81534381..837cc44d8d9f 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -20,6 +20,7 @@
 #include <asm/byteorder.h>
 
 #include "core.h"
+#include <trace/events/firewire.h>
 
 #define SELF_ID_PHY_ID(q)		(((q) >> 24) & 0x3f)
 #define SELF_ID_EXTENDED(q)		(((q) >> 23) & 0x01)
@@ -507,6 +508,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 	struct fw_node *local_node;
 	unsigned long flags;
 
+	trace_bus_reset_handle(generation, node_id, bm_abdicate, self_ids, self_id_count);
+
 	spin_lock_irqsave(&card->lock, flags);
 
 	/*
diff --git a/include/trace/events/firewire.h b/include/trace/events/firewire.h
index 92bcbe69bb42..4163959be57a 100644
--- a/include/trace/events/firewire.h
+++ b/include/trace/events/firewire.h
@@ -204,7 +204,6 @@ DEFINE_EVENT(async_outbound_complete_template, async_response_outbound_complete,
 #undef ASYNC_HEADER_GET_SOURCE
 #undef ASYNC_HEADER_GET_OFFSET
 #undef ASYNC_HEADER_GET_RCODE
-#undef QUADLET_SIZE
 
 TRACE_EVENT(async_phy_outbound_initiate,
 	TP_PROTO(u64 packet, unsigned int generation, u32 first_quadlet, u32 second_quadlet),
@@ -317,6 +316,33 @@ DEFINE_EVENT(bus_reset_arrange_template, bus_reset_postpone,
 	TP_ARGS(generation, short_reset)
 );
 
+TRACE_EVENT(bus_reset_handle,
+	TP_PROTO(unsigned int generation, unsigned int node_id, bool bm_abdicate, u32 *self_ids, unsigned int self_id_count),
+	TP_ARGS(generation, node_id, bm_abdicate, self_ids, self_id_count),
+	TP_STRUCT__entry(
+		__field(u8, generation)
+		__field(u8, node_id)
+		__field(bool, bm_abdicate)
+		__dynamic_array(u32, self_ids, self_id_count)
+	),
+	TP_fast_assign(
+		__entry->generation = generation;
+		__entry->node_id = node_id;
+		__entry->bm_abdicate = bm_abdicate;
+		memcpy(__get_dynamic_array(self_ids), self_ids, __get_dynamic_array_len(self_ids));
+	),
+	TP_printk(
+		"generation=%u node_id=0x%04x bm_abdicate=%s self_ids=%s",
+		__entry->generation,
+		__entry->node_id,
+		__entry->bm_abdicate ? "true" : "false",
+		__print_array(__get_dynamic_array(self_ids),
+			      __get_dynamic_array_len(self_ids) / QUADLET_SIZE, QUADLET_SIZE)
+	)
+);
+
+#undef QUADLET_SIZE
+
 #endif // _FIREWIRE_TRACE_EVENT_H
 
 #include <trace/define_trace.h>
-- 
2.43.0

Re: [PATCH 0/5] firewire: core/ohci: add tracepoints events for bus-reset

[Takashi Sakamoto]

Hi,

On Wed, May 01, 2024 at 04:32:33PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> IEEE 1394 bus is under bus-reset state when the physical state of bus is
> changed; e.g. bus topology change by adding new nodes in the bus. It is
> important to handle the state appropriately for the maintenance of bus.
> 
> This series of change adds some tracepoints events to trace the events
> related to bus-reset. Some kernel log messages are obsoleted and
> deleted. It also includes for 1394 OHCI driver so that bus-reset IRQ
> event is recorded as much as possible, and obsoletes bus-resets bit
> from debug parameter successfully.
> 
> Takashi Sakamoto (5):
>   firewire: ohci: add bus-reset event for initial set of handled irq
>   firewire: ohci: obsolete OHCI_PARAM_DEBUG_BUSRESETS from debug module
>     parameter
>   firewire: core: add tracepoints events for initiating bus reset
>   Revert "firewire: core: option to log bus reset initiation"
>   firewire: core: add tracepoint event for handling bus reset
> 
>  drivers/firewire/core-card.c        | 13 +++---
>  drivers/firewire/core-topology.c    |  3 ++
>  drivers/firewire/core-transaction.c |  7 ----
>  drivers/firewire/core.h             |  4 --
>  drivers/firewire/ohci.c             | 18 +++------
>  include/trace/events/firewire.h     | 61 ++++++++++++++++++++++++++++-
>  6 files changed, 73 insertions(+), 33 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto