On 2025/09/30 14:40, Nick Hudson wrote:
On 30/09/2025 02:36, Rin Okuyama wrote:
Hi,
On 2025/09/30 1:57, Nick Hudson wrote:
On 29/09/2025 17:31, Nick Hudson wrote:
On 29/09/2025 15:21, Rin Okuyama wrote:
Module Name: src
Committed By: rin
Date: Mon Sep 29 14:21:46 UTC 2025
Modified Files:
src/sys/dev/usb: ehci.c
Log Message:
ehci: usb_syncmem against qtd **after** KASSERT for that qtd
Otherwise, we end up with stale data for DIAGNOSTIC kernel.
Fix device-probe failures discussed in PR kern/58730 for me.
This doesn't make sense to me.
The TD shouldn't be accessed by hardware at this point (unless
something is using the transfer incorrectly, e.g. twice) and the
KASSERT is there to ensure correct protocol - the status stage of
the transfer should always have toggle set.
It suggests to me that the cache operation is not writing back and
invalidating, and therefore losing the TD configuration.
To be a little clearer...
it suggests the writeback doesn't happen before an invalidation so
the toggle bit being set doesn't make it to memory.
The BUS_DMASYNC_PREREAD should ensure the writeback part.
I guess that even without writeback, CPU recognize toggle bit
as it is cached. Is this not correct? --- Perhaps, other CPU
cores turn on and check the bit?
The usb_syncmem then KASSERT was really checking the toggle bit made
it to memory and wasn't just checking cached view.
armv5 doesn't do MP so it can't be another CPU.
Thanks, I understand your intention of KASSERT. It confirms
that the toggle bit is set in memory.
Then, does the code below seems OK to you?
```
usb_syncmem(&status->dma, status->offs, sizeof(*status->qtd),
BUS_DMASYNC_PREWRITE | BUS_DMASYNC_PREREAD);
#ifdef DIAGNOSTIC
KASSERT(status->qtd->qtd_status & htole32(EHCI_QTD_TOGGLE_MASK));
usb_syncmem(&status->dma,
status->offs + offsetof(ehci_qtd_t, qtd_status),
sizeof(status->qtd->qtd_status), BUS_DMASYNC_PREREAD);
#endif
```
Not really. As above the code should be put back to the way it was.
From a point of view of **device**, the first usb_syncmem is
enough. But, for CPU, the second is necessary.
This is because CPU fetch status->qtd->qtd_status into cache
due to this KASSERT; it should be invalidated in preparation to
read updated value from device.
For other parts of echi.c, we already use usb_syncmem in
a similar manner: see, e.g.:
https://nxr.netbsd.org/xref/src/sys/dev/usb/ehci.c#976