On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote: > Hi Huajun, > > On Sat, Apr 21, 2012 at 2:39 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote: > >> Hi Ming, >> That's why my patch uses skb_queue_walk() to traverse the queue, >> it guarantees the skb available in each loop. Is this what you >> expected? >> >> The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is: >> 1. If the skb in txq/rxq, then it must be available, >> unlink_urbs() can refer to it safely while it holds q->lock; >> 2. If the skb in txq/rxq and its state is >> rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then >> don't need to unlink it again; > > As I said before, at least current code is not mistaken, since unlink > can handle the case correctly. > >> 3. Before releasing q->lock in unlink_urbs(), it will increase >> the URB's refercount, so even the related skb is freed in future, the >> URB is still available. > > No, increasing URB's reference count does not prevent the referenced skb > from being freed, see usbnet_bh(), so 'tmp = skb->next' in skb_queue_walk_safe > still may reference a freed pointer. > 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. >>> As far as I can think of, we can hold lock of done queue to forbid skb free >>> during unlinking. The below patch may fix the problem, are you OK >>> with it? >> >> Just skip trying this per your following email's comments. > > I will prepare a new patch later, if you'd like to try it. > > > 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
