I'm cc-ing this back to the list, in case Greg or others want to comment. Let's keep this open from now on.
On Wed, 25 Oct 2006 10:31:57 +0200, Paolo Abeni <[EMAIL PROTECTED]> wrote: > On Tue, 2006-10-24 at 22:48 -0700, Pete Zaitcev wrote: > > By the way, there is no patch yet, but I blogged a small update: > > http://zaitcev.livejournal.com/97533.html > > I worked out a third version of my patch. Maybe you can find it useful. > It use char device with dynamically allocated major number. A char > device is created for each detected usb bus. URB are ref-counted and > queued on a per-file list. URB can be read only with ioctl operation. I was enthusiastic about this, it looks great. But on second thought, we can't do this: > +static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb, > + char ev_type) > +{ > + usb_get_urb(urb); > + ep->urb = urb; > ......... > + list_add_tail(&ep->e_link, &rp->e_list); > + wake_up(&rp->wait); > +} In the USB stack, URBs are often reused. So, if a driver allocates an URB, allocates a buffer, and repeatably resubmits it, it is going to overwrite the data before Wireshark had a chance to run and fetch it. Actually, overwriting parts of URB itself is legal too. So, I think we should do it my way, at least for now. This is unfortunate because keeping references would be more efficient, and I really do not like huge copies under spinlocks, but there seems to be no other way. Please let me know if I'm wrong here. The rest looks good. I saw a couple of very small warts, like this: > +static int mon_bin_ioctl(struct inode *inode, struct file *file, > + unsigned int cmd, unsigned long arg) > +{ > + unsigned flags; > + case MON_IOCQ_URB_LEN: > + spin_lock_irqsave(&mbus->lock, flags); (must be a long, or corruption happens) > @@ -401,7 +408,8 @@ static void __exit mon_exit(void) > kref_put(&mbus->ref, mon_bus_drop); > } > mutex_unlock(&mon_lock); > - > + > + mon_bin_exit(); (careless whitespace) Oh, and another thing... I thought a bit about the event structure as seen by userspace, and I laid it out a little bit differently. Here's what we have now: > +struct mon_bin_hdr { > + s32 type; /* submit, complete, etc. */ > + u32 pipe; /* Pipe */ > + struct timeval tstamp; > + s32 status; > + u32 caplen; /* length of data delivered to the application */ > + u32 len; /* the actual length of URB data */ > + u32 flags; > +}; This layout still varies between CPU modes, because timeval contains longs. I thought it would cost us nothing to make a struct of a completely fixed size. Secondly, a pipe is out fashion nowadays. And finally, I'd like to deliver the bus number. Currently it makes no sense, but I do not want to close the door on "bus zero" which delivers events from all buses. This may be useful when HCs are hotplugged. +struct mon_packet { + u64 id; /* URB ID - from submission to callback */ + unsigned char type; /* Same as in text API; extensible. */ + unsigned char xfer_type; /* ISO, Intr, Control, Bulk; 0x80 IN */ + unsigned char epnum; /* Endpoint number */ + unsigned char devnum; /* Device address */ + unsigned short busnum; /* Bus number */ + unsigned short _pad; + s64 ts_sec; /* gettimeofday */ + s32 ts_usec; /* gettimeofday */ + int status; + unsigned int length; /* Length of data (submitted or actual) */ + unsigned int len_cap; /* Delivered length */ + unsigned char setup[SETUP_LEN]; /* Only for Control S-type */ +}; This should be sufficiently future-proof. Greetings, -- Pete ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
