Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Aug 05, 2022 at 09:23:32AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > +static uint64_t dbc_work_ring_size(const struct dbc_work_ring *ring)
> > +{
> > +    if ( ring->enq >= ring->deq )
> > +        return ring->enq - ring->deq;
> > +
> > +    return DBC_WORK_RING_CAP - ring->deq + ring->enq;
> > +}
> 
> Doesn't unsigned int suffice as a return type here?

Yes, it does.

> > +static int64_t dbc_push_work(struct dbc *dbc, struct dbc_work_ring *ring,
> > +                             const char *buf, unsigned int len)
> > +{
> > +    unsigned int i = 0;
> > +    unsigned int end, start = ring->enq;
> > +
> > +    while ( !dbc_work_ring_full(ring) && i < len )
> > +    {
> > +        ring->buf[ring->enq] = buf[i++];
> > +        ring->enq = (ring->enq + 1) & (DBC_WORK_RING_CAP - 1);
> > +    }
> > +
> > +    end = ring->enq;
> > +
> > +    if ( end > start )
> > +        cache_flush(&ring->buf[start], end - start);
> > +    else if ( i > 0 )
> > +    {
> > +        cache_flush(&ring->buf[start], DBC_WORK_RING_CAP - start);
> > +        cache_flush(&ring->buf[0], end);
> > +    }
> > +
> > +    return i;
> > +}
> 
> The function's return type is int64_t but the sole return statement
> hands back an unsigned int - what's the deal here?

And also, the only use for the return value is comparing to 0. So, yes,
should be unsigned int.

> > +static struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
> > +static struct xhci_trb out_trb[DBC_TRB_RING_CAP];
> > +static struct xhci_trb in_trb[DBC_TRB_RING_CAP];
> > +static struct xhci_erst_segment erst __aligned(64);
> > +static struct xhci_dbc_ctx ctx __aligned(64);
> > +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> 
> I've been trying to identify the reason for the alignment here,
> compared to the other buffers which are no longer page-aligned. I
> haven't even been able to locate the place where the address of
> this buffer is actually written to hardware; all I could find was
> the respective virt_to_maddr(). Could you please point me at that?

It's dbc_flush() -> dbc_push_trb().
And indeed, I think I can drop the alignment when it's moved into
structure dedicated for DMA-accessible buffers.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature