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(-)

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), &reg->ctrl);
         writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->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(&reg->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, &reg->db);
+    dbc_ring_doorbell(dbc, trb->db);
 }
 
 /**
-- 
git-series 0.9.1

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), &reg->ctrl);
>          writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->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, &reg->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), &reg->ctrl);
>>          writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->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, &reg->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), &reg->ctrl);
>>>          writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->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, &reg->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