On Thu Dec 19, 2024 at 1:08 AM AEST, Phil Dennis-Jordan wrote:
> This looks sensible to me overall.
>
> For the new symbolic constants for MMIO register offsets such as
> XHCI_OPER_*, XHCI_INTR_* and so on, I'm wondering if it would be clearer to
> give them all an _OFFSET suffix. It's not perfectly consistent to do so
> across the code base, but quite a few device types do follow that
> convention. In my opinion it improves readability, especially in the header
> file, where these offset constants are frequently mixed in with constant
> values that can be written to or read from these registers.
I'm not strongly attached. I slightly prefer suffix, like
XHCI_HCCAP_REG_CAPLENGTH) for MMIO regs, so all the common
part of the name lines up.
I'm guessing you mean prefix? (_REG_ here) Works fine as well from my point of view.
Should the entire codebase have a guideline for memory offset
naming conventions? Or does it not matter that much.
I'm not aware of a *documented* convention.
[snip]
> > - case 0x0c: /* HCSPARAMS 3 */
> > + case XHCI_HCCAP_HCSPARAMS3:
> > ret = 0x00000000;
> > break;
> > - case 0x10: /* HCCPARAMS */
> > - if (sizeof(dma_addr_t) == 4) {
> > - ret = 0x00080000 | (xhci->max_pstreams_mask << 12);
> > - } else {
> > - ret = 0x00080001 | (xhci->max_pstreams_mask << 12);
> > + case XHCI_HCCAP_HCCPARAMS1:
> > + ret = (XHCI_HCCAP_EXTCAP_START >> 2) | (xhci->max_pstreams_mask
> > << 12);
> >
>
> This doesn't look like it's equivalent to the original code. I think you
> want
> ((XHCI_HCCAP_EXTCAP_START >> 2) << 16) | (xhci->max_pstreams_mask << 12);
Good catch.
>
> That's… not particularly readable either though, so if we're going to break
> up the magic numbers here, how about something like:
>
> ret = (XHCI_HCCAP_EXTCAP_START / 4) << XHCI_HCCPARAM_EXTPTR_SHIFT;
> ret |= xhci->max_pstreams_mask << XHCI_HCCPARAM_MAXPSASIZE_SHIFT;
>
>
> > + if (sizeof(dma_addr_t) == 8) {
> > + ret |= 0x00000001; /* AC64 */
> >
>
> and then this can become
> ret |= XHCI_HCCPARAM_AC64;
>
> or something like that.
Sure.
[snip]
> > switch (reg) {
> > - case 0x00: /* PORTSC */
> > + case XHCI_PORT_PORTSC:
> > ret = port->portsc;
> > break;
> > - case 0x04: /* PORTPMSC */
> > - case 0x08: /* PORTLI */
> > + case XHCI_PORT_PORTPMSC:
> > + case XHCI_PORT_PORTLI:
> > ret = 0;
> > break;
> > - case 0x0c: /* reserved */
> >
>
> I think it's worth keeping explicitly unhandled case labels documented like
> this. (This one appears to be XHCI_PORT_PORTHLPMC nowadays, I assume it was
> reserved in an earlier spec version.)
Okay.
> default:
> > trace_usb_xhci_unimplemented("port read", reg);
> > ret = 0;
> > @@ -2829,7 +2668,7 @@ static void xhci_port_write(void *ptr, hwaddr reg,
> > trace_usb_xhci_port_write(port->portnr, reg, val);
> >
> > switch (reg) {
> > - case 0x00: /* PORTSC */
> > + case XHCI_PORT_PORTSC:
> > /* write-1-to-start bits */
> > if (val & PORTSC_WPR) {
> > xhci_port_reset(port, true);
> > @@ -2880,8 +2719,6 @@ static void xhci_port_write(void *ptr, hwaddr reg,
> > xhci_port_notify(port, notify);
> > }
> > break;
> > - case 0x04: /* PORTPMSC */
> > - case 0x08: /* PORTLI */
> >
>
> Hmm. Looks like PORTLI is actually a read-only register, so writing this
> ought to trigger a LOG_GUEST_ERROR. And I don't think it's a bad thing to
> explicitly document PORTPMSC as unimplemented. (And I guess that ought to
> be a LOG_UNIMP, not a trace, sigh.) The improved logging can be a separate
> commit - in fact I don't mind tagging that fix onto my own pending XHCI
> patch set, but I think for this commit we ought to keep the case labels
> (with the new symbolic constants).
Okay I'll add back the reserved cases.
Thanks - I can only speak for myself, but I do tend to find those useful when first getting into the code for a device. (Plus turning on GUEST_ERROR and UNIMP logging is usually my first step when debugging issues in a new-to-me device, and we can only really wire those up if all the labels are there.)
I've put together a patch to wire up the correct logging for these cases, and I'll add that to the next iteration of my pending XHCI patch set, so don't need to bother with that. It'll cause merge conflicts with your patch but that'll be easy to fix in whichever patch ends up getting merged second.
Yeah, memory access handling in drivers is really inconsistent all over
the tree. It would be nice if there was some helpers or something that
added common template for access tracing, unimp and guest error logs,
etc.
Yeah, it's a bit of a jungle out there in the code base, but there's also tons of weird edge cases that need handling, so I guess setting hard rules is just difficult. Still, calling out some positive examples in the contributor docs might be useful.
Thanks,
Nick