On Mon, Jan 31, 2005 at 09:29:03PM -0800, Pete Zaitcev wrote:
> This patch adds so-called "usbmon", or USB monitoring framework, similar
> to what tcpdump provides for Ethernet. This is an initial version, but
> it should be safe and useful. It adds an overhead of an if () statement
> into submission and giveback paths even when not monitoring, but this
> was deemed a lesser evil than stealth manipulation of function pointers.
Few minor comments on the code:
First off, why make usbmon a module? You aren't allowing it to happen,
so just take out the parts of the patch that allow it.
>
> /* host controllers we manage */
> LIST_HEAD (usb_bus_list);
> +EXPORT_SYMBOL_GPL (usb_bus_list);
Not needed if not a module.
> /* used when allocating bus numbers */
> #define USB_MAXBUS 64
> @@ -96,6 +97,7 @@ static struct usb_busmap busmap;
>
> /* used when updating list of hcds */
> DECLARE_MUTEX (usb_bus_list_lock); /* exported only for usbfs */
> +EXPORT_SYMBOL_GPL (usb_bus_list_lock);
Not needed if not a module.
>
> /* used when updating hcd data */
> static DEFINE_SPINLOCK(hcd_data_lock);
> @@ -103,6 +105,10 @@ static DEFINE_SPINLOCK(hcd_data_lock);
> /* wait queue for synchronous unlinks */
> DECLARE_WAIT_QUEUE_HEAD(usb_kill_urb_queue);
>
> +#if defined(CONFIG_USB_MON) || defined(CONFIG_USB_MON_MODULE)
> +struct usb_mon_operations *mon_ops;
> +#endif /* CONFIG_USB_MON */
Not needed at all, as you create it down below. Ah, the .h files, no
wait, you refer to it there too, so removing it here should be fine.
> @@ -1103,8 +1110,6 @@ static int hcd_submit_urb (struct urb *u
> * valid and usb_buffer_{sync,unmap}() not be needed, since
> * they could clobber root hub response data.
> */
> - urb->transfer_flags |= (URB_NO_TRANSFER_DMA_MAP
> - | URB_NO_SETUP_DMA_MAP);
> status = rh_urb_enqueue (hcd, urb);
> goto done;
> }
Why remove that statment? What does that have to do with usbmon?
> void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct
> pt_regs *regs)
> {
> - urb_unlink (urb);
> + int at_root_hub;
>
> - // NOTE: a generic device/urb monitoring hook would go here.
> - // hcd_monitor_hook(MONITOR_URB_FINISH, urb, dev)
> - // It would catch exit/unlink paths for all urbs.
> + at_root_hub = (urb->dev == hcd->self.root_hub);
> + urb_unlink (urb);
>
> /* lower level hcd code should use *_dma exclusively */
> - if (hcd->self.controller->dma_mask) {
> + if (hcd->self.controller->dma_mask && !at_root_hub) {
> if (usb_pipecontrol (urb->pipe)
> && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
> dma_unmap_single (hcd->self.controller, urb->setup_dma,
You change the logic here a bit too. Why?
> diff -urpN -X dontdiff linux-2.6.11-rc2/drivers/usb/core/hcd.h
> linux-2.6.11-rc2-lem/drivers/usb/core/hcd.h
> --- linux-2.6.11-rc2/drivers/usb/core/hcd.h 2005-01-22 14:54:24.000000000
> -0800
> +++ linux-2.6.11-rc2-lem/drivers/usb/core/hcd.h 2005-01-23
> 17:19:43.000000000 -0800
> @@ -411,6 +411,63 @@ static inline void usbfs_cleanup(void) {
>
> /*-------------------------------------------------------------------------*/
>
> +#if defined(CONFIG_USB_MON) || defined(CONFIG_USB_MON_MODULE)
> +
> +struct usb_mon_operations {
> + void (*urb_submit)(struct usb_bus *bus, struct urb *urb);
> + void (*urb_submit_error)(struct usb_bus *bus, struct urb *urb, int err);
> + void (*urb_complete)(struct usb_bus *bus, struct urb *urb);
> + /* void (*urb_unlink)(struct usb_bus *bus, struct urb *urb); */
> + void (*bus_add)(struct usb_bus *bus);
> + void (*bus_remove)(struct usb_bus *bus);
> +};
> +
> +extern struct usb_mon_operations *mon_ops;
> +
> +#define usbmon_urb_submit(bus, urb) \
Can you make these inlines? That makes the code nicer as we get
typechecking and everything :)
> +#else
> +
> +#define usbmon_urb_submit(bus, urb) /* */
> +#define usbmon_urb_submit(bus, urb, error) /* */
> +#define usbmon_urb_complete(bus, urb) /* */
> +#define usbmon_notify_bus_add(bus) /* */
> +#define usbmon_notify_bus_remove(bus) /* */
static inlines for these too if you can.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
