Re: CVS commit: src/sys/dev/usb

[Nick Hudson <nick.hudson%gmx.co.uk@localhost>]

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.


> 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.

> For some bus_dma implementation (including armv5 one),
> cache is not invalidated for
> BUS_DMASYNC_POSTWRITE | BUS_DMASYNC_POSTREAD.

Neither are required for CPUs that don't do speculation.

In the case of POSTWRITE because the expectation is the device doesn't 
change memory.

In the case of POSTREAD because there should have been a PREREAD 
operation that invalidated the cache (after writing back).

What CPU are we talking about here? I think these...

- KUROBOX_PRO: https://dmesgd.nycbug.org/index.cgi?do=view&id=6594
- OPENBLOCKS_A6: https://dmesgd.nycbug.org/index.cgi?do=view&id=6595

https://developer.arm.com/documentation/prdc002691/a/?lang=en says r0p0 
has a writeback problem and shouldn't be used.

Maybe this is your problem?

Nick