clear_halt_work

Roman Kagan rkagan at mail.ru
Fri May 6 16:33:56 EDT 2005


On Fri, May 06, 2005 at 11:20:14AM +0200, Duncan Sands wrote:
> > As per David Brownell's comment, usb core only uses urb_list field in
> > struct urb when it owns that urb, that is, since the call to
> > usb_submit_urb() till the entry into the completion routine.  Outside of
> > this scope, drivers are free to use it for whatever they want.  Thus we
> > can get rid of struct usbatm_transceiver, and use struct urb directly
> > instead.
> 
> let's wait until your patch moving the list_head to the public part is
> accepted.

Suddenly, it already is.  I've received a confirmation message from
Greg.

> > The following patch does this.   In addition, I've simplified a bit the
> > allocation of the array of urbs (formerly transceivers), merging it into
> > the allocation of struct usbatm_data. [...]
> 
> It can go in later.

Well, it's practically zero risk, but it improves readability and saves
a number of dereferences...  I'm attaching it rediffed against the
current CVS, in case you change your mind :)

Cheers,
  Roman.

 usbatm.h |   10 ----
 usbatm.c |  154 ++++++++++++++++++++++++++++-----------------------------------
 2 files changed, 72 insertions(+), 92 deletions(-)


Index: usbatm.h
===================================================================
RCS file: /home/cvs/usbatm/usbatm.h,v
retrieving revision 1.17
diff -u -p -r1.17 usbatm.h
--- usbatm.h	6 May 2005 09:09:01 -0000	1.17
+++ usbatm.h	6 May 2005 20:19:29 -0000
@@ -136,12 +136,6 @@ struct usbatm_channel {
 	struct usbatm_data *usbatm;
 };
 
-struct usbatm_transceiver {
-	struct list_head list;
-	struct urb *urb;
-	struct usbatm_channel *channel;
-};
-
 /* main driver data */
 
 struct usbatm_data {
@@ -177,13 +171,13 @@ struct usbatm_data {
 	/* ATM device */
 	struct list_head vcc_list;
 
-	struct usbatm_transceiver *transceivers;
-
 	struct usbatm_channel rx_channel;
 	struct usbatm_channel tx_channel;
 
 	struct sk_buff_head sndqueue;
 	struct sk_buff *current_skb;			/* being emptied */
+
+	struct urb *urbs[0];
 };
 
 #endif	/* _USBATM_H_ */
Index: usbatm.c
===================================================================
RCS file: /home/cvs/usbatm/usbatm.c,v
retrieving revision 1.41
diff -u -p -r1.41 usbatm.c
--- usbatm.c	6 May 2005 09:20:12 -0000	1.41
+++ usbatm.c	6 May 2005 20:19:29 -0000
@@ -203,13 +203,13 @@ static inline void usbatm_pop(struct atm
 }
 
 
-/*******************
-**  transceivers  **
-*******************/
+/***********
+**  urbs  **
+************/
 
-static inline struct usbatm_transceiver *usbatm_pop_transceiver(struct usbatm_channel *channel)
+static inline struct urb *usbatm_pop_urb(struct usbatm_channel *channel)
 {
-	struct usbatm_transceiver *trx;
+	struct urb *urb;
 
 	spin_lock_irq(&channel->lock);
 	if (list_empty(&channel->list)) {
@@ -217,32 +217,32 @@ static inline struct usbatm_transceiver 
 		return NULL;
 	}
 	
-	trx = list_entry(channel->list.next, struct usbatm_transceiver, list);
-	list_del(&trx->list);
+	urb = list_entry(channel->list.next, struct urb, urb_list);
+	list_del(&urb->urb_list);
 	spin_unlock_irq(&channel->lock);
 
-	return trx;
+	return urb;
 }
 
-static inline int usbatm_submit_transceiver(struct usbatm_transceiver *trx)
+static inline int usbatm_submit_urb(struct urb *urb)
 {
-	struct usbatm_channel *channel = trx->channel;
+	struct usbatm_channel *channel = urb->context;
 	int ret;
 
-	vdbg("%s: submitting urb 0x%p, trx 0x%p, size %u",
-	     __func__, trx->urb, trx, trx->urb->transfer_buffer_length);
+	vdbg("%s: submitting urb 0x%p, size %u",
+	     __func__, urb, urb->transfer_buffer_length);
 
-	ret = usb_submit_urb(trx->urb, GFP_ATOMIC);
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
 	if (ret) {
-		atm_dbg(channel->usbatm, "%s: trx 0x%p urb 0x%p submission failed (%d)!\n",
-			__func__, trx, trx->urb, ret);
+		atm_dbg(channel->usbatm, "%s: urb 0x%p submission failed (%d)!\n",
+			__func__, urb, ret);
 
 		/* consider all errors transient and return the buffer back to the queue */
-		trx->urb->status = -EAGAIN;
+		urb->status = -EAGAIN;
 		spin_lock_irq(&channel->lock);
 
 		/* must add to the front when sending; doesn't matter when receiving */
-		list_add(&trx->list, &channel->list);
+		list_add(&urb->urb_list, &channel->list);
 
 		spin_unlock_irq(&channel->lock);
 
@@ -255,18 +255,17 @@ static inline int usbatm_submit_transcei
 
 static void usbatm_complete(struct urb *urb, struct pt_regs *regs)
 {
-	struct usbatm_transceiver *trx = urb->context;
-	struct usbatm_channel *channel = trx->channel;
+	struct usbatm_channel *channel = urb->context;
 	unsigned long flags;
 
-	vdbg("%s: urb 0x%p, status %d, actual_length %d, trx 0x%p",
-	     __func__, urb, urb->status, urb->actual_length, trx);
+	vdbg("%s: urb 0x%p, status %d, actual_length %d",
+	     __func__, urb, urb->status, urb->actual_length);
 
 	/* usually in_interrupt(), but not always */
 	spin_lock_irqsave(&channel->lock, flags);
 
 	/* must add to the back when receiving; doesn't matter when sending */
-	list_add_tail(&trx->list, &channel->list);
+	list_add_tail(&urb->urb_list, &channel->list);
 
 	spin_unlock_irqrestore(&channel->lock, flags);
 
@@ -481,11 +480,9 @@ static unsigned int usbatm_write_cells(s
 static void usbatm_rx_process(unsigned long data)
 {
 	struct usbatm_data *instance = (struct usbatm_data *)data;
-	struct usbatm_transceiver *rx;
-
-	while ((rx = usbatm_pop_transceiver(&instance->rx_channel))) {
-		struct urb *urb = rx->urb;
+	struct urb *urb;
 
+	while ((urb = usbatm_pop_urb(&instance->rx_channel))) {
 		vdbg("%s: processing rx 0x%p", __func__, rx);
 
 		if (usb_pipeisoc(urb->pipe)) {
@@ -500,7 +497,7 @@ static void usbatm_rx_process(unsigned l
 			if (!urb->status)
 				usbatm_extract_cells(instance, urb->transfer_buffer, urb->actual_length);
 
-		if (usbatm_submit_transceiver(rx))
+		if (usbatm_submit_urb(urb))
 			return;
 	}
 }
@@ -514,7 +511,7 @@ static void usbatm_tx_process(unsigned l
 {
 	struct usbatm_data *instance = (struct usbatm_data *)data;
 	struct sk_buff *skb = instance->current_skb;
-	struct usbatm_transceiver *tx = NULL;
+	struct urb *urb = NULL;
 	const unsigned int buf_size = instance->tx_channel.buf_size;
 	unsigned int num_written = 0;
 	u8 *buffer = NULL;
@@ -523,21 +520,21 @@ static void usbatm_tx_process(unsigned l
 		skb = skb_dequeue(&instance->sndqueue);
 
 	while (skb) {
-		if (!tx) {
-			tx = usbatm_pop_transceiver(&instance->tx_channel);
-			if (!tx)
+		if (!urb) {
+			urb = usbatm_pop_urb(&instance->tx_channel);
+			if (!urb)
 				break;		/* no more senders */
-			buffer = tx->urb->transfer_buffer;
-			num_written = (tx->urb->status == -EAGAIN) ?
-				tx->urb->transfer_buffer_length : 0;
+			buffer = urb->transfer_buffer;
+			num_written = (urb->status == -EAGAIN) ?
+				urb->transfer_buffer_length : 0;
 		}
 
 		num_written += usbatm_write_cells(instance, skb,
 						  buffer + num_written,
 						  buf_size - num_written);
 
-		vdbg("%s: wrote %u bytes from skb 0x%p to sender 0x%p",
-		     __func__, num_written, skb, tx);
+		vdbg("%s: wrote %u bytes from skb 0x%p to urb 0x%p",
+		     __func__, num_written, skb, urb);
 
 		if (!UDSL_SKB(skb)->len) {
 			struct atm_vcc *vcc = UDSL_SKB(skb)->atm.vcc;
@@ -549,13 +546,12 @@ static void usbatm_tx_process(unsigned l
 		}
 
 		if (num_written == buf_size || (!skb && num_written)) {
-			tx->urb->transfer_buffer_length = num_written;
+			urb->transfer_buffer_length = num_written;
 
-			if (usbatm_submit_transceiver(tx))
+			if (usbatm_submit_urb(urb))
 				break;
-			tx = NULL;
+			urb = NULL;
 		}
-
 	}
 
 	instance->current_skb = skb;
@@ -876,7 +872,7 @@ static int usbatm_atm_init(struct usbatm
 
 	/* submit all rx URBs */
 	for (i = 0; i < num_rcv_urbs; i++)
-		usbatm_submit_transceiver(instance->transceivers + i);
+		usbatm_submit_urb(instance->urbs[i]);
 
 	return 0;
 
@@ -963,7 +959,9 @@ int usbatm_usb_probe(struct usb_interfac
 			intf->altsetting->desc.bInterfaceNumber);
 
 	/* instance init */
-	if (!(instance = kmalloc(sizeof(*instance), GFP_KERNEL))) {
+	instance = kmalloc(sizeof(*instance) + sizeof(struct urb *) * (num_rcv_urbs + num_snd_urbs),
+			   GFP_KERNEL);
+	if (!instance) {
 		dev_dbg(dev, "%s: no memory for instance data!\n", __func__);
 		return -ENOMEM;
 	}
@@ -1031,63 +1029,55 @@ int usbatm_usb_probe(struct usb_interfac
 
 	skb_queue_head_init(&instance->sndqueue);
 
-	instance->transceivers = kmalloc(sizeof(*instance->transceivers) * (num_rcv_urbs + num_snd_urbs),
-					 GFP_KERNEL);
-	if (!instance->transceivers) {
-		dev_dbg(dev, "%s: no memory for transceivers!\n", __func__);
-		goto fail_unbind;
-	}
-
-	memset(instance->transceivers, 0, sizeof(*instance->transceivers) * (num_rcv_urbs + num_snd_urbs));
-
 	for (i = 0; i < num_rcv_urbs + num_snd_urbs; i++) {
-		struct usbatm_transceiver *trx = instance->transceivers + i;
+		struct urb *urb;
 		u8 *buffer;
 		unsigned int iso_packets = 0, iso_size = 0;
+		struct usbatm_channel *channel = i < num_rcv_urbs ?
+			&instance->rx_channel : &instance->tx_channel;
 
-		trx->channel = i < num_rcv_urbs ? &instance->rx_channel : &instance->tx_channel;
-
-		buffer = kmalloc(trx->channel->buf_size, GFP_KERNEL);
+		buffer = kmalloc(channel->buf_size, GFP_KERNEL);
 		if (!buffer) {
 			dev_dbg(dev, "%s: no memory for buffer %d!\n", __func__, i);
 			goto fail_unbind;
 		}
-		memset(buffer, 0, trx->channel->buf_size);
+		memset(buffer, 0, channel->buf_size);
 
-		if (usb_pipeisoc(trx->channel->endpoint)) {
+		if (usb_pipeisoc(channel->endpoint)) {
 			/* don't expect iso out endpoints */
-			iso_size = usb_maxpacket(instance->usb_dev, trx->channel->endpoint, 0);
-			iso_size -= iso_size % trx->channel->stride;	/* alignment */
+			iso_size = usb_maxpacket(instance->usb_dev, channel->endpoint, 0);
+			iso_size -= iso_size % channel->stride;	/* alignment */
 			BUG_ON(!iso_size);
-			iso_packets = (trx->channel->buf_size - 1) / iso_size + 1;
+			iso_packets = (channel->buf_size - 1) / iso_size + 1;
 		}
 
-		trx->urb = usb_alloc_urb(iso_packets, GFP_KERNEL);
-		if (!trx->urb) {
+		urb = usb_alloc_urb(iso_packets, GFP_KERNEL);
+		if (!urb) {
 			dev_dbg(dev, "%s: no memory for urb %d!\n", __func__, i);
 			goto fail_unbind;
 		}
 
-		usb_fill_bulk_urb(trx->urb, instance->usb_dev, trx->channel->endpoint,
-				  buffer, trx->channel->buf_size, usbatm_complete, trx);
+		usb_fill_bulk_urb(urb, instance->usb_dev, channel->endpoint,
+				  buffer, channel->buf_size, usbatm_complete, channel);
 		if (iso_packets) {
 			int j;
-			trx->urb->interval = 1;
-			trx->urb->transfer_flags = URB_ISO_ASAP;
-			trx->urb->number_of_packets = iso_packets;
+			urb->interval = 1;
+			urb->transfer_flags = URB_ISO_ASAP;
+			urb->number_of_packets = iso_packets;
 			for (j = 0; j < iso_packets; j++) {
-				trx->urb->iso_frame_desc[j].offset = iso_size * j;
-				trx->urb->iso_frame_desc[j].length = min_t(int, iso_size,
-									   trx->channel->buf_size - trx->urb->iso_frame_desc[j].offset);
+				urb->iso_frame_desc[j].offset = iso_size * j;
+				urb->iso_frame_desc[j].length = min_t(int, iso_size,
+								      channel->buf_size - urb->iso_frame_desc[j].offset);
 			}
 		}
 
 		/* put all tx URBs on the list of spares */
 		if (i >= num_rcv_urbs)
-			list_add_tail(&trx->list, &trx->channel->list);
+			list_add_tail(&urb->urb_list, &channel->list);
 
-		vdbg("%s: alloced trx 0x%p buffer 0x%p buf size %u urb 0x%p",
-		     __func__, trx, trx->urb->transfer_buffer, trx->urb->transfer_buffer_length, trx->urb);
+		vdbg("%s: alloced buffer 0x%p buf size %u urb 0x%p",
+		     __func__, urb->transfer_buffer, urb->transfer_buffer_length, urb);
+		instance->urbs[i] = urb;
 	}
 
 	if (need_heavy && driver->heavy_init) {
@@ -1109,13 +1099,11 @@ int usbatm_usb_probe(struct usb_interfac
 	if (instance->driver->unbind)
 		instance->driver->unbind(instance, intf);
  fail_free:
-	if (instance->transceivers)
-		for (i = 0; i < num_rcv_urbs + num_snd_urbs; i++) {
-			kfree(instance->transceivers[i].urb->transfer_buffer);
-			usb_free_urb(instance->transceivers[i].urb);
-		}
+	for (i = 0; i < num_rcv_urbs + num_snd_urbs; i++) {
+		kfree(instance->urbs[i]->transfer_buffer);
+		usb_free_urb(instance->urbs[i]);
+	}
 
-	kfree(instance->transceivers);
 	kfree (instance);
 
 	return error;
@@ -1148,7 +1136,7 @@ void usbatm_usb_disconnect(struct usb_in
 	tasklet_disable(&instance->tx_channel.tasklet);
 
 	for (i = 0; i < num_rcv_urbs + num_snd_urbs; i++)
-		usb_kill_urb(instance->transceivers[i].urb);
+		usb_kill_urb(instance->urbs[i]);
 
 	del_timer_sync(&instance->rx_channel.delay);
 	del_timer_sync(&instance->tx_channel.delay);
@@ -1170,12 +1158,10 @@ void usbatm_usb_disconnect(struct usb_in
 	tasklet_enable(&instance->tx_channel.tasklet);
 
 	for (i = 0; i < num_rcv_urbs + num_snd_urbs; i++) {
-		kfree(instance->transceivers[i].urb->transfer_buffer);
-		usb_free_urb(instance->transceivers[i].urb);
+		kfree(instance->urbs[i]->transfer_buffer);
+		usb_free_urb(instance->urbs[i]);
 	}
 
-	kfree(instance->transceivers);
-
 	/* ATM finalize */
 	if (instance->atm_dev)
 		shutdown_atm_dev(instance->atm_dev);



More information about the Usbatm mailing list