lkml.org 
[lkml]   [2015]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH net-next 1/7] r8152: adjust rx_bottom
From
On Mon, Jan 19, 2015 at 6:48 PM, Hayes Wang <hayeswang@realtek.com> wrote:
> David Miller [mailto:davem@davemloft.net]
>> Sent: Tuesday, January 20, 2015 5:14 AM
> [...]
>> >> - r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> + if (!ret) {
>> >> + ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
>> >> + } else {
>> >> + urb->actual_length = 0;
>> >> + list_add_tail(&agg->list, next);
>> >
>> > Do you need a spin_lock_irqsave(&tp->rx_lock, flags) around this?
>>
>> Indeed, and rtl_start_rx() seems to also access agg->list without
>> proper locking.
>
> It is unnecessary because I deal with them in a local list_head. My steps are
> 1. Move the whole list from tp->rx_done to local rx_queue. (with spin lock)
> 2. dequeue/enqueue the lists in rx_queue.
> 3. Move the lists in rx_queue to tp->rx_done if it is necessary. (spin lock)
> For step 2, it wouldn't have race, because the list_head is local and no other
> function would change it. Therefore, I don't think it needs the spin lock.

Sorry guys, I think I made a mistake in my review and caused some
confusion/grief.

My mistake was getting the params to list_add_tail() backwards. It's
list_add_tail(entry, head). I saw list_add_tail(&agg->list, next) and
was fooled into thinking agg->list was the list getting appended with
the entry 'next'. It's the opposite. Duh. So locking isn't needed
because the list is indeed local.

-scott


\
 
 \ /
  Last update: 2015-01-26 10:21    [W:0.073 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site