[PATCH] Re: [patch 2.5.31-bk5] uhci, misc

This patch has some small UHCI bugfixes

   - on submit error, frees memory and (!) returns error code
   - root hub should disconnect only once
   - pci pool code shouldn't be given GFP_DMA
   - uses del_timer_sync(), which behaves on SMP, not del_timer()

and cleanups:

   - use container_of
   - doesn't replicate so much hcd state
   - no such status -ECONNABORTED
   - uses bus_name in procfs, not "hc0", "hc1" etc

drivers/usb/host/uhci-hcd.c

@@ -146,7 +146,7 @@ static struct uhci_td *uhci_alloc_td(struct uhci_hcd *uhci, struct usb_device *d
 	dma_addr_t dma_handle;
 	struct uhci_td *td;
 
-	td = pci_pool_alloc(uhci->td_pool, GFP_DMA | GFP_ATOMIC, &dma_handle);
+	td = pci_pool_alloc(uhci->td_pool, GFP_ATOMIC, &dma_handle);
 	if (!td)
 		return NULL;
 
@@ -325,7 +325,7 @@ static struct uhci_qh *uhci_alloc_qh(struct uhci_hcd *uhci, struct usb_device *d
 	dma_addr_t dma_handle;
 	struct uhci_qh *qh;
 
-	qh = pci_pool_alloc(uhci->qh_pool, GFP_DMA | GFP_ATOMIC, &dma_handle);
+	qh = pci_pool_alloc(uhci->qh_pool, GFP_ATOMIC, &dma_handle);
 	if (!qh)
 		return NULL;
 
@@ -1498,6 +1498,10 @@ static int uhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, int mem_flags)
 
 	spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 
+	if (ret != -EINPROGRESS) {
+		uhci_destroy_urb_priv (uhci, urb);
+		return ret;
+	}
 	return 0;
 }
 
@@ -1806,8 +1810,7 @@ static void uhci_finish_urb(struct usb_hcd *hcd, struct urb *urb)
 
 	spin_lock_irqsave(&urb->lock, flags);
 
-	killed = (urb->status == -ENOENT || urb->status == -ECONNABORTED ||
-			urb->status == -ECONNRESET);
+ 	killed = (urb->status == -ENOENT || urb->status == -ECONNRESET);
 	resubmit_interrupt = (usb_pipetype(urb->pipe) == PIPE_INTERRUPT &&
 			urb->interval);
 
@@ -1828,9 +1831,7 @@ static void uhci_finish_urb(struct usb_hcd *hcd, struct urb *urb)
 	if (resubmit_interrupt)
 		/* Recheck the status. The completion handler may have */
 		/*  unlinked the resubmitting interrupt URB */
-		killed = (urb->status == -ENOENT ||
-			  urb->status == -ECONNABORTED ||
-			  urb->status == -ECONNRESET);
+		killed = (urb->status == -ENOENT || urb->status == -ECONNRESET);
 
 	if (resubmit_interrupt && !killed) {
 		urb->dev = dev;
@@ -2027,19 +2028,12 @@ static void start_hc(struct uhci_hcd *uhci)
         uhci->hcd.state = USB_STATE_READY;
 }
 
-#ifdef CONFIG_PROC_FS
-static int uhci_num = 0;
-#endif
-
 /*
  * De-allocate all resources..
  */
 static void release_uhci(struct uhci_hcd *uhci)
 {
 	int i;
-#ifdef CONFIG_PROC_FS
-	char buf[8];
-#endif
 
 	for (i = 0; i < UHCI_NUM_SKELQH; i++)
 		if (uhci->skelqh[i]) {
@@ -2064,15 +2058,13 @@ static void release_uhci(struct uhci_hcd *uhci)
 	}
 
 	if (uhci->fl) {
-		pci_free_consistent(uhci->dev, sizeof(*uhci->fl), uhci->fl, uhci->fl->dma_handle);
+		pci_free_consistent(uhci->hcd.pdev, sizeof(*uhci->fl), uhci->fl, uhci->fl->dma_handle);
 		uhci->fl = NULL;
 	}
 
 #ifdef CONFIG_PROC_FS
 	if (uhci->proc_entry) {
-		sprintf(buf, "hc%d", uhci->num);
-
-		remove_proc_entry(buf, uhci_proc_root);
+		remove_proc_entry(uhci->hcd.self.bus_name, uhci_proc_root);
 		uhci->proc_entry = NULL;
 	}
 #endif
@@ -2097,33 +2089,20 @@ static void release_uhci(struct uhci_hcd *uhci)
 static int __devinit uhci_start(struct usb_hcd *hcd)
 {
 	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
-	struct pci_dev *dev = hcd->pdev;
 	int retval = -EBUSY;
 	int i, port;
+	unsigned io_size;
 	dma_addr_t dma_handle;
+	struct usb_device *udev;
 #ifdef CONFIG_PROC_FS
-	char buf[8];
 	struct proc_dir_entry *ent;
 #endif
 
-	uhci->dev = dev;
-
-	/* Should probably move to core/hcd.c */
-	if (pci_set_dma_mask(dev, 0xFFFFFFFF)) {
-		err("couldn't set PCI dma mask");
-		retval = -ENODEV;
-		goto err_pci_set_dma_mask;
-	}
-
-	uhci->io_addr = pci_resource_start(dev, hcd->region);
-	uhci->io_size = pci_resource_len(dev, hcd->region);
+	uhci->io_addr = (unsigned) hcd->regs;
+	io_size = pci_resource_len(hcd->pdev, hcd->region);
 
 #ifdef CONFIG_PROC_FS
-	uhci->num = uhci_num++;
-
-	sprintf(buf, "hc%d", uhci->num);
-
-	ent = create_proc_entry(buf, S_IFREG|S_IRUGO|S_IWUSR, uhci_proc_root);
+	ent = create_proc_entry(hcd->self.bus_name, S_IFREG|S_IRUGO|S_IWUSR, uhci_proc_root);
 	if (!ent) {
 		err("couldn't create uhci proc entry");
 		retval = -ENOMEM;
@@ -2159,7 +2138,7 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 
 	spin_lock_init(&uhci->frame_list_lock);
 
-	uhci->fl = pci_alloc_consistent(dev, sizeof(*uhci->fl), &dma_handle);
+	uhci->fl = pci_alloc_consistent(hcd->pdev, sizeof(*uhci->fl), &dma_handle);
 	if (!uhci->fl) {
 		err("unable to allocate consistent memory for frame list");
 		goto err_alloc_fl;
@@ -2169,15 +2148,15 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 
 	uhci->fl->dma_handle = dma_handle;
 
-	uhci->td_pool = pci_pool_create("uhci_td", dev,
-		sizeof(struct uhci_td), 16, 0, GFP_DMA | GFP_ATOMIC);
+	uhci->td_pool = pci_pool_create("uhci_td", hcd->pdev,
+		sizeof(struct uhci_td), 16, 0, GFP_ATOMIC);
 	if (!uhci->td_pool) {
 		err("unable to create td pci_pool");
 		goto err_create_td_pool;
 	}
 
-	uhci->qh_pool = pci_pool_create("uhci_qh", dev,
-		sizeof(struct uhci_qh), 16, 0, GFP_DMA | GFP_ATOMIC);
+	uhci->qh_pool = pci_pool_create("uhci_qh", hcd->pdev,
+		sizeof(struct uhci_qh), 16, 0, GFP_ATOMIC);
 	if (!uhci->qh_pool) {
 		err("unable to create qh pci_pool");
 		goto err_create_qh_pool;
@@ -2189,7 +2168,7 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 	/*  they may have more but give no way to determine how many they */
 	/*  have. However, according to the UHCI spec, Bit 7 is always set */
 	/*  to 1. So we try to use this to our advantage */
-	for (port = 0; port < (uhci->io_size - 0x10) / 2; port++) {
+	for (port = 0; port < (io_size - 0x10) / 2; port++) {
 		unsigned int portstatus;
 
 		portstatus = inw(uhci->io_addr + 0x10 + (port * 2));
@@ -2208,13 +2187,13 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 
 	uhci->rh_numports = port;
 
-	hcd->self.root_hub = uhci->rh_dev = usb_alloc_dev(NULL, &hcd->self);
-	if (!uhci->rh_dev) {
+	hcd->self.root_hub = udev = usb_alloc_dev(NULL, &hcd->self);
+	if (!udev) {
 		err("unable to allocate root hub");
 		goto err_alloc_root_hub;
 	}
 
-	uhci->skeltd[0] = uhci_alloc_td(uhci, uhci->rh_dev);
+	uhci->skeltd[0] = uhci_alloc_td(uhci, udev);
 	if (!uhci->skeltd[0]) {
 		err("unable to allocate TD 0");
 		goto err_alloc_skeltd;
@@ -2227,7 +2206,7 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 	for (i = 1; i < 9; i++) {
 		struct uhci_td *td;
 
-		td = uhci->skeltd[i] = uhci_alloc_td(uhci, uhci->rh_dev);
+		td = uhci->skeltd[i] = uhci_alloc_td(uhci, udev);
 		if (!td) {
 			err("unable to allocate TD %d", i);
 			goto err_alloc_skeltd;
@@ -2238,14 +2217,14 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 		td->link = cpu_to_le32(uhci->skeltd[i - 1]->dma_handle);
 	}
 
-	uhci->skel_term_td = uhci_alloc_td(uhci, uhci->rh_dev);
+	uhci->skel_term_td = uhci_alloc_td(uhci, udev);
 	if (!uhci->skel_term_td) {
 		err("unable to allocate skel TD term");
 		goto err_alloc_skeltd;
 	}
 
 	for (i = 0; i < UHCI_NUM_SKELQH; i++) {
-		uhci->skelqh[i] = uhci_alloc_qh(uhci, uhci->rh_dev);
+		uhci->skelqh[i] = uhci_alloc_qh(uhci, udev);
 		if (!uhci->skelqh[i]) {
 			err("unable to allocate QH %d", i);
 			goto err_alloc_skelqh;
@@ -2314,12 +2293,12 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 	init_stall_timer(hcd);
 
 	/* disable legacy emulation */
-	pci_write_config_word(dev, USBLEGSUP, USBLEGSUP_DEFAULT);
+	pci_write_config_word(hcd->pdev, USBLEGSUP, USBLEGSUP_DEFAULT);
 
-	usb_connect(uhci->rh_dev);
-        uhci->rh_dev->speed = USB_SPEED_FULL;
+	usb_connect(udev);
+	udev->speed = USB_SPEED_FULL;
 
-	if (usb_register_root_hub(uhci->rh_dev, &dev->dev) != 0) {
+	if (usb_register_root_hub(udev, &hcd->pdev->dev) != 0) {
 		err("unable to start root hub");
 		retval = -ENOMEM;
 		goto err_start_root_hub;
@@ -2333,7 +2312,7 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 err_start_root_hub:
 	reset_hc(uhci);
 
-	del_timer(&uhci->stall_timer);
+	del_timer_sync(&uhci->stall_timer);
 
 	for (i = 0; i < UHCI_NUM_SKELQH; i++)
 		if (uhci->skelqh[i]) {
@@ -2349,8 +2328,7 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 		}
 
 err_alloc_skeltd:
-	usb_free_dev(uhci->rh_dev);
-	uhci->rh_dev = NULL;
+	usb_free_dev(udev);
 
 err_alloc_root_hub:
 	pci_pool_destroy(uhci->qh_pool);
@@ -2361,19 +2339,17 @@ static int __devinit uhci_start(struct usb_hcd *hcd)
 	uhci->td_pool = NULL;
 
 err_create_td_pool:
-	pci_free_consistent(dev, sizeof(*uhci->fl), uhci->fl, uhci->fl->dma_handle);
+	pci_free_consistent(hcd->pdev, sizeof(*uhci->fl), uhci->fl, uhci->fl->dma_handle);
 	uhci->fl = NULL;
 
 err_alloc_fl:
 #ifdef CONFIG_PROC_FS
-	remove_proc_entry(buf, uhci_proc_root);
+	remove_proc_entry(hcd->self.bus_name, uhci_proc_root);
 	uhci->proc_entry = NULL;
 
 err_create_proc_entry:
 #endif
 
-err_pci_set_dma_mask:
-
 	return retval;
 }
 
@@ -2381,10 +2357,7 @@ static void uhci_stop(struct usb_hcd *hcd)
 {
 	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
 
-	if (uhci->rh_dev)
-		usb_disconnect(&uhci->rh_dev);
-
-	del_timer(&uhci->stall_timer);
+	del_timer_sync(&uhci->stall_timer);
 
 	/*
 	 * At this point, we're guaranteed that no new connects can be made
@@ -2413,7 +2386,7 @@ static int uhci_resume(struct usb_hcd *hcd)
 {
 	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
 
-	pci_set_master(uhci->dev);
+	pci_set_master(uhci->hcd.pdev);
 
 	reset_hc(uhci);
 	start_hc(uhci);

drivers/usb/host/uhci-hcd.h

@@ -308,7 +308,7 @@ static inline int __interval_to_skel(int interval)
 	return 7;				/* int128 for 128-255 ms (Max.) */
 }
 
-#define hcd_to_uhci(hcd_ptr) list_entry(hcd_ptr, struct uhci_hcd, hcd)
+#define hcd_to_uhci(hcd_ptr) container_of(hcd_ptr, struct uhci_hcd, hcd)
 
 /*
  * This describes the full uhci information.
@@ -319,18 +319,13 @@ static inline int __interval_to_skel(int interval)
 struct uhci_hcd {
 	struct usb_hcd hcd;
 
-	struct pci_dev *dev;
-
 #ifdef CONFIG_PROC_FS
 	/* procfs */
-	int num;
 	struct proc_dir_entry *proc_entry;
 #endif
 
 	/* Grabbed from PCI */
-	int irq;
 	unsigned int io_addr;
-	unsigned int io_size;
 
 	struct pci_pool *qh_pool;
 	struct pci_pool *td_pool;
@@ -362,7 +357,6 @@ struct uhci_hcd {
 	spinlock_t complete_list_lock;
 	struct list_head complete_list;		/* P: uhci->complete_list_lock */
 
-	struct usb_device *rh_dev;		/* Root hub */
 	int rh_numports;
 
 	struct timer_list stall_timer;