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

[Rin Okuyama <rokuyama.rk%gmail.com@localhost>]

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

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

I observed device-probe problem in the following machines:

- OPENBLOCKS_A6 (Sheeva 88SV131 rev 1)
- NSLU2 (IXP425 266MHz rev 1)
  https://dmesgd.nycbug.org/index.cgi?do=view&id=7180
- USL-5P (SH4)
  https://dmesgd.nycbug.org/index.cgi?do=view&id=6213

As you pointed out, KUROBOX_PRO's CPU is strange. But,
NSLU2 and OPENBLOCKS_A6 are just working fine.

Thanks,
rin

> Nick