[PATCH] usbmon: Report device address assigned to USB device

[PATCH] usbmon: Report device address assigned to USB device

[Tomasz Moń]

Make USB device addresses match while sniffing USB communication
with usbmon and hardware USB sniffer (OpenVizsla) at the same time.
On xHCI root hubs the address is assigned by hardware and can be
different than devnum.

Signed-off-by: Tomasz Moń <desowin@gmail.com>
---
 Documentation/usb/usbmon.rst |  2 +-
 drivers/usb/mon/mon_bin.c    |  6 +++---
 drivers/usb/mon/mon_text.c   | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
index b0bd51080799..e603159f8a93 100644
--- a/Documentation/usb/usbmon.rst
+++ b/Documentation/usb/usbmon.rst
@@ -220,7 +220,7 @@ the following structure (its name is made up, so that we can refer to it)::
 	unsigned char type;	/*  8: Same as text; extensible. */
 	unsigned char xfer_type; /*    ISO (0), Intr, Control, Bulk (3) */
 	unsigned char epnum;	/*     Endpoint number and transfer direction */
-	unsigned char devnum;	/*     Device address */
+	unsigned char devaddr;	/*     Device address */
 	u16 busnum;		/* 12: Bus number */
 	char flag_setup;	/* 14: Same as text */
 	char flag_data;		/* 15: Same as text; Binary zero is OK. */
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f48a23adbc35..d7a55be5df68 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -91,7 +91,7 @@ struct mon_bin_hdr {
 	unsigned char type;	/* Same as in text API; extensible. */
 	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
 	unsigned char epnum;	/* Endpoint number and transfer direction */
-	unsigned char devnum;	/* Device address */
+	unsigned char devaddr;	/* Device address */
 	unsigned short busnum;	/* Bus number */
 	char flag_setup;
 	char flag_data;
@@ -567,7 +567,7 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
 	ep->type = ev_type;
 	ep->xfer_type = xfer_to_pipe[usb_endpoint_type(epd)];
 	ep->epnum = dir | usb_endpoint_num(epd);
-	ep->devnum = urb->dev->devnum;
+	ep->devaddr = urb->dev->devaddr;
 	ep->busnum = urb->dev->bus->busnum;
 	ep->id = (unsigned long) urb;
 	ep->ts_sec = ts.tv_sec;
@@ -655,7 +655,7 @@ static void mon_bin_error(void *data, struct urb *urb, int error)
 	ep->xfer_type = xfer_to_pipe[usb_endpoint_type(&urb->ep->desc)];
 	ep->epnum = usb_urb_dir_in(urb) ? USB_DIR_IN : 0;
 	ep->epnum |= usb_endpoint_num(&urb->ep->desc);
-	ep->devnum = urb->dev->devnum;
+	ep->devaddr = urb->dev->devaddr;
 	ep->busnum = urb->dev->bus->busnum;
 	ep->id = (unsigned long) urb;
 	ep->ts_sec = ts.tv_sec;
diff --git a/drivers/usb/mon/mon_text.c b/drivers/usb/mon/mon_text.c
index bc5ecd5ff565..8f0d4a4416ef 100644
--- a/drivers/usb/mon/mon_text.c
+++ b/drivers/usb/mon/mon_text.c
@@ -59,7 +59,7 @@ struct mon_event_text {
 	unsigned long id;	/* From pointer, most of the time */
 	unsigned int tstamp;
 	int busnum;
-	char devnum;
+	char devaddr;
 	char epnum;
 	char is_in;
 	char xfertype;
@@ -210,7 +210,7 @@ static void mon_text_event(struct mon_reader_text *rp, struct urb *urb,
 	ep->type = ev_type;
 	ep->id = (unsigned long) urb;
 	ep->busnum = urb->dev->bus->busnum;
-	ep->devnum = urb->dev->devnum;
+	ep->devaddr = urb->dev->devaddr;
 	ep->epnum = usb_endpoint_num(&urb->ep->desc);
 	ep->xfertype = usb_endpoint_type(&urb->ep->desc);
 	ep->is_in = usb_urb_dir_in(urb);
@@ -282,7 +282,7 @@ static void mon_text_error(void *data, struct urb *urb, int error)
 	ep->type = 'E';
 	ep->id = (unsigned long) urb;
 	ep->busnum = urb->dev->bus->busnum;
-	ep->devnum = urb->dev->devnum;
+	ep->devaddr = urb->dev->devaddr;
 	ep->epnum = usb_endpoint_num(&urb->ep->desc);
 	ep->xfertype = usb_endpoint_type(&urb->ep->desc);
 	ep->is_in = usb_urb_dir_in(urb);
@@ -523,7 +523,7 @@ static void mon_text_read_head_t(struct mon_reader_text *rp,
 	p->cnt += snprintf(p->pbuf + p->cnt, p->limit - p->cnt,
 	    "%lx %u %c %c%c:%03u:%02u",
 	    ep->id, ep->tstamp, ep->type,
-	    utype, udir, ep->devnum, ep->epnum);
+	    utype, udir, ep->devaddr, ep->epnum);
 }
 
 static void mon_text_read_head_u(struct mon_reader_text *rp,
@@ -541,7 +541,7 @@ static void mon_text_read_head_u(struct mon_reader_text *rp,
 	p->cnt += snprintf(p->pbuf + p->cnt, p->limit - p->cnt,
 	    "%lx %u %c %c%c:%d:%03u:%u",
 	    ep->id, ep->tstamp, ep->type,
-	    utype, udir, ep->busnum, ep->devnum, ep->epnum);
+	    utype, udir, ep->busnum, ep->devaddr, ep->epnum);
 }
 
 static void mon_text_read_statset(struct mon_reader_text *rp,
-- 
2.24.1

Re: [PATCH] usbmon: Report device address assigned to USB device

[Pete Zaitcev]

On Mon,  6 Jan 2020 10:37:17 +0100
Tomasz Moń <desowin@gmail.com> wrote:

> Make USB device addresses match while sniffing USB communication
> with usbmon and hardware USB sniffer (OpenVizsla) at the same time.
> On xHCI root hubs the address is assigned by hardware and can be
> different than devnum.

> -	unsigned char devnum;	/*     Device address */
> +	unsigned char devaddr;	/*     Device address */

I think it's fine, the name of the field is not exported by a header
anyway, so there's no impact.

Acked-by: Pete Zaitcev <zaitcev@redhat.com>

-- Pete

Re: [PATCH] usbmon: Report device address assigned to USB device

[Greg Kroah-Hartman]

On Mon, Jan 06, 2020 at 10:37:17AM +0100, Tomasz Moń wrote:
> Make USB device addresses match while sniffing USB communication
> with usbmon and hardware USB sniffer (OpenVizsla) at the same time.
> On xHCI root hubs the address is assigned by hardware and can be
> different than devnum.

This breaks the userspace abi for matching up the devnum with the number
that is listed by the kernel to userspace, making it really hard to
match things up for xhci devices now :(

I understand the need to look at this data, but you can't do it in a way
that will change things like this.

thanks,

greg k-h

Re: [PATCH] usbmon: Report device address assigned to USB device

[Pete Zaitcev]

On Wed, 8 Jan 2020 16:55:13 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Mon, Jan 06, 2020 at 10:37:17AM +0100, Tomasz Moń wrote:
> > Make USB device addresses match while sniffing USB communication
> > with usbmon and hardware USB sniffer (OpenVizsla) at the same time.
> > On xHCI root hubs the address is assigned by hardware and can be
> > different than devnum.
> 
> This breaks the userspace abi for matching up the devnum with the number
> that is listed by the kernel to userspace, making it really hard to
> match things up for xhci devices now :(
> 
> I understand the need to look at this data, but you can't do it in a way
> that will change things like this.

This sounds reasonable, although perhaps unfortunate. I acked Tomasz's
patch because I thought that XHCI is new, and thus it's no big deal
if its results are different.

Of course I rushed to examine the packet structure, but yes, there's
no space in the header, not without some trickery. For example, the
upper 8 bits of the ID are probably the same for all packets, so
it may be stuffed in there. Or, it might be possible to create an
extra header and attach it at the end of ISO descriptors.

Alan's suggestion of leaving the physical address in /sys appeals
the most to me, honestly. One thing though, libpcap will need to
rifle through /sys and then store that address, so its serialization
has to be changed no matter what. Unfortunately, I'm wholly ignorant
as to what syntax it uses and how extensible it is.

-- Pete

Re: [PATCH] usbmon: Report device address assigned to USB device

[Alan Stern]

On Sat, 11 Jan 2020, Pete Zaitcev wrote:

> On Wed, 8 Jan 2020 16:55:13 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Mon, Jan 06, 2020 at 10:37:17AM +0100, Tomasz Moń wrote:
> > > Make USB device addresses match while sniffing USB communication
> > > with usbmon and hardware USB sniffer (OpenVizsla) at the same time.
> > > On xHCI root hubs the address is assigned by hardware and can be
> > > different than devnum.
> > 
> > This breaks the userspace abi for matching up the devnum with the number
> > that is listed by the kernel to userspace, making it really hard to
> > match things up for xhci devices now :(
> > 
> > I understand the need to look at this data, but you can't do it in a way
> > that will change things like this.
> 
> This sounds reasonable, although perhaps unfortunate. I acked Tomasz's
> patch because I thought that XHCI is new, and thus it's no big deal
> if its results are different.
> 
> Of course I rushed to examine the packet structure, but yes, there's
> no space in the header, not without some trickery. For example, the
> upper 8 bits of the ID are probably the same for all packets, so
> it may be stuffed in there. Or, it might be possible to create an
> extra header and attach it at the end of ISO descriptors.
> 
> Alan's suggestion of leaving the physical address in /sys appeals
> the most to me, honestly. One thing though, libpcap will need to
> rifle through /sys and then store that address, so its serialization
> has to be changed no matter what. Unfortunately, I'm wholly ignorant
> as to what syntax it uses and how extensible it is.

Alternatively, libpcap can ignore the issue and just display the device
numbers, as it does now.  A separate program or the user could convert
the number to a physical address, if necessary, using the information 
in sysfs.

Alan Stern

Re: [PATCH] usbmon: Report device address assigned to USB device

[Pete Zaitcev]

On Mon, 13 Jan 2020 10:05:42 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> > Alan's suggestion of leaving the physical address in /sys appeals
> > the most to me, honestly. One thing though, libpcap will need to
> > rifle through /sys and then store that address, so its serialization
> > has to be changed no matter what. Unfortunately, I'm wholly ignorant
> > as to what syntax it uses and how extensible it is.  
> 
> Alternatively, libpcap can ignore the issue and just display the device
> numbers, as it does now.  A separate program or the user could convert
> the number to a physical address, if necessary, using the information 
> in sysfs.

I thoght that people sometimes saved the traces and replayed them
on other machines, which obviously have their own /sys or even run
Windows or iOS.

-- Pete