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

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

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?

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

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

Therefore, stale status field is kept in cache without
2nd usb_syncmen() in ifdef DIAGNOSTIC block.

Thanks,
rin
> Nick