| Message ID | bf26655295d0d85b1718d60f2e4390da7ec62b93.1661181584.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote: > @@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc) > writel(ctrl | (1U << DBC_CTRL_DRC), ®->ctrl); > writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc); > wmb(); > + dbc_ring_doorbell(dbc, dbc->dbc_iring.db); > + dbc_ring_doorbell(dbc, dbc->dbc_oring.db); > } You retain the wmb() here, but ... > @@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, > } > } > > - wmb(); > - writel(db, ®->db); > + dbc_ring_doorbell(dbc, trb->db); > } ... you drop it here. Why the difference? Jan
On 26/08/2022 15:50, Jan Beulich wrote: > On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote: >> @@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc) >> writel(ctrl | (1U << DBC_CTRL_DRC), ®->ctrl); >> writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc); >> wmb(); >> + dbc_ring_doorbell(dbc, dbc->dbc_iring.db); >> + dbc_ring_doorbell(dbc, dbc->dbc_oring.db); >> } > You retain the wmb() here, but ... > >> @@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, >> } >> } >> >> - wmb(); >> - writel(db, ®->db); >> + dbc_ring_doorbell(dbc, trb->db); >> } > ... you drop it here. Why the difference? As a tangent, every single barrier in this file is buggy. Should be smp_*() variants, not mandatory variants. All (interesting) data is in plain WB cached memory, and the few BAR registers which are configured have a UC mapping which orders properly WRT other writes on x86. ~Andrew
On 26.08.2022 17:44, Andrew Cooper wrote: > On 26/08/2022 15:50, Jan Beulich wrote: >> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote: >>> @@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc) >>> writel(ctrl | (1U << DBC_CTRL_DRC), ®->ctrl); >>> writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc); >>> wmb(); >>> + dbc_ring_doorbell(dbc, dbc->dbc_iring.db); >>> + dbc_ring_doorbell(dbc, dbc->dbc_oring.db); >>> } >> You retain the wmb() here, but ... >> >>> @@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, >>> } >>> } >>> >>> - wmb(); >>> - writel(db, ®->db); >>> + dbc_ring_doorbell(dbc, trb->db); >>> } >> ... you drop it here. Why the difference? > > As a tangent, every single barrier in this file is buggy. Should be > smp_*() variants, not mandatory variants. > > All (interesting) data is in plain WB cached memory, and the few BAR > registers which are configured have a UC mapping which orders properly > WRT other writes on x86. But such drivers shouldn't be x86-specific when it comes to their use of barriers. For this reason I specifically did not complain about any of the barrier uses throughout the series (with the further thinking of "better one too many than one too few"). Jan
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c index e838e285fb75..0ff340dac103 100644 --- a/xen/drivers/char/xhci-dbc.c +++ b/xen/drivers/char/xhci-dbc.c @@ -553,6 +553,15 @@ static unsigned int dbc_work_ring_space_to_end(const struct dbc_work_ring *ring) return ring->deq - ring->enq; } +static void dbc_ring_doorbell(struct dbc *dbc, int doorbell) +{ + uint32_t __iomem *db_reg = &dbc->dbc_reg->db; + uint32_t db = (readl(db_reg) & ~DBC_DOORBELL_TARGET_MASK) | + (doorbell << DBC_DOORBELL_TARGET_SHIFT); + + writel(db, db_reg); +} + static void dbc_push_trb(struct dbc *dbc, struct xhci_trb_ring *ring, uint64_t dma, uint64_t len) { @@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc) writel(ctrl | (1U << DBC_CTRL_DRC), ®->ctrl); writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc); wmb(); + dbc_ring_doorbell(dbc, dbc->dbc_iring.db); + dbc_ring_doorbell(dbc, dbc->dbc_oring.db); } return true; @@ -1040,10 +1051,6 @@ static bool dbc_ensure_running(struct dbc *dbc) static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, struct dbc_work_ring *wrk) { - struct dbc_reg *reg = dbc->dbc_reg; - uint32_t db = (readl(®->db) & ~DBC_DOORBELL_TARGET_MASK) | - (trb->db << DBC_DOORBELL_TARGET_SHIFT); - if ( xhci_trb_ring_full(trb) ) return; @@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb, } } - wmb(); - writel(db, ®->db); + dbc_ring_doorbell(dbc, trb->db); } /**
When cable is unplugged, dbc_ensure_running() correctly detects this situation (DBC_CTRL_DCR flag is clear), and prevent sending data immediately to the device. It gets only queued in work ring buffers. When cable is plugged in again, subsequent dbc_flush() will send the buffered data. But there is a corner case, where no subsequent data was buffered in the work buffer, but a TRB was still pending. Ring the doorbell to let the controller re-send them. For console output it is rare corner case (TRB is pending for a very short time), but for console input it is very normal case (there is always one pending TRB for input). Extract doorbell ringing into separate function to avoid duplication. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- xen/drivers/char/xhci-dbc.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)