On Sat, Apr 21, 2012 at 5:40 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> 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.
>
Yes, this is a problem should be avoided.
> [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
