Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OHCI: Fixed a bug in the OHCI implementation from QEMU #1531

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
+168 −62
Open

OHCI: Fixed a bug in the OHCI implementation from QEMU #1531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ff79cd5
Fixed a bug in OHCI
faha223 Sep 23, 2023
a3d61ca
Added a missing check for usb_packet_is_inflight
faha223 Oct 7, 2023
b5de99f
Fixed some nits and used QTAILQ_INIT to initialize active_packets
faha223 Oct 22, 2023
28119cf
Merge branch 'master' into ohci_fix
faha223 Nov 23, 2023
7e0734c
Added back usb_buf, async_td, and async_complete
faha223 Nov 27, 2023
d3e4b0e
Merge pull request #23 from faha223/master
faha223 Dec 18, 2023
114d994
Merge branch 'ohci_fix' of https://github.com/faha223/xemu into ohci_fix
faha223 Dec 18, 2023
fe10357
Added logic to clear out the packets in usb_ohci_exit
faha223 Feb 26, 2024
51a0a4e
Removed the unnecessary foreach loop. No need to clear out the packet…
faha223 Feb 26, 2024
d265219
Replicated the change from the FixMacOsBuild branch so that the macos…
faha223 Feb 26, 2024
adb0eb3
ran clang-format on the branch
faha223 Feb 28, 2024
98aa18b
Merge pull request #24 from xemu-project/master
faha223 Mar 10, 2024
950519a
Merge branch 'master' into ohci_fix
faha223 Mar 10, 2024
3b15f6a
Merge branch 'xemu-project:master' into ohci_fix
faha223 Sep 20, 2024
9f0e17f
Merge branch 'master' into ohci_fix
faha223 Dec 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions hw/usb/hcd-ohci-pci.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,13 @@ static void usb_ohci_realize_pci(PCIDevice *dev, Error **errp)

static void usb_ohci_exit(PCIDevice *dev)
{
fprintf(stderr, "usb_ohci_exit\n");
OHCIPCIState *ohci = PCI_OHCI(dev);
OHCIState *s = &ohci->state;

trace_usb_ohci_exit(s->name);
ohci_bus_stop(s);

if (s->async_td) {
usb_cancel_packet(&s->usb_packet);
s->async_td = 0;
}
ohci_stop_endpoints(s);

if (!ohci->masterbus) {
Expand Down
204 changes: 149 additions & 55 deletions hw/usb/hcd-ohci.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,7 @@ void ohci_stop_endpoints(OHCIState *ohci)
USBDevice *dev;
int i, j;

if (ohci->async_td) {
usb_cancel_packet(&ohci->usb_packet);
ohci->async_td = 0;
}
ohci_clear_active_packets(ohci);
for (i = 0; i < ohci->num_ports; i++) {
dev = ohci->rhport[i].port.dev;
if (dev && dev->attached) {
Expand Down Expand Up @@ -869,18 +866,18 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
return 1;
}

/* See if this TD has already been submitted to the device. */
completion = (addr == ohci->async_td);
if (completion && !ohci->async_complete) {
trace_usb_ohci_td_skip_async();
return 1;
}
if (ohci_read_td(ohci, addr, &td)) {
trace_usb_ohci_td_read_error(addr);
ohci_die(ohci);
return 1;
}

dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
if (dev == NULL) {
trace_usb_ohci_td_dev_error();
return 1;
}

dir = OHCI_BM(ed->flags, ED_D);
switch (dir) {
case OHCI_TD_DIR_OUT:
Expand Down Expand Up @@ -909,6 +906,35 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
trace_usb_ohci_td_bad_direction(dir);
return 1;
}

ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
USBActivePacket *packet = NULL;
USBActivePacket *iter;
QTAILQ_FOREACH (iter, &ohci->active_packets, next) {
if (iter->ep == ep) {
packet = iter;
break;
}
}

/* A packet for this endpoint doesn't exist yet. Make one */
if (packet == NULL) {
packet = g_malloc(sizeof(USBActivePacket));
usb_packet_init(&packet->usb_packet);
packet->ep = ep;
packet->async_complete = false;
packet->async_td = 0;
packet->ohci = ohci;
QTAILQ_INSERT_TAIL(&ohci->active_packets, packet, next);
}

/* See if this TD has already been submitted to the device. */
completion = (addr == packet->async_td);
if (completion && !packet->async_complete) {
trace_usb_ohci_td_skip_async();
return 1;
}

if (td.cbp && td.be) {
if ((td.cbp & 0xfffff000) != (td.be & 0xfffff000)) {
len = (td.be & 0xfff) + 0x1001 - (td.cbp & 0xfff);
Expand All @@ -920,8 +946,8 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
}
len = (td.be - td.cbp) + 1;
}
if (len > sizeof(ohci->usb_buf)) {
len = sizeof(ohci->usb_buf);
if (len > sizeof(packet->usb_buf)) {
len = sizeof(packet->usb_buf);
}

pktlen = len;
Expand All @@ -932,7 +958,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
pktlen = len;
}
if (!completion) {
if (ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen,
if (ohci_copy_td(ohci, &td, packet->usb_buf, pktlen,
DMA_DIRECTION_TO_DEVICE)) {
ohci_die(ohci);
}
Expand All @@ -943,52 +969,42 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
flag_r = (td.flags & OHCI_TD_R) != 0;
trace_usb_ohci_td_pkt_hdr(addr, (int64_t)pktlen, (int64_t)len, str,
flag_r, td.cbp, td.be);
ohci_td_pkt("OUT", ohci->usb_buf, pktlen);
ohci_td_pkt("OUT", packet->usb_buf, pktlen);

if (completion) {
ohci->async_td = 0;
ohci->async_complete = false;
packet->async_td = 0;
packet->async_complete = false;
} else {
dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
if (dev == NULL) {
trace_usb_ohci_td_dev_error();
return 1;
}
ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
if (ohci->async_td) {
/* ??? The hardware should allow one active packet per
endpoint. We only allow one active packet per controller.
This should be sufficient as long as devices respond in a
timely manner.
*/
if (packet->async_td) {
// Only allow one active packet per endpoint.
trace_usb_ohci_td_too_many_pending(ep->nr);
return 1;
}
usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r,
usb_packet_setup(&packet->usb_packet, pid, ep, 0, addr, !flag_r,
OHCI_BM(td.flags, TD_DI) == 0);
usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
usb_handle_packet(dev, &ohci->usb_packet);
trace_usb_ohci_td_packet_status(ohci->usb_packet.status);
usb_packet_addbuf(&packet->usb_packet, packet->usb_buf, pktlen);
usb_handle_packet(dev, &packet->usb_packet);
trace_usb_ohci_td_packet_status(packet->usb_packet.status);

if (ohci->usb_packet.status == USB_RET_ASYNC) {
if (packet->usb_packet.status == USB_RET_ASYNC) {
usb_device_flush_ep_queue(dev, ep);
ohci->async_td = addr;
packet->async_td = addr;
return 1;
}
}
if (ohci->usb_packet.status == USB_RET_SUCCESS) {
ret = ohci->usb_packet.actual_length;
if (packet->usb_packet.status == USB_RET_SUCCESS) {
ret = packet->usb_packet.actual_length;
} else {
ret = ohci->usb_packet.status;
ret = packet->usb_packet.status;
}

if (ret >= 0) {
if (dir == OHCI_TD_DIR_IN) {
if (ohci_copy_td(ohci, &td, ohci->usb_buf, ret,
if (ohci_copy_td(ohci, &td, packet->usb_buf, ret,
DMA_DIRECTION_FROM_DEVICE)) {
ohci_die(ohci);
}
ohci_td_pkt("IN", ohci->usb_buf, pktlen);
ohci_td_pkt("IN", packet->usb_buf, pktlen);
} else {
ret = pktlen;
}
Expand Down Expand Up @@ -1078,15 +1094,20 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
/* Service an endpoint list. Returns nonzero if active TD were found. */
static int ohci_service_ed_list(OHCIState *ohci, uint32_t head)
{
struct ohci_td td;
struct ohci_ed ed;
uint32_t next_ed;
uint32_t cur;
int active;
int dir, pid;
uint32_t link_cnt = 0;
active = 0;
USBDevice *dev;
USBEndpoint *ep;

if (head == 0)
if (head == 0) {
return 0;
}

for (cur = head; cur && link_cnt++ < ED_LINK_LIMIT; cur = next_ed) {
if (ohci_read_ed(ohci, cur, &ed)) {
Expand All @@ -1101,11 +1122,62 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head)
uint32_t addr;
/* Cancel pending packets for ED that have been paused. */
addr = ed.head & OHCI_DPTR_MASK;
if (ohci->async_td && addr == ohci->async_td) {
usb_cancel_packet(&ohci->usb_packet);
ohci->async_td = 0;
usb_device_ep_stopped(ohci->usb_packet.ep->dev,
ohci->usb_packet.ep);

if (ohci_read_td(ohci, addr, &td)) {
trace_usb_ohci_td_read_error(addr);
ohci_die(ohci);
return 1;
}

dir = OHCI_BM(ed.flags, ED_D);
switch (dir) {
case OHCI_TD_DIR_OUT:
case OHCI_TD_DIR_IN:
/* Same value. */
break;
default:
dir = OHCI_BM(td.flags, TD_DP);
break;
}

switch (dir) {
case OHCI_TD_DIR_IN:
pid = USB_TOKEN_IN;
break;
case OHCI_TD_DIR_OUT:
pid = USB_TOKEN_OUT;
break;
case OHCI_TD_DIR_SETUP:
pid = USB_TOKEN_SETUP;
break;
default:
continue;
}

dev = ohci_find_device(ohci, OHCI_BM(ed.flags, ED_FA));
if (dev != NULL) {
ep = usb_ep_get(dev, pid, OHCI_BM(ed.flags, ED_EN));

if (ep != NULL) {
USBActivePacket *iter;
QTAILQ_FOREACH (iter, &ohci->active_packets, next) {
if (iter->ep == ep) {
if (iter->async_td && addr == iter->async_td) {
if (usb_packet_is_inflight(&iter->usb_packet))
usb_cancel_packet(&iter->usb_packet);
iter->async_td = 0;
usb_device_ep_stopped(iter->usb_packet.ep->dev,
iter->usb_packet.ep);
}
break;
}
}
}
} else {
if (ohci_put_ed(ohci, cur, &ed)) {
ohci_die(ohci);
return 0;
}
}
continue;
}
Expand Down Expand Up @@ -1751,11 +1823,17 @@ static void ohci_child_detach(USBPort *port1, USBDevice *dev)
{
OHCIState *ohci = port1->opaque;

if (ohci->async_td &&
usb_packet_is_inflight(&ohci->usb_packet) &&
ohci->usb_packet.ep->dev == dev) {
usb_cancel_packet(&ohci->usb_packet);
ohci->async_td = 0;
USBActivePacket *iter, *iter2;
QTAILQ_FOREACH_SAFE (iter, &ohci->active_packets, next, iter2) {
if (iter->usb_packet.ep->dev == dev) {
if (iter->async_td && usb_packet_is_inflight(&iter->usb_packet) &&
iter->usb_packet.ep->dev == dev) {
usb_cancel_packet(&iter->usb_packet);
}

QTAILQ_REMOVE(&ohci->active_packets, iter, next);
g_free(iter);
}
}
}

Expand Down Expand Up @@ -1812,11 +1890,12 @@ static void ohci_wakeup(USBPort *port1)

static void ohci_async_complete_packet(USBPort *port, USBPacket *packet)
{
OHCIState *ohci = container_of(packet, OHCIState, usb_packet);
USBActivePacket *active_packet =
container_of(packet, USBActivePacket, usb_packet);

trace_usb_ohci_async_complete();
ohci->async_complete = true;
ohci_process_lists(ohci);
active_packet->async_complete = true;
ohci_process_lists(active_packet->ohci);
}

static USBPortOps ohci_port_ops = {
Expand Down Expand Up @@ -1890,9 +1969,8 @@ void usb_ohci_init(OHCIState *ohci, DeviceState *dev, uint32_t num_ports,
ohci->localmem_base = localmem_base;

ohci->name = object_get_typename(OBJECT(dev));
usb_packet_init(&ohci->usb_packet);

ohci->async_td = 0;
QTAILQ_INIT(&ohci->active_packets);

ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
ohci_frame_boundary, ohci);
Expand All @@ -1906,10 +1984,26 @@ void ohci_sysbus_die(struct OHCIState *ohci)
{
trace_usb_ohci_die();

ohci_clear_active_packets(ohci);
ohci_set_interrupt(ohci, OHCI_INTR_UE);
ohci_bus_stop(ohci);
}

void ohci_clear_active_packets(struct OHCIState *ohci)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why clear and not cancel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear because this function makes the list empty. I am not opposed to changing it to cancel

{
while (!QTAILQ_EMPTY(&ohci->active_packets)) {
USBActivePacket *packet = QTAILQ_FIRST(&ohci->active_packets);
if (packet->async_td) {
usb_cancel_packet(&packet->usb_packet);
packet->async_td = 0;
usb_device_ep_stopped(packet->usb_packet.ep->dev,
packet->usb_packet.ep);
}
QTAILQ_REMOVE(&ohci->active_packets, packet, next);
g_free(packet);
}
}

static void ohci_realize_pxa(DeviceState *dev, Error **errp)
{
OHCISysBusState *s = SYSBUS_OHCI(dev);
Expand Down
21 changes: 18 additions & 3 deletions hw/usb/hcd-ohci.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,21 @@ typedef struct OHCIPort {
uint32_t ctrl;
} OHCIPort;

typedef struct OHCIState {
typedef struct USBActivePacket USBActivePacket;
typedef struct OHCIState OHCIState;

struct USBActivePacket {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is basically 1:1 with endpoints, does it makes sense to just move the async tracking into USBEndpoint and avoid all the list iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USBEndpoint is shared between OHCI, EHCI, UHCI, and XHCI. Moving the async tracking into USBEndpoint would make sense from the perspective of OHCI, but wouldn't be used by EHCI, UHCI, or XHCI

USBEndpoint *ep;
USBPacket usb_packet;
uint8_t usb_buf[8192];
uint32_t async_td;
bool async_complete;
OHCIState *ohci;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need OHCIState pointer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in ohci_async_complete_packet when calling ohci_process_lists. This is the only place it is used.

We get USBActivePacket by using container_of on the USBPacket argument. It might be possible to use container_of to get the OHCI pointer we need here if we can use one of the QTAILQ macros to get to the head of the list.


QTAILQ_ENTRY(USBActivePacket) next;
};

struct OHCIState {
USBBus bus;
qemu_irq irq;
MemoryRegion mem;
Expand Down Expand Up @@ -84,13 +98,13 @@ typedef struct OHCIState {

/* Active packets. */
uint32_t old_ctl;
USBPacket usb_packet;
uint8_t usb_buf[8192];
uint32_t async_td;
bool async_complete;
QTAILQ_HEAD(, USBActivePacket) active_packets;

void (*ohci_die)(struct OHCIState *ohci);
} OHCIState;
};

#define TYPE_SYSBUS_OHCI "sysbus-ohci"
OBJECT_DECLARE_SIMPLE_TYPE(OHCISysBusState, SYSBUS_OHCI)
Expand All @@ -117,5 +131,6 @@ void ohci_bus_stop(OHCIState *ohci);
void ohci_stop_endpoints(OHCIState *ohci);
void ohci_hard_reset(OHCIState *ohci);
void ohci_sysbus_die(struct OHCIState *ohci);
void ohci_clear_active_packets(struct OHCIState *ohci);

#endif
Loading