Re: [PATCH v3 10/10] driver/char: add RX support to the XHCI driver

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

On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > @@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
> >      trb->ctrl |= 0x20;
> >  }
> >  
> > +static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb)
> 
> const please.
> 
> > +{
> > +    return trb->params;
> > +}
> > +
> > +static uint32_t xhci_trb_norm_len(struct xhci_trb *trb)
> 
> And again.
> 
> > +{
> > +    return trb->status & 0x1FFFF;
> > +}
> > +
> >  /**
> >   * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event
> >   * TRBs are read-only from software
> > @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct xhci_trb 
> > *trb)
> >      return trb->status >> 24;
> >  }
> >  
> > +/* Amount of data _not_ transferred */
> > +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb)
> > +{
> > +    return trb->status & 0x1FFFF;
> > +}
> 
> Same as xhci_trb_norm_len()?

Yes, I was considering to use that, but technically those are different
packets, only incidentally using the same bits.

> 
> > @@ -985,6 +1054,33 @@ static void dbc_flush(struct dbc *dbc, struct 
> > xhci_trb_ring *trb,
> >  }
> >  
> >  /**
> > + * Ensure DbC has a pending transfer TRB to receive data into.
> > + *
> > + * @param dbc the dbc to flush
> > + * @param trb the ring for the TRBs to transfer
> > + * @param wrk the work ring to receive data into
> > + */
> > +static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb,
> > +                           struct dbc_work_ring *wrk)
> 
> I can't seem to be able to spot any use of this function - it being
> static, how do things build for you?

It's in dbc_uart_poll().

> 
> > +{
> > +    struct dbc_reg *reg = dbc->dbc_reg;
> > +    uint32_t db = (readl(&reg->db) & 0xFFFF00FF) | (trb->db << 8);
> 
> I think I've seen this constant in earlier patches. Can this be
> a #define please, such that one can easily connect all the places
> where the same things is meant?

Ok.

> > +
> > +    /* Check if there is already queued TRB */
> > +    if ( xhci_trb_ring_size(trb) >= 1 )
> > +        return;
> > +
> > +    if ( dbc_work_ring_full(wrk) )
> > +        return;
> 
> What made me spot the lack of caller are these return statements.
> Without letting the caller know of the failure, how would it know
> to make another attempt later?

Next iteration of dbc_uart_poll() will take care of it.

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

Attachment: signature.asc
Description: PGP signature