Re: [PATCH v7 05/10] usb: dbc: add bulk out and bulk in interfaces

[Lu Baolu]

On 02/18/2016 09:32 PM, Mathias Nyman wrote:
> On 26.01.2016 14:58, Lu Baolu wrote:
>> This patch adds interfaces for bulk out and bulk in ops. These
>> interfaces could be used to implement early printk bootconsole
>> or hook to various system debuggers.
>>
>> Signed-off-by: Lu Baolu 
>> ---
>>   drivers/usb/early/xhci-dbc.c | 373 
>> +++
>>   include/linux/usb/xhci-dbc.h |  30 
>>   2 files changed, 403 insertions(+)
>>
>
> ...
>
>> +
>> +/*
>> + * Check and dispatch events in event ring. It also checks status
>> + * of hardware. This function will be called from multiple threads.
>> + * An atomic lock is applied to protect the access of event ring.
>> + */
>> +static int xdbc_check_event(void)
>> +{
>> +/* event ring is under checking by other thread? */
>> +if (!test_bit(XDBC_ATOMIC_EVENT, >atomic_flags) &&
>> +!test_and_set_bit(XDBC_ATOMIC_EVENT,
>> +>atomic_flags))
>> +return 0;
>
> homemade trylock, can't the real ones be used?
>
>> +
>> +xdbc_handle_events();
>> +
>> +test_and_clear_bit(XDBC_ATOMIC_EVENT, >atomic_flags);
>> +
>> +return 0;
>>  +}
>> +
>> +#defineBULK_IN_COMPLETED(p)((xdbcp->in_pending == (p)) && \
>> + xdbcp->in_complete)
>> +#defineBULK_OUT_COMPLETED(p)((xdbcp->out_pending == (p)) && \
>> + xdbcp->out_complete)
>> +
>
> ...
>
>> +}
>> +
>> +int xdbc_bulk_read(void *data, int size, int loops)
>> +{
>> +int ret;
>> +
>> +do {
>> +if (!test_bit(XDBC_ATOMIC_BULKIN, >atomic_flags) &&
>> +!test_and_set_bit(XDBC_ATOMIC_BULKIN,
>> +>atomic_flags))
>> +break;
>> +} while (1);
>
> homemeade spin_lock, can't the real one be used?
>
> If the xdbc_bulk_write() can be accessed from interrupt context (handler, 
> soft, timer) it
> may deadlock
>
>> +
>> +ret = xdbc_bulk_transfer(data, size, loops, true);
>> +
>> +test_and_clear_bit(XDBC_ATOMIC_BULKIN, >atomic_flags);
>> +
>> +return ret;
>> +}
>> +
>> +int xdbc_bulk_write(const char *bytes, int size)
>> +{
>> +int ret;
>> +
>> +do {
>> +if (!test_bit(XDBC_ATOMIC_BULKOUT, >atomic_flags) &&
>> +!test_and_set_bit(XDBC_ATOMIC_BULKOUT,
>> +>atomic_flags))
>> +break;
>> +} while (1);
>
> Another homemeade spin_lock, can't the real one be used?
>
> same issue here, deadlock if accessible from interrupt context.

I will try to rework this spin_lock with the real one and keep avoiding 
deadlock in mind.

>
>
> Would it make sense to have only one spinlock, and start one separate thread 
> for
> reading the event ring. The thread would,  lock, handle pending events, 
> unlock,
> then call shedule, in a loop. ehci early debug code has some variant of this.

Let me try to find this part of code.

>
> So the lock would be taken while events are being handled.
>
> The same lock would be used for bulk_read and bulk_write. Yes this would 
> prevent read and
> write at the same time, and the read and writes need to be modified to not 
> block until
> the reansfer is finished, just to write the TRBs on the ring, update ring 
> pointers,
> and ring the doorbell.
>
> Or is all this impossibe due to the earlyness of the code?

It's not only due to earlyness of the code. But also, these read/write ops were 
designed to
be used by a debugger (for example kgdb) as well. Using the kernel provided 
interface
might make things simple, but what should happen when the debugger is used
to debug the kernel subsystem itself?

So, it seems that I should implement read/write ops depends on the use case. 
For this
time being, let's focus on the boot console case.

Some transfers take place when the thread/lock subsystem is not initialized yet.
But after thread/lock subsystem is able to be used, we are able to use the real 
one.

Let me wrapper them in functions. For the transfers taken place before the 
subsystem
initialization (that's single thread context, no worry about deadlock), it will 
use the
current methods (it might be possible to drop lock due the single thread 
context),
and after the subsystem being initialized, it will use those provided by the 
kernel.

>
> -Mathias
>

Very appreciated for your time.

Regards,
-Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 05/10] usb: dbc: add bulk out and bulk in interfaces

[Mathias Nyman]

On 26.01.2016 14:58, Lu Baolu wrote:

This patch adds interfaces for bulk out and bulk in ops. These
interfaces could be used to implement early printk bootconsole
or hook to various system debuggers.

Signed-off-by: Lu Baolu 
---
  drivers/usb/early/xhci-dbc.c | 373 +++
  include/linux/usb/xhci-dbc.h |  30 
  2 files changed, 403 insertions(+)



...


+
+/*
+ * Check and dispatch events in event ring. It also checks status
+ * of hardware. This function will be called from multiple threads.
+ * An atomic lock is applied to protect the access of event ring.
+ */
+static int xdbc_check_event(void)
+{
+   /* event ring is under checking by other thread? */
+   if (!test_bit(XDBC_ATOMIC_EVENT, >atomic_flags) &&
+   !test_and_set_bit(XDBC_ATOMIC_EVENT,
+   >atomic_flags))
+   return 0;


homemade trylock, can't the real ones be used?


+
+   xdbc_handle_events();
+
+   test_and_clear_bit(XDBC_ATOMIC_EVENT, >atomic_flags);
+
+   return 0;
 +}
+
+#defineBULK_IN_COMPLETED(p)((xdbcp->in_pending == (p)) && \
+xdbcp->in_complete)
+#defineBULK_OUT_COMPLETED(p)   ((xdbcp->out_pending == (p)) && \
+xdbcp->out_complete)
+


...


+}
+
+int xdbc_bulk_read(void *data, int size, int loops)
+{
+   int ret;
+
+   do {
+   if (!test_bit(XDBC_ATOMIC_BULKIN, >atomic_flags) &&
+   !test_and_set_bit(XDBC_ATOMIC_BULKIN,
+   >atomic_flags))
+   break;
+   } while (1);


homemeade spin_lock, can't the real one be used?

If the xdbc_bulk_write() can be accessed from interrupt context (handler, soft, 
timer) it
may deadlock


+
+   ret = xdbc_bulk_transfer(data, size, loops, true);
+
+   test_and_clear_bit(XDBC_ATOMIC_BULKIN, >atomic_flags);
+
+   return ret;
+}
+
+int xdbc_bulk_write(const char *bytes, int size)
+{
+   int ret;
+
+   do {
+   if (!test_bit(XDBC_ATOMIC_BULKOUT, >atomic_flags) &&
+   !test_and_set_bit(XDBC_ATOMIC_BULKOUT,
+   >atomic_flags))
+   break;
+   } while (1);


Another homemeade spin_lock, can't the real one be used?

same issue here, deadlock if accessible from interrupt context.


Would it make sense to have only one spinlock, and start one separate thread for
reading the event ring. The thread would,  lock, handle pending events, unlock,
then call shedule, in a loop. ehci early debug code has some variant of this.

So the lock would be taken while events are being handled.

The same lock would be used for bulk_read and bulk_write. Yes this would 
prevent read and
write at the same time, and the read and writes need to be modified to not 
block until
the reansfer is finished, just to write the TRBs on the ring, update ring 
pointers,
and ring the doorbell.

Or is all this impossibe due to the earlyness of the code?

-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 05/10] usb: dbc: add bulk out and bulk in interfaces

[Lu Baolu]

This patch adds interfaces for bulk out and bulk in ops. These
interfaces could be used to implement early printk bootconsole
or hook to various system debuggers.

Signed-off-by: Lu Baolu 
---
 drivers/usb/early/xhci-dbc.c | 373 +++
 include/linux/usb/xhci-dbc.h |  30 
 2 files changed, 403 insertions(+)

diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index 6855048..f59c80ef 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -219,11 +219,21 @@ static void xdbc_dbg_dump_data(char *str)
xdbc_dbg_dump_string("String Descriptor:");
 }
 
+static void xdbc_dbg_dump_trb(struct xdbc_trb *trb, char *str)
+{
+   xdbc_trace("DBC trb: %s\n", str);
+   xdbc_trace("@%016llx %08x %08x %08x %08x\n", (u64)__pa(trb),
+   le32_to_cpu(trb->field[0]),
+   le32_to_cpu(trb->field[1]),
+   le32_to_cpu(trb->field[2]),
+   le32_to_cpu(trb->field[3]));
+}
 #else
 static inline void xdbc_trace(const char *fmt, ...) { }
 static inline void xdbc_dump_debug_buffer(void) { }
 static inline void xdbc_dbg_dump_regs(char *str) { }
 static inline void xdbc_dbg_dump_data(char *str) { }
+static inline void xdbc_dbg_dump_trb(struct xdbc_trb *trb, char *str) { }
 #endif /* DBC_DEBUG */
 
 /*
@@ -334,6 +344,7 @@ static void *xdbc_get_page(dma_addr_t *dma_addr,
static char in_ring_page[PAGE_SIZE] __aligned(PAGE_SIZE);
static char out_ring_page[PAGE_SIZE] __aligned(PAGE_SIZE);
static char table_page[PAGE_SIZE] __aligned(PAGE_SIZE);
+   static char bulk_buf_page[PAGE_SIZE] __aligned(PAGE_SIZE);
 
switch (type) {
case XDBC_PAGE_EVENT:
@@ -348,6 +359,9 @@ static void *xdbc_get_page(dma_addr_t *dma_addr,
case XDBC_PAGE_TABLE:
virt = (void *)table_page;
break;
+   case XDBC_PAGE_BUFFER:
+   virt = (void *)bulk_buf_page;
+   break;
default:
return NULL;
}
@@ -694,6 +708,12 @@ static int xdbc_mem_init(void)
dev_info = cpu_to_le32((XDBC_DEVICE_REV << 16) | XDBC_PRODUCT_ID);
writel(dev_info, >xdbc_reg->devinfo2);
 
+   /* get and store the transfer buffer */
+   xdbcp->out_buf = xdbc_get_page(>out_dma,
+   XDBC_PAGE_BUFFER);
+   xdbcp->in_buf = xdbcp->out_buf + XDBC_MAX_PACKET;
+   xdbcp->in_dma = xdbcp->out_dma + XDBC_MAX_PACKET;
+
return 0;
 }
 
@@ -789,6 +809,9 @@ static int xdbc_start(void)
 
xdbc_trace("root hub port number %d\n", DCST_DPN(status));
 
+   xdbcp->in_ep_state = EP_RUNNING;
+   xdbcp->out_ep_state = EP_RUNNING;
+
xdbc_trace("DbC is running now, control 0x%08x\n",
readl(>xdbc_reg->control));
 
@@ -882,3 +905,353 @@ int __init early_xdbc_init(char *s)
 
return 0;
 }
+
+static void xdbc_queue_trb(struct xdbc_ring *ring,
+   u32 field1, u32 field2, u32 field3, u32 field4)
+{
+   struct xdbc_trb *trb, *link_trb;
+
+   trb = ring->enqueue;
+   trb->field[0] = cpu_to_le32(field1);
+   trb->field[1] = cpu_to_le32(field2);
+   trb->field[2] = cpu_to_le32(field3);
+   trb->field[3] = cpu_to_le32(field4);
+
+   xdbc_dbg_dump_trb(trb, "enqueue trb");
+
+   ++(ring->enqueue);
+   if (ring->enqueue >= >segment->trbs[TRBS_PER_SEGMENT - 1]) {
+   link_trb = ring->enqueue;
+   if (ring->cycle_state)
+   link_trb->field[3] |= cpu_to_le32(TRB_CYCLE);
+   else
+   link_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
+
+   ring->enqueue = ring->segment->trbs;
+   ring->cycle_state ^= 1;
+   }
+}
+
+static void xdbc_ring_doorbell(int target)
+{
+   writel(DOOR_BELL_TARGET(target), >xdbc_reg->doorbell);
+}
+
+static void xdbc_handle_port_status(struct xdbc_trb *evt_trb)
+{
+   u32 port_reg;
+
+   port_reg = readl(>xdbc_reg->portsc);
+
+   if (port_reg & PORTSC_CSC) {
+   xdbc_trace("%s: connect status change event\n", __func__);
+   writel(port_reg | PORTSC_CSC, >xdbc_reg->portsc);
+   port_reg = readl(>xdbc_reg->portsc);
+   }
+
+   if (port_reg & PORTSC_PRC) {
+   xdbc_trace("%s: port reset change event\n", __func__);
+   writel(port_reg | PORTSC_PRC, >xdbc_reg->portsc);
+   port_reg = readl(>xdbc_reg->portsc);
+   }
+
+   if (port_reg & PORTSC_PLC) {
+   xdbc_trace("%s: port link status change event\n", __func__);
+   writel(port_reg | PORTSC_PLC, >xdbc_reg->portsc);
+   port_reg = readl(>xdbc_reg->portsc);
+   }
+
+   if (port_reg & PORTSC_CEC) {
+   xdbc_trace("%s: config error change\n", __func__);
+   writel(port_reg | PORTSC_CEC,