linux-kernel - Re: [PATCH] usb: dwc3: Log dwc3 instance name in traces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250902234450.vdair2jjrtpmpdal@synopsys.com>
Date: Tue, 2 Sep 2025 23:44:52 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Prashanth K <prashanth.k@....qualcomm.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: Log dwc3 instance name in traces

On Mon, Sep 01, 2025, Prashanth K wrote:
> 
> 
> On 8/29/2025 4:18 AM, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Mon, Aug 25, 2025, Prashanth K wrote:
> >> When multiple DWC3 controllers are being used, trace events from
> >> different instances get mixed up making debugging difficult as
> >> there's no way to distinguish which instance generated the trace.
> >>
> >> Append the device name to trace events to clearly identify the
> >> source instance.
> > 
> > Can we print the base address instead of the device name? This will be
> > consistent across different device names, and it will be easier to
> > create filter.
> > 
> Did you mean to print the iomem (base address) directly?
> I think using device name is more readable, in most cases device name
> would contain the base address also. Let me know if you are pointing to
> something else.>>

Yes, I mean the device base address. PCI devices won't have the base
address as part of the device name.

> >> Example trace output,
> >> before ->  dwc3_event: event (00000101): Reset [U0]
> >> after  ->  dwc3_event: a600000.usb: event (00000101): Reset [U0]
> >>
> >> Signed-off-by: Prashanth K <prashanth.k@....qualcomm.com>
> >> ---
> >>  drivers/usb/dwc3/ep0.c    |  2 +-
> >>  drivers/usb/dwc3/gadget.c |  2 +-
> >>  drivers/usb/dwc3/gadget.h |  1 +
> >>  drivers/usb/dwc3/io.h     | 12 ++++---
> >>  drivers/usb/dwc3/trace.h  | 76 ++++++++++++++++++++++++---------------
> >>  5 files changed, 59 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> >> index 666ac432f52d..b814bbba18ac 100644
> >> --- a/drivers/usb/dwc3/ep0.c
> >> +++ b/drivers/usb/dwc3/ep0.c
> >> @@ -830,7 +830,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
> >>  	if (!dwc->gadget_driver || !dwc->softconnect || !dwc->connected)
> >>  		goto out;
> >>  
> >> -	trace_dwc3_ctrl_req(ctrl);
> >> +	trace_dwc3_ctrl_req(dwc, ctrl);
> >>  
> >>  	len = le16_to_cpu(ctrl->wLength);
> >>  	if (!len) {
> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >> index 25db36c63951..e3621cc318ea 100644
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -271,7 +271,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> >>  		status = -ETIMEDOUT;
> >>  	}
> >>  
> >> -	trace_dwc3_gadget_generic_cmd(cmd, param, status);
> >> +	trace_dwc3_gadget_generic_cmd(dwc, cmd, param, status);
> >>  
> >>  	return ret;
> >>  }
> >> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> >> index d73e735e4081..dc9985523ed8 100644
> >> --- a/drivers/usb/dwc3/gadget.h
> >> +++ b/drivers/usb/dwc3/gadget.h
> >> @@ -131,6 +131,7 @@ int dwc3_gadget_start_config(struct dwc3 *dwc, unsigned int resource_index);
> >>  static inline void dwc3_gadget_ep_get_transfer_index(struct dwc3_ep *dep)
> >>  {
> >>  	u32			res_id;
> >> +	struct dwc3		*dwc = dep->dwc;
> > 
> > This looks unused when it's not.
> > 
> >>  
> >>  	res_id = dwc3_readl(dep->regs, DWC3_DEPCMD);
> >>  	dep->resource_index = DWC3_DEPCMD_GET_RSC_IDX(res_id);
> >> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
> >> index 1e96ea339d48..8e8eb3265676 100644
> >> --- a/drivers/usb/dwc3/io.h
> >> +++ b/drivers/usb/dwc3/io.h
> >> @@ -16,7 +16,11 @@
> >>  #include "debug.h"
> >>  #include "core.h"
> >>  
> >> -static inline u32 dwc3_readl(void __iomem *base, u32 offset)
> >> +/* Note: Caller must have a reference to dwc3 structure */
> >> +#define dwc3_readl(b, o) __dwc3_readl(dwc, b, o)
> >> +#define dwc3_writel(b, o, v) __dwc3_writel(dwc, b, o, v)
> > 
> > Can we not do this. This will be hard to read. If we just print the base
> > address, this will be simpler.
> > 
> > Thanks,
> > Thinh
> > 
> 
> This is just a wrapper macro for readl/writel APIs. I tried using
> container_of in dwc3_readl/writel() to get the dwc pointer,
> container_of(base, struct dwc3, regs))
> but this causes trouble since we use dep->regs in some cases,
> thats why i used a wrapper macro instead.
> 

We already have the base address in the argument. Just print that.
There's no need to use container_of.

BR,
Thinh