hi Oliver:
2017-03-23 17:33 GMT+08:00 Oliver Neukum <oneukum@xxxxxxxx>:
> Am Donnerstag, den 23.03.2017, 16:39 +0800 schrieb pierre Kuo:
>
> Hi,
>
>> for sitd_link and itd_link, put wmb after the last memory access to make
>> sure ehci->period[frame] is visible by host before scheduling the
>> descriptors
>
> Please explain what the problem you are trying to fix is.
I just trace this part of code and try to make sure host will see data
structure correctly before scheduling.
>
>> Signed-off-by: pierre Kuo <vichy.kuo@xxxxxxxxx>
>> ---
>> drivers/usb/host/ehci-sched.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
>> index 980a6b3..11b517c 100644
>> --- a/drivers/usb/host/ehci-sched.c
>> +++ b/drivers/usb/host/ehci-sched.c
>> @@ -1754,8 +1754,9 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
>> itd->hw_next = *hw_p;
>> prev->itd = itd;
>> itd->frame = frame;
>> - wmb();
>
> This looks very wrong. You need to make sure that a data structure
> is written to RAM before you announce it to the hardware.
> The code before the wmb() initializes a structure and the write
> after it announces it to the hardware. This wmb() is exactly where
> it needs to be.
>
>> *hw_p = cpu_to_hc32(ehci, itd->itd_dma | Q_TYPE_ITD);
>> + /* make sure host see periodic frame change before scheduling */
>> + wmb();
>
> If you need to flush something, wmb() is the wrong primitive.
> It merely ensures ordering. It is not guaranteed to flush writes
Not guaranteed to flush writes?that makes me curious.
As far as I know, wmb() make sure any write operation before the wmb()
is completed and are observed before prior to the execution of any
subsequent write.
I can understand ur opinion that below (1) wmb is needed, since it
preserve the write order of (A) and (B).
But shouldn't we add wmb in (2) to make sure the ordering of (B) and
following writes?
sitd->hw_next = ehci->periodic[frame];
ehci->pshadow[frame].sitd = sitd;
sitd->frame = frame; --------------------> (A)
- wmb(); ------------------------------------> (1)
ehci->periodic[frame] = cpu_to_hc32(ehci, sitd->sitd_dma |
Q_TYPE_SITD); -----> (B)
+ /* make sure host see periodic frame change before scheduling */
+ wmb(); ------------------------------------> (2)
Appreciate your comment,
--
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
