[2.4, 2.5, USB] locking issue

[2.4, 2.5, USB] locking issue

[Samium Gromoff]

        The possible problem is encountered in ehci-q.c and ehci-sched.c
  in 2.4.19-pre9 and in one occurence in ehci-q.c of 2.5.47.

        the offending pattern is the same in both files:

        if (!list_empty (qtd_list)) {
 -----------------------8<----------------------------------------------
                list_splice (qtd_list, &qh->qtd_list);
                qh_update (qh, list_entry (qtd_list->next, struct ehci_qtd, qtd\_list));
 -----------------------8<----------------------------------------------
        } else {
                qh->hw_qtd_next = qh->hw_alt_next = EHCI_LIST_END;
        }


        since list_splice() the qtd_list is diposed of its belongings and
        immediately in the next line we rely on qtd_list->next to point
        at an existing list_head.

        i haven`t noticed any locking out there, and i`m afraid of what
        could result from a preemption happening between these two lines.


regards, Samium Gromoff
_______________________________
____________________________________

Re: [2.4, 2.5, USB] locking issue

[Greg KH]

On Mon, Nov 18, 2002 at 05:34:20PM +0300, Samium Gromoff wrote:
>         The possible problem is encountered in ehci-q.c and ehci-sched.c
>   in 2.4.19-pre9 and in one occurence in ehci-q.c of 2.5.47.
> 
>         the offending pattern is the same in both files:
> 
>         if (!list_empty (qtd_list)) {
>  -----------------------8<----------------------------------------------
>                 list_splice (qtd_list, &qh->qtd_list);
>                 qh_update (qh, list_entry (qtd_list->next, struct ehci_qtd, qtd\_list));
>  -----------------------8<----------------------------------------------
>         } else {
>                 qh->hw_qtd_next = qh->hw_alt_next = EHCI_LIST_END;
>         }
> 
> 
>         since list_splice() the qtd_list is diposed of its belongings and
>         immediately in the next line we rely on qtd_list->next to point
>         at an existing list_head.
> 
>         i haven`t noticed any locking out there, and i`m afraid of what
>         could result from a preemption happening between these two lines.

Um, David, any thoughts about this?

thanks,

greg k-h

Re: [2.4, 2.5, USB] locking issue

[David Brownell]

Greg KH wrote:
> On Mon, Nov 18, 2002 at 05:34:20PM +0300, Samium Gromoff wrote:
> 
>>        The possible problem is encountered in ehci-q.c and ehci-sched.c
>>  in 2.4.19-pre9 and in one occurence in ehci-q.c of 2.5.47.

It's not a problem, see below.  The 2.5 code is better, since only one
body of code is managing the TD queues for async (control, bulk) and
interrupt queue heads.


>>        the offending pattern is the same in both files:
>>
>>        if (!list_empty (qtd_list)) {
>> -----------------------8<----------------------------------------------
>>                list_splice (qtd_list, &qh->qtd_list);
>>                qh_update (qh, list_entry (qtd_list->next, struct ehci_qtd, qtd_list));
>> -----------------------8<----------------------------------------------
>>        } else {
>>                qh->hw_qtd_next = qh->hw_alt_next = EHCI_LIST_END;
>>        }

Slightly different in 2.5.48 since it uses a dummy TD (so certain race
conditions can't happen), and it won't line-wrap that qh_update() call.


>>        since list_splice() the qtd_list is diposed of its belongings and
>>        immediately in the next line we rely on qtd_list->next to point
>>        at an existing list_head.
>>
>>        i haven`t noticed any locking out there, and i`m afraid of what
>>        could result from a preemption happening between these two lines.
> 
> 
> Um, David, any thoughts about this?

That code runs under a spinlock_irqsave(&ehci->lock,...), so no
other task can preempt.  And list_splice() doesn't modify qtd_list,
so the qtd_list->next pointer stays valid ... it's like list_del(),
not list_del_init(), read <linux/list.h> to see.

- Dave