Received-SPF: pass (google.com: domain of linux-kernel+bounces-116359-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Feedback-ID: ie8e14432:Fastmail
Date: Mon, 25 Mar 2024 09:41:34 +0900
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Adam Goldman <adamg@pobox.com>
Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] firewire: core: option to log bus reset initiation
Message-ID: <20240325004134.GA21329@workstation.local>
Mail-Followup-To: Adam Goldman <adamg@pobox.com>,
	linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
References: <Zfqo43xhFluOgO01@iguana.24-8.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zfqo43xhFluOgO01@iguana.24-8.net>

Hi Adam,

On Wed, Mar 20, 2024 at 02:14:11AM -0700, Adam Goldman wrote:
> Add a debug parameter to firewire-core, analogous to the one in 
> firewire-ohci. When this is set to 1, log when we schedule, delay, or 
> initiate a bus reset. Since FireWire bus resets can originate from any 
> node on the bus, specific logging of the resets we initiate provides 
> additional insight.
> 
> Signed-off-by: Adam Goldman <adamg@pobox.com>

Thanks for the patch. I applied it to for-next[1].

Now we have two debug parameters per module for the slightly-similar
purpose. In my opinion, it is a pretty cumbersome to enable them when
checking bus-reset behaviour. I think it is time to investigate the other
way.

Linux Kernel Tracepoints[2] is one of options. Roughly describing, the
tracepoints mechanism allows users to deliver structured data from kernel
space to user space via ring-buffer when enabling it by either sysfs or
kernel command-line parameters. Linux kernel also has a command-line
parameter to redirect the human-readable formatted data to kernel log[3].
I think it is suitable in the case.

It requires many work to replace the existent debug parameter of
firewire-ohci, while it is a good start to work just for bus-reset debug.
The data structure layout should be pre-defined in each subsystem, thus we
need to decide it. In my opinion, it would be like:

```
struct bus_reset_event {
    enum reason {
        Initiate,
	Schedule,
	Postpone,
	Detect,
    },
    // We can put any other data if prefering.
}
```

Would I ask your opinion about my idea?


[1] https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git/log/?h=for-next
[2] https://docs.kernel.org/trace/tracepoints.html
[3] 'tp_printk' in kernel/trace/trace.c, 'trace_event' in kernel/trace/trace_event.c

Regards

Takashi Sakamoto