Skip to contentSkip to conversation

locking issue

2.4, 2.5, USB Samium Gromoff 3 participants 3 messages Nov 18, 2002 – Nov 20, 2002

lore
Share & tools
lore

Samium Gromoff

author
#0 lore raw

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 havent noticed any locking out there, and im afraid of what could result from a preemption happening between these two lines.

regards, Samium Gromoff
_______________________________
____________________________________

#1 lore raw
Quoted 15 lines · 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;
}

Quoted 6 lines

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 havent noticed any locking out there, and im afraid of what could result from a preemption happening between these two lines.

Um, David, any thoughts about this?

thanks,

greg k-h

#2 lore raw
Quoted 4 lines · Greg KH
Quoted 2 lines · 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.

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.

Quoted 10 lines
Quoted 10 lines

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.

Quoted 9 lines
Quoted 6 lines

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 havent noticed any locking out there, and im 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