Hi Huajun,
On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote:
> On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
>> Hi Huajun,
>>
>> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote:
>>
>>> Did we on the same page, could you please review my patch again?
>>>
>>> My draft patch was based on current mainline( 3.4.0-rc3) which had
>>> already integrated your previous patch. And in my patch, it replaced
>>> skb_queue_walk_safe() with skb_queue_walk(), so you will not see 'tmp
>>> = skb->next' any more.
>>
>> Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
>> the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
>> the oops, does it?
>>
>
> No.
> In each loop, my patch traverse the queue from its head, and it always
> holds q->lock when it need refer "skb->next", this can make sure the
> right skb is not moved out of rxq/txq.
OK, your patch can avoid the oops, sorry for miss the point.
>
> Can this fix what you concern? If so, IMO, there is no need to revert
> your previous patch.
But your patch may introduce another problem, in fact, what your patch does
is basically same with the below change[1]:
So we can find easily that one same URB may be unlinked more than one
time with your patch because usb_unlink_urb is asynchronous even though
it behaves synchronously sometimes.
I remembered that is not allowed, at least usb_unlink_urb's comment says so:
URBs complete only once per submission, and may be canceled only
once per submission.
[1], against 3.4.0-rc3
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..aadf009 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -578,15 +578,19 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
{
unsigned long flags;
- struct sk_buff *skb, *skbnext;
+ struct sk_buff *skb;
int count = 0;
spin_lock_irqsave (&q->lock, flags);
- skb_queue_walk_safe(q, skb, skbnext) {
+ while (1) {
struct skb_data *entry;
struct urb *urb;
int retval;
+ skb = q->next;
+ if (skb == (struct sk_buff *)q)
+ break;
+
entry = (struct skb_data *) skb->cb;
urb = entry->urb;
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
