On 16/10/2026 11.30, Junjie Cao wrote:
From: Junjie Cao <[email protected]>
Check guest-visible state for OHCI transfers whose ED type disagrees with
the endpoint type. Successful control and isochronous transfers exercise
the same endpoint paths, ruling out inactive endpoints or an idle schedule.
Signed-off-by: Junjie Cao <[email protected]>
---
Depends-on: <[email protected]>
https://lore.kernel.org/qemu-devel/[email protected]/
Based on origin/master a19f59a72d96 plus the above OHCI fix.
Tested on x86_64-softmmu and i386-softmmu with usb-audio and -audiodev none.
On x86_64, both mismatches fail on that master and pass with the
prerequisite; the normal cases pass in both. All four cases pass together
for three rounds on both targets with the fix. OHCI hotplug and
UHCI/EHCI/xHCI tests also pass.
These cases check transfer state, not the original USB_RET_ASYNC assert.
Thanks for taking care of supplying a test case for this scenario!
Some comments below...
...
+static void test_ohci_transfer(void *obj, void *data, QGuestAllocator *alloc)
+{
+ OHCITransferTest t;
+ unsigned int scenario = GPOINTER_TO_UINT(data);
+ uint32_t head, td_flags, psw, frame, done;
+ unsigned int i;
+
+ ohci_transfer_init(&t, obj, alloc);
+ ohci_get_device_descriptor(&t);
+ if (scenario == OHCI_TEST_CONTROL) {
+ qpci_iounmap(t.pci, t.bar);
+ guest_free(alloc, t.data);
+ guest_free(alloc, t.desc);
+ guest_free(alloc, t.hcca);
+ return;
+ }
+ if (scenario == OHCI_TEST_ISO_TO_CONTROL) {
+ const uint8_t setup[8] = { 0x80, 6, 0, 1, 0, 0, 18, 0 };
+ uint32_t expected[] = { CTRL_SETUP_TD(&t) };
+
+ qtest_memwrite(t.qts, SETUP_BUF(&t), setup, 8);
+ ohci_write_td(&t, CTRL_SETUP_TD(&t), TD_DP_SETUP,
+ SETUP_BUF(&t), CTRL_TAIL_TD(&t), SETUP_BUF(&t) + 7);
+ ohci_write_ed(&t, CTRL_ED(&t), 64 << 16, CTRL_SETUP_TD(&t),
+ CTRL_TAIL_TD(&t));
+ ohci_writel(&t, HC_COMMAND_STATUS, HC_CLF);
+ ohci_step(&t);
+ g_assert_cmpuint(TD_CC(qtest_readl(t.qts, CTRL_SETUP_TD(&t))), ==,
+ CC_NOERROR);
+ ohci_drain_done(&t, expected, G_N_ELEMENTS(expected));
+ ohci_periodic(&t, true, 0, true, 18);
+ } else {
+ ohci_enable_audio_stream(&t);
+ if (scenario == OHCI_TEST_GENERAL_TO_ISO) {
+ /* Prove this configured endpoint and periodic schedule work. */
+ ohci_run_valid_iso(&t);
+ g_assert_cmphex(qtest_readl(t.qts, t.hcca + 0x84), ==, 0);
+ g_assert_cmphex(ohci_readl(&t, HC_DONE_HEAD), ==, 0);
+ g_assert_cmphex(ohci_readl(&t, HC_INTERRUPT_STATUS) & HC_WD, ==,
+ 0);
+ }
+ if (scenario != OHCI_TEST_GENERAL_TO_ISO) {
You check here scenario twice for OHCI_TEST_GENERAL_TO_ISO ... I think you
could merge the two if-statements.
+ ohci_periodic(&t, true, 1, false, 192);
+ } else {
+ ohci_periodic(&t, false, 1, false, 192);
+ }
+ }
+
+ frame = ohci_readl(&t, HC_FM_NUMBER) & 0xffff;
+ done = qtest_readl(t.qts, t.hcca + 0x84);
+ for (i = 0; i < 5; i++) {
+ ohci_step(&t);
+ }
+ g_assert_cmpuint((ohci_readl(&t, HC_FM_NUMBER) - frame) & 0xffff, >=, 5);
+ head = qtest_readl(t.qts, PERIODIC_ED(&t) + 8);
+ td_flags = qtest_readl(t.qts, PERIODIC_TD(&t));
+ if (scenario == OHCI_TEST_GENERAL_TO_ISO) {
+ g_assert_cmphex(qtest_readl(t.qts, t.hcca + 0x84), ==, done);
+ g_assert_cmphex(ohci_readl(&t, HC_DONE_HEAD), ==, 0);
+ g_assert_cmphex(ohci_readl(&t, HC_INTERRUPT_STATUS) & HC_WD, ==, 0);
+ g_assert_cmphex(head & ~15U, ==, PERIODIC_TD(&t));
+ g_assert_cmphex(head & ED_HEAD_HALT, ==, 0);
+ g_assert_cmpuint(TD_CC(td_flags), ==, CC_NOTACCESSED);
+ g_assert_cmphex(qtest_readl(t.qts, PERIODIC_TD(&t) + 4), ==,
+ ISO_BUF(&t));
+ } else {
+ g_assert_cmphex(head & ~15U, ==, PERIODIC_TAIL(&t));
+ /* The iTD CC does not report a packet-level STALL. */
+ g_assert_cmpuint(TD_CC(td_flags), ==, CC_NOERROR);
+ psw = qtest_readw(t.qts, PERIODIC_TD(&t) + 16);
+ g_assert_cmpuint(psw >> 12, ==,
+ scenario == OHCI_TEST_ISO_TO_CONTROL ? CC_STALL :
+ CC_NOERROR);
+ g_assert_cmpuint(psw & 0xfff, ==, 0);
+ {
+ uint32_t expected[] = { PERIODIC_TD(&t) };
+
+ ohci_drain_done(&t, expected, G_N_ELEMENTS(expected));
+ }
Having a sub-block here for the expected[] declaration looks weird. Please
move the declaration of the expected[] array to the beginning of the else-{}
block instead.
+ }
+ qpci_iounmap(t.pci, t.bar);
+ guest_free(alloc, t.data);
+ guest_free(alloc, t.desc);
+ guest_free(alloc, t.hcca);
+}
+
static void test_ohci_hotplug(void *obj, void *data, QGuestAllocator *alloc)
{
usb_test_hotplug(global_qtest, "ohci", "1", NULL);
@@ -41,6 +432,7 @@ static void *ohci_pci_get_driver(void *obj, const char
*interface)
static void *ohci_pci_create(void *pci_bus, QGuestAllocator *alloc, void
*addr)
{
QOHCI_PCI *ohci_pci = g_new0(QOHCI_PCI, 1);
+ qpci_device_init(&ohci_pci->dev, pci_bus, addr);
ohci_pci->obj.get_driver = ohci_pci_get_driver;
return &ohci_pci->obj;
@@ -62,7 +454,30 @@ libqos_init(ohci_pci_register_nodes);
static void register_ohci_pci_test(void)
{
+ QOSGraphTestOptions opts = {
+ .edge.after_cmd_line = "-audiodev none,id=snd0 "
+ "-device usb-audio,audiodev=snd0,"
+ "bus=ohci.0,port=1",
+ };
+
qos_add_test("ohci_pci-test-hotplug", "pci-ohci", test_ohci_hotplug,
NULL);
+ if (strcmp(qtest_get_arch(), "i386") &&
+ strcmp(qtest_get_arch(), "x86_64")) {
+ return;
+ }
Is there any special reason that you limit this to x86? Couldn't this be run
on other targets as well (especially since OHCI is more common on non-x86
targets)?
+ if (!qtest_has_device("usb-audio")) {
+ return;
+ }
+ opts.arg = GUINT_TO_POINTER(OHCI_TEST_CONTROL);
+ qos_add_test("ohci_pci-control", "pci-ohci", test_ohci_transfer, &opts);
+ opts.arg = GUINT_TO_POINTER(OHCI_TEST_ISO);
+ qos_add_test("ohci_pci-iso", "pci-ohci", test_ohci_transfer, &opts);
+ opts.arg = GUINT_TO_POINTER(OHCI_TEST_ISO_TO_CONTROL);
+ qos_add_test("ohci_pci-iso-to-control", "pci-ohci", test_ohci_transfer,
+ &opts);
+ opts.arg = GUINT_TO_POINTER(OHCI_TEST_GENERAL_TO_ISO);
+ qos_add_test("ohci_pci-general-to-iso", "pci-ohci", test_ohci_transfer,
+ &opts);
}
Thomas
