From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S932535AbdJYCPP (ORCPT <rfc822;w@1wt.eu>);
        Tue, 24 Oct 2017 22:15:15 -0400
Received: from mga05.intel.com ([192.55.52.43]:15747 "EHLO mga05.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S932343AbdJYCPL (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 24 Oct 2017 22:15:11 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.43,430,1503385200"; 
   d="scan'208";a="1209712781"
Subject: Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver
To: Mathias Nyman <mathias.nyman@linux.intel.com>
References: <1504576740-11689-1-git-send-email-baolu.lu@linux.intel.com>
 <1504576740-11689-3-git-send-email-baolu.lu@linux.intel.com>
 <59EF7257.8020801@linux.intel.com>
From: Lu Baolu <baolu.lu@linux.intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Message-ID: <59EFF3AC.5050307@linux.intel.com>
Date: Wed, 25 Oct 2017 10:15:08 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.5.1
MIME-Version: 1.0
In-Reply-To: <59EF7257.8020801@linux.intel.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Mathias,

Thanks for your time.

On 10/25/2017 01:03 AM, Mathias Nyman wrote:
> Hi
>
> Skipping lists, partial review.
> Many of the questions are real questions that I was wondering about
> while lookinga the code. Not necessarily thing that must be changed.

No problem.

>
> On 05.09.2017 04:58, Lu Baolu wrote:
>> xHCI compatible USB host controllers(i.e. super-speed USB3 controllers)
>> can be implemented with the Debug Capability(DbC). It presents a debug
>> device which is fully compliant with the USB framework and provides the
>> equivalent of a very high performance full-duplex serial link. The debug
>> capability operation model and registers interface are defined in 7.6.8
>> of the xHCI specification, revision 1.1.
>>
>> The DbC debug device shares a root port with the xHCI host. By default,
>> the debug capability is disabled and the root port is assigned to xHCI.
>> When the DbC is enabled, the root port will be assigned to the DbC debug
>> device, and the xHCI sees nothing on this port. This implementation uses
>> a sysfs node named <dbc> under the xHCI device to manage the enabling
>> and disabling of the debug capability.
>>
>> When the debug capability is enabled, it will present a debug device
>> through the debug port. This debug device is fully compliant with the
>> USB3 framework, and it can be enumerated by a debug host on the other
>> end of the USB link. As soon as the debug device is configured, a TTY
>> serial device named /dev/ttyDBC0 will be created.
>>
>> One use of this link is running a login service on the debug target.
>> Hence it can be remote accessed by a debug host. Another use case can
>> probably be found in servers. It provides a peer-to-peer USB link
>> between two host-only machines. This provides a reasonable out-of-band
>> communication method between two servers.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd     |   25 +
>>   drivers/usb/host/Kconfig                           |    9 +
>>   drivers/usb/host/Makefile                          |    5 +
>>   drivers/usb/host/xhci-dbgcap.c                     | 1016 ++++++++++++++++++++
>>   drivers/usb/host/xhci-dbgcap.h                     |  247 +++++
>>   drivers/usb/host/xhci-dbgtty.c                     |  586 +++++++++++
>>   drivers/usb/host/xhci-trace.h                      |   60 ++
>>   drivers/usb/host/xhci.c                            |   10 +
>>   drivers/usb/host/xhci.h                            |    1 +
>>   9 files changed, 1959 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>>   create mode 100644 drivers/usb/host/xhci-dbgcap.c
>>   create mode 100644 drivers/usb/host/xhci-dbgcap.h
>>   create mode 100644 drivers/usb/host/xhci-dbgtty.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> new file mode 100644
>> index 0000000..0088aba
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> @@ -0,0 +1,25 @@
>> +What:        /sys/bus/pci/drivers/xhci_hcd/.../dbc
>> +Date:        June 2017
>> +Contact:    Lu Baolu <baolu.lu@linux.intel.com>
>> +Description:
>> +        xHCI compatible USB host controllers (i.e. super-speed
>> +        USB3 controllers) are often implemented with the Debug
>> +        Capability (DbC). It can present a debug device which
>> +        is fully compliant with the USB framework and provides
>> +        the equivalent of a very high performance full-duplex
>> +        serial link for debug purpose.
>> +
>> +        The DbC debug device shares a root port with xHCI host.
>> +        When the DbC is enabled, the root port will be assigned
>> +        to the Debug Capability. Otherwise, it will be assigned
>> +        to xHCI.
>> +
>> +        Writing "enable" to this attribute will enable the DbC
>> +        functionality and the shared root port will be assigned
>> +        to the DbC device. Writing "disable" to this attribute
>> +        will disable the DbC functionality and the shared root
>> +        port will roll back to the xHCI.
>> +
>> +        Reading this attribute gives the state of the DbC. It
>> +        can be one of the following states: disabled, enabled,
>> +        initialized, connected, configured and stalled.
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index fa5692d..968a196 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -27,6 +27,15 @@ config USB_XHCI_HCD
>>         module will be called xhci-hcd.
>>
>>   if USB_XHCI_HCD
>> +config USB_XHCI_DBGCAP
>> +    bool "xHCI support for debug capability"
>> +    depends on TTY
>> +    select USB_U_SERIAL
>> +    ---help---
>> +      Say 'Y' to enable the support for the xHCI debug capability. Make
>> +      sure that your xHCI host supports the extended debug capability and
>> +      you want a TTY serial device based on the xHCI debug capability
>> +      before enabling this option. If unsure, say 'N'.
>>
>>   config USB_XHCI_PCI
>>          tristate
>> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
>> index cf2691f..175c80a 100644
>> --- a/drivers/usb/host/Makefile
>> +++ b/drivers/usb/host/Makefile
>> @@ -13,6 +13,11 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o
>>   xhci-hcd-y := xhci.o xhci-mem.o
>>   xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
>>   xhci-hcd-y += xhci-trace.o
>> +
>> +ifneq ($(CONFIG_USB_XHCI_DBGCAP), )
>> +    xhci-hcd-y += xhci-dbgcap.o xhci-dbgtty.o
>> +endif
>> +
>>   ifneq ($(CONFIG_USB_XHCI_MTK), )
>>       xhci-hcd-y += xhci-mtk-sch.o
>>   endif
>> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
>> new file mode 100644
>> index 0000000..9816085
>> --- /dev/null
>> +++ b/drivers/usb/host/xhci-dbgcap.c
>> @@ -0,0 +1,1016 @@
>> +/**
>> + * xhci-dbgcap.c - xHCI debug capability support
>> + *
>> + * Copyright (C) 2017 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@linux.intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/dma-mapping.h>
>> +#include <linux/slab.h>
>> +
>> +#include "xhci.h"
>> +#include "xhci-trace.h"
>> +#include "xhci-dbgcap.h"
>> +
>> +static inline void *
>> +dbc_dma_alloc_coherent(struct xhci_hcd *xhci, size_t size,
>> +               dma_addr_t *dma_handle, gfp_t flags)
>> +{
>> +    void        *vaddr;
>> +
>> +    vaddr = dma_alloc_coherent(xhci_to_hcd(xhci)->self.sysdev,
>> +                   size, dma_handle, flags);
>> +    memset(vaddr, 0, size);
>> +    return vaddr;
>> +}
>> +
>> +static inline void
>> +dbc_dma_free_coherent(struct xhci_hcd *xhci, size_t size,
>> +              void *cpu_addr, dma_addr_t dma_handle)
>> +{
>> +    if (cpu_addr)
>> +        dma_free_coherent(xhci_to_hcd(xhci)->self.sysdev,
>> +                  size, cpu_addr, dma_handle);
>> +}
>> +
>
> Is there any benefit in having these dbc wrapped variants
> of dma alloc/free coherent functions?
> Can't dma_zalloc_coherent() and dma_free_coherent() be used directly?

The benefit of this wrapper is to zero out the dma space during allocation.

>
>> +static inline void xhci_put_utf16(u16 *s, const char *c)
>> +{
>> +    int        i;
>> +    size_t        size;
>> +
>> +    size = strlen(c);
>> +    for (i = 0; i < size; i++)
>> +        s[i] = cpu_to_le16(c[i]);
>> +}
>> +
>
> could utf8s_to_utf16s() be used instead?
> At least drivers/usb/misc/usb251xb.c is using it to set some string descriptors.

Good suggestion. I will try this in code.

>
>> +static u32 xhci_dbc_populate_strings(struct dbc_strings *strings)
>> +{
>> +    struct usb_string_descriptor    *s_desc;
>> +    u32                string_length;
>> +
>> +    /* Serial string: */
>> +    s_desc = (struct usb_string_descriptor *)strings->serial;
>
> Took me a second to understand that dbc_strings is not just strings, but char
> arrays where we store entire string descriptors with length, type and UTF16LE string.
>
> small thing, but maybe it structure could be called struct dbc_str_descs or similar

Yes, dbc_str_descs is easier for understanding.

>
>> +    xhci_put_utf16(s_desc->wData, DBC_STRING_SERIAL);
>> +
>> +    s_desc->bLength        = (strlen(DBC_STRING_SERIAL) + 1) * 2;
>> +    s_desc->bDescriptorType    = USB_DT_STRING;
>> +    string_length        = s_desc->bLength;
>> +    string_length        <<= 8;
>> +
>> +    /* Product string: */
>> +    s_desc = (struct usb_string_descriptor *)strings->product;
>> +    xhci_put_utf16(s_desc->wData, DBC_STRING_PRODUCT);
>> +
>> +    s_desc->bLength        = (strlen(DBC_STRING_PRODUCT) + 1) * 2;
>> +    s_desc->bDescriptorType    = USB_DT_STRING;
>> +    string_length        += s_desc->bLength;
>> +    string_length        <<= 8;
>> +
>> +    /* Manufacture string: */
>> +    s_desc = (struct usb_string_descriptor *)strings->manufacturer;
>> +    xhci_put_utf16(s_desc->wData, DBC_STRING_MANUFACTURER);
>> +
>> +    s_desc->bLength        = (strlen(DBC_STRING_MANUFACTURER) + 1) * 2;
>> +    s_desc->bDescriptorType    = USB_DT_STRING;
>> +    string_length        += s_desc->bLength;
>> +    string_length        <<= 8;
>> +
>> +    /* String0: */
>> +    strings->string0[0]    = 4;
>> +    strings->string0[1]    = USB_DT_STRING;
>> +    strings->string0[2]    = 0x09;
>> +    strings->string0[3]    = 0x04;
>> +    string_length        += 4;
>> +
>> +    return string_length;
>> +}
>> +
>> +static void xhci_dbc_populate_contexts(struct xhci_hcd *xhci, u32 string_length)
>> +{
>> +    struct xhci_dbc        *dbc;
>> +    struct dbc_info_context    *info;
>> +    struct xhci_ep_ctx    *ep_ctx;
>> +    u32            dev_info;
>> +    dma_addr_t        deq, dma;
>> +    unsigned int        max_burst;
>> +
>> +    dbc = xhci->dbc;
>> +    if (!dbc)
>> +        return;
>> +
>> +    /* Populate info Context: */
>> +    info            = (struct dbc_info_context *)dbc->ctx->bytes;
>> +    dma            = dbc->string_dma;
>> +    info->string0        = cpu_to_le64(dma);
>> +    info->manufacturer    = cpu_to_le64(dma + DBC_MAX_STRING_LENGTH);
>> +    info->product        = cpu_to_le64(dma + DBC_MAX_STRING_LENGTH * 2);
>> +    info->serial        = cpu_to_le64(dma + DBC_MAX_STRING_LENGTH * 3);
>> +    info->length        = cpu_to_le32(string_length);
>> +
>> +    /* Populate bulk out endpoint context: */
>> +    ep_ctx            = dbc_bulkout_ctx(dbc);
>> +    max_burst        = DBC_CTRL_MAXBURST(readl(&dbc->regs->control));
>> +    deq            = dbc_bulkout_enq(dbc);
>> +    ep_ctx->ep_info        = 0;
>> +    ep_ctx->ep_info2    = dbc_epctx_info2(BULK_OUT_EP, 1024, max_burst);
>> +    ep_ctx->deq        = cpu_to_le64(deq | dbc->ring_out->cycle_state);
>> +
>> +    /* Populate bulk in endpoint context: */
>> +    ep_ctx            = dbc_bulkin_ctx(dbc);
>> +    deq            = dbc_bulkin_enq(dbc);
>> +    ep_ctx->ep_info        = 0;
>> +    ep_ctx->ep_info2    = dbc_epctx_info2(BULK_IN_EP, 1024, max_burst);
>> +    ep_ctx->deq        = cpu_to_le64(deq | dbc->ring_in->cycle_state);
>
> Are we setting the ep_ctx->deq to ring deq instead of
> segment start because we are going to call this runtime/mid tranfer?

Ah, that's my fault. I should set it to the segment start although they
have the same values.

This function can't be called during DbC runtime. DbC must be stopped
and reinitialized before re-enabling it.

>
>> +
>> +    /* Set DbC context and info registers: */
>> +    xhci_write_64(xhci, dbc->ctx->dma, &dbc->regs->dccp);
>> +
>> +    dev_info = cpu_to_le32((DBC_VENDOR_ID << 16) | DBC_PROTOCOL);
>> +    writel(dev_info, &dbc->regs->devinfo1);
>> +
>> +    dev_info = cpu_to_le32((DBC_DEVICE_REV << 16) | DBC_PRODUCT_ID);
>> +    writel(dev_info, &dbc->regs->devinfo2);
>
> Minor thing again but this function initializes a couple registers in addition
> to just populating the contexts. Maybe split or rename function

Okay, will clean it.

>
>> +}
>> +
>> +static void xhci_dbc_giveback(struct dbc_request *req, int status)
>> +    __releases(&dbc->lock)
>> +    __acquires(&dbc->lock)
>> +{
>> +    struct dbc_ep        *dep = req->dep;
>> +    struct xhci_dbc        *dbc = dep->dbc;
>> +    struct xhci_hcd        *xhci = dbc->xhci;
>> +    struct device        *dev = xhci_to_hcd(dbc->xhci)->self.sysdev;
>> +
>> +    trace_xhci_dbc_giveback_request(req);
>> +
>> +    list_del_init(&req->list_pending);
>
> why do we need to reinitialize the entry, is this entry reused
> or accessed after later afetr this. would list_del() work?

The requests are preallocated in a pool, so they can be reused.

>
>> +    req->trb_dma = 0;
>> +    req->trb = NULL;
>> +
>> +    if (req->status == -EINPROGRESS)
>> +        req->status = status;
>> +
>> +    dma_unmap_single(dev,
>> +             req->dma,
>> +             req->length,
>> +             dbc_ep_dma_direction(dep));
>> +
>> +    /* Give back the transfer request: */
>> +    spin_unlock(&dbc->lock);
>> +    req->complete(xhci, req);
>> +    spin_lock(&dbc->lock);
>> +}
>> +
>> +static void xhci_dbc_flush_single_request(struct dbc_request *req)
>> +{
>> +    union xhci_trb    *trb = req->trb;
>> +
>> +    trb->generic.field[0]    = 0;
>> +    trb->generic.field[1]    = 0;
>> +    trb->generic.field[2]    = 0;
>> +    trb->generic.field[3]    &= cpu_to_le32(TRB_CYCLE);
>> +    trb->generic.field[3]    |= cpu_to_le32(TRB_TYPE(TRB_TR_NOOP));
>> +
>> +    xhci_dbc_giveback(req, -ESHUTDOWN);
>> +}
>> +
>> +static void xhci_dbc_flush_endpoint_requests(struct dbc_ep *dep)
>> +{
>> +    struct dbc_request    *req, *tmp;
>> +
>> +    list_for_each_entry_safe(req, tmp, &dep->list_pending, list_pending)
>> +        xhci_dbc_flush_single_request(req);
>> +}
>> +
>> +static void xhci_dbc_flush_reqests(struct xhci_dbc *dbc)
>> +{
>> +    int            i;
>> +    struct dbc_ep        *dep;
>> +
>> +    for (i = BULK_OUT; i <= BULK_IN; i++) {
>> +        dep = &dbc->eps[i];
>> +        xhci_dbc_flush_endpoint_requests(dep);
>> +    }
>
> we only have one in and one out endpiont, how about just
>     xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_OUT]);
>     xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_IN]);

Yes, will clean it.

>
>> +}
>> +
>> +static struct dbc_request *
>> +dbc_ep_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags)
>> +{
>> +    struct dbc_request    *req;
>> +
>> +    req = kzalloc(sizeof(*req), gfp_flags);
>> +    if (!req)
>> +        return NULL;
>> +
>> +    req->dep = dep;
>> +    INIT_LIST_HEAD(&req->list_pending);
>> +    INIT_LIST_HEAD(&req->list_pool);
>> +
>> +    trace_xhci_dbc_alloc_request(req);
>> +
>> +    return req;
>> +}
>> +
>> +static void
>> +dbc_ep_free_request(struct dbc_ep *dep, struct dbc_request *req)
>> +{
>> +    trace_xhci_dbc_free_request(req);
>> +
>> +    kfree(req);
>> +}
>> +
>> +static void
>> +xhci_dbc_queue_trb(struct xhci_ring *ring, u32 field1,
>> +           u32 field2, u32 field3, u32 field4)
>> +{
>> +    union xhci_trb        *trb, *next;
>> +
>> +    trb = ring->enqueue;
>> +    trb->generic.field[0]    = cpu_to_le32(field1);
>> +    trb->generic.field[1]    = cpu_to_le32(field2);
>> +    trb->generic.field[2]    = cpu_to_le32(field3);
>> +    trb->generic.field[3]    = cpu_to_le32(field4);
>> +
>> +    trace_xhci_dbc_gadget_ep_queue(ring, &trb->generic);
>> +
>> +    ring->num_trbs_free--;
>> +    next = ++(ring->enqueue);
>> +    if (TRB_TYPE_LINK_LE32(next->link.control)) {
>> +        next->link.control ^= cpu_to_le32(TRB_CYCLE);
>> +        ring->enqueue = ring->enq_seg->trbs;
>> +        ring->cycle_state ^= 1;
>> +    }
>> +}
>> +
>> +static int xhci_dbc_queue_bulk_tx(struct dbc_ep *dep,
>> +                  struct dbc_request *req)
>> +{
>> +    u64            addr;
>> +    union xhci_trb        *trb;
>> +    unsigned int        num_trbs;
>> +    struct xhci_dbc        *dbc = dep->dbc;
>> +    struct xhci_ring    *ring = dep->ring;
>> +    u32            length, control, cycle;
>> +
>> +    num_trbs = count_trbs(req->dma, req->length);
>> +    WARN_ON(num_trbs != 1);
>> +    if (ring->num_trbs_free < num_trbs)
>> +        return -EBUSY;
>> +
>> +    addr    = req->dma;
>> +    trb    = ring->enqueue;
>> +    cycle    = ring->cycle_state;
>> +    length    = TRB_LEN(req->length);
>> +    control    = TRB_TYPE(TRB_NORMAL) | TRB_IOC;
>> +
>> +    if (cycle)
>> +        control &= cpu_to_le32(~TRB_CYCLE);
>> +    else
>> +        control |= cpu_to_le32(TRB_CYCLE);
>> +
>> +    req->trb = ring->enqueue;
>> +    req->trb_dma = xhci_trb_virt_to_dma(ring->enq_seg, ring->enqueue);
>> +    xhci_dbc_queue_trb(ring,
>> +               lower_32_bits(addr),
>> +               upper_32_bits(addr),
>> +               length, control);
>> +
>> +    /*
>> +     * Add a barrier between writes of trb fields and flipping
>> +     * the cycle bit:
>> +     */
>> +    wmb();
>> +
>> +    if (cycle)
>> +        trb->generic.field[3] |= cpu_to_le32(TRB_CYCLE);
>> +    else
>> +        trb->generic.field[3] &= cpu_to_le32(~TRB_CYCLE);
>> +
>> +    writel(DBC_DOOR_BELL_TARGET(dep->direction), &dbc->regs->doorbell);
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +dbc_ep_do_queue(struct dbc_ep *dep, struct dbc_request *req)
>> +{
>> +    int            ret;
>> +    struct device        *dev;
>> +    struct xhci_dbc        *dbc = dep->dbc;
>> +    struct xhci_hcd        *xhci = dbc->xhci;
>> +
>> +    dev = xhci_to_hcd(xhci)->self.sysdev;
>> +
>> +    if (!req->length || !req->buf)
>> +        return -EINVAL;
>> +
>> +    req->actual        = 0;
>> +    req->status        = -EINPROGRESS;
>> +
>> +    req->dma = dma_map_single(dev,
>> +                  req->buf,
>> +                  req->length,
>> +                  dbc_ep_dma_direction(dep));
>> +    if (dma_mapping_error(dev, req->dma)) {
>> +        xhci_err(xhci, "failed to map buffer\n");
>> +        return -EFAULT;
>> +    }
>> +
>> +    ret = xhci_dbc_queue_bulk_tx(dep, req);
>> +    if (ret) {
>> +        xhci_err(xhci, "failed to queue trbs\n");
>> +        dma_unmap_single(dev,
>> +                 req->dma,
>> +                 req->length,
>> +                 dbc_ep_dma_direction(dep));
>> +        return -EFAULT;
>> +    }
>> +
>> +    list_add_tail(&req->list_pending, &dep->list_pending);
>> +
>> +    return 0;
>> +}
>> +
>> +static int dbc_ep_queue(struct dbc_ep *dep,
>> +            struct dbc_request *req,
>> +            gfp_t gfp_flags)
>> +{
>> +    struct xhci_dbc        *dbc = dep->dbc;
>> +    int            ret = -ESHUTDOWN;
>> +
>> +    spin_lock(&dbc->lock);
>> +    if (dbc->state == DS_CONFIGURED)
>> +        ret = dbc_ep_do_queue(dep, req);
>> +    spin_unlock(&dbc->lock);
>> +
>> +    trace_xhci_dbc_queue_request(req);
>> +
>> +    return ret;
>> +}
>> +
>> +static inline void xhci_dbc_do_eps_init(struct xhci_hcd *xhci, bool direction)
>> +{
>> +    struct dbc_ep        *dep;
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    dep            = &dbc->eps[direction];
>> +    dep->dbc        = dbc;
>> +    dep->direction        = direction;
>> +    dep->ring        = direction ? dbc->ring_in : dbc->ring_out;
>> +    dep->alloc_request    = dbc_ep_alloc_request;
>> +    dep->free_request    = dbc_ep_free_request;
>> +    dep->queue        = dbc_ep_queue;
>
> why do we need the alloc_request, free_request and queue function pointers in struct dbc_ep?
> They are always pointing to the same functions.

I did this in order to make DbC base driver and upper glue layer independent.
It's unnecessary now. I will re-factor this code to make it simple.

>
>> +
>> +    INIT_LIST_HEAD(&dep->list_pending);
>> +}
>> +
>> +static void xhci_dbc_eps_init(struct xhci_hcd *xhci)
>> +{
>> +    xhci_dbc_do_eps_init(xhci, BULK_OUT);
>> +    xhci_dbc_do_eps_init(xhci, BULK_IN);
>> +}
>> +
>> +static void xhci_dbc_eps_exit(struct xhci_hcd *xhci)
>> +{
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    memset(dbc->eps, 0, ARRAY_SIZE(dbc->eps));
>> +}
>> +
>> +static int xhci_dbc_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>> +{
>> +    int            ret;
>> +    dma_addr_t        deq;
>> +    u32            string_length;
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    /* Allocate various rings for events and transfers: */
>> +    dbc->ring_evt = xhci_ring_alloc(xhci, 1, 1, TYPE_EVENT, 0, flags);
>> +    if (!dbc->ring_evt)
>> +        goto evt_fail;
>> +
>> +    dbc->ring_in = xhci_ring_alloc(xhci, 1, 1, TYPE_BULK, 0, flags);
>> +    if (!dbc->ring_in)
>> +        goto in_fail;
>> +
>> +    dbc->ring_out = xhci_ring_alloc(xhci, 1, 1, TYPE_BULK, 0, flags);
>> +    if (!dbc->ring_out)
>> +        goto out_fail;
>> +
>> +    /* Allocate and populate ERST: */
>> +    ret = xhci_alloc_erst(xhci, dbc->ring_evt, &dbc->erst, flags);
>> +    if (ret)
>> +        goto erst_fail;
>> +
>> +    /* Allocate context data structure: */
>> +    dbc->ctx = xhci_alloc_container_ctx(xhci, XHCI_CTX_TYPE_DEVICE, flags);
>> +    if (!dbc->ctx)
>> +        goto ctx_fail;
>> +
>> +    /* Allocate the string table: */
>> +    dbc->string_size = sizeof(struct dbc_strings);
>> +    dbc->string = dbc_dma_alloc_coherent(xhci,
>> +                         dbc->string_size,
>> +                         &dbc->string_dma,
>> +                         flags);
>> +    if (!dbc->string)
>> +        goto string_fail;
>> +
>> +    /* Setup ERST register: */
>> +    writel(dbc->erst.erst_size, &dbc->regs->ersts);
>> +    xhci_write_64(xhci, dbc->erst.erst_dma_addr, &dbc->regs->erstba);
>> +    deq = xhci_trb_virt_to_dma(dbc->ring_evt->deq_seg,
>> +                   dbc->ring_evt->dequeue);
>> +    xhci_write_64(xhci, deq, &dbc->regs->erdp);
>> +
>> +    /* Setup strings and contexts: */
>> +    string_length = xhci_dbc_populate_strings(dbc->string);
>> +    xhci_dbc_populate_contexts(xhci, string_length);
>> +
>> +    mmiowb();
>> +
>> +    xhci_dbc_eps_init(xhci);
>> +    dbc->state = DS_INITIALIZED;
>> +
>> +    return 0;
>> +
>> +string_fail:
>> +    xhci_free_container_ctx(xhci, dbc->ctx);
>> +    dbc->ctx = NULL;
>> +ctx_fail:
>> +    xhci_free_erst(xhci, &dbc->erst);
>> +erst_fail:
>> +    xhci_ring_free(xhci, dbc->ring_out);
>> +    dbc->ring_out = NULL;
>> +out_fail:
>> +    xhci_ring_free(xhci, dbc->ring_in);
>> +    dbc->ring_in = NULL;
>> +in_fail:
>> +    xhci_ring_free(xhci, dbc->ring_evt);
>> +    dbc->ring_evt = NULL;
>> +evt_fail:
>> +    return -ENOMEM;
>> +}
>> +
>> +static void xhci_dbc_mem_cleanup(struct xhci_hcd *xhci)
>> +{
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    if (!dbc)
>> +        return;
>> +
>> +    xhci_dbc_eps_exit(xhci);
>> +
>> +    if (dbc->string) {
>> +        dbc_dma_free_coherent(xhci,
>> +                      dbc->string_size,
>> +                      dbc->string, dbc->string_dma);
>> +        dbc->string = NULL;
>> +    }
>> +
>> +    xhci_free_container_ctx(xhci, dbc->ctx);
>> +    dbc->ctx = NULL;
>> +
>> +    xhci_free_erst(xhci, &dbc->erst);
>> +    xhci_ring_free(xhci, dbc->ring_out);
>> +    xhci_ring_free(xhci, dbc->ring_in);
>> +    xhci_ring_free(xhci, dbc->ring_evt);
>> +    dbc->ring_in = NULL;
>> +    dbc->ring_out = NULL;
>> +    dbc->ring_evt = NULL;
>> +}
>> +
>> +static void xhci_dbc_reset_debug_port(struct xhci_hcd *xhci)
>> +{
>> +    u32            val;
>> +    unsigned long        flags;
>> +    int            port_index;
>> +    __le32 __iomem        **port_array;
>> +
>> +    spin_lock_irqsave(&xhci->lock, flags);
>> +
>> +    port_index = xhci->num_usb3_ports;
>> +    port_array = xhci->usb3_ports;
>> +    if (port_index <= 0) {
>> +        spin_unlock_irqrestore(&xhci->lock, flags);
>> +        return;
>> +    }
>
> we could take the spin lock here, and avoid the above unlock case.

Yes, will re-factor it.

>
>> +
>> +    while (port_index--) {
>> +        val = readl(port_array[port_index]);
>> +        if (!(val & PORT_CONNECT))
>> +            writel(val | PORT_RESET, port_array[port_index]);
>> +    }
>
> This resets every USB3 port that does not have a device connected.
> Shouldn't the debug port be the first USB3 port in the root hub, or do I remeber wrong?
> That should the be xhci->usb3_ports[0]. Can't we reset just that one?

Yes. I will revisit this function. I even think that we don't need it at all.

>
>> +
>> +    spin_unlock_irqrestore(&xhci->lock, flags);
>> +}
>> +
>> +static int xhci_do_dbc_start(struct xhci_hcd *xhci)
>> +{
>> +    int            ret;
>> +    u32            ctrl;
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    if (dbc->state != DS_DISABLED)
>> +        return -EINVAL;
>> +
>> +    writel(0, &dbc->regs->control);
>> +    ret = xhci_handshake(&dbc->regs->control,
>> +                 DBC_CTRL_DBC_ENABLE,
>> +                 0, 1000);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = xhci_dbc_mem_init(xhci, GFP_ATOMIC);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ctrl = readl(&dbc->regs->control);
>> +    writel(ctrl | DBC_CTRL_DBC_ENABLE | DBC_CTRL_PORT_ENABLE,
>> +           &dbc->regs->control);
>> +    ret = xhci_handshake(&dbc->regs->control,
>> +                 DBC_CTRL_DBC_ENABLE,
>> +                 DBC_CTRL_DBC_ENABLE, 1000);
>> +    if (ret)
>> +        return ret;
>> +
>> +    xhci_dbc_reset_debug_port(xhci);
>> +    dbc->state = DS_ENABLED;
>> +
>> +    return 0;
>> +}
>> +
>> +static void xhci_do_dbc_stop(struct xhci_hcd *xhci)
>> +{
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    if (dbc->state == DS_DISABLED)
>> +        return;
>> +
>> +    writel(0, &dbc->regs->control);
>> +    xhci_dbc_mem_cleanup(xhci);
>> +    dbc->state = DS_DISABLED;
>> +}
>> +
>> +static int xhci_dbc_start(struct xhci_hcd *xhci)
>> +{
>> +    int            ret;
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    WARN_ON(!dbc);
>> +
>> +    spin_lock(&dbc->lock);
>
> Maybe call the pm_runtime_get_sync() already here to prevent runtime pm from
> stopping the controller while starting dbc
>
> we then need to pm_runtme_put it do_dbc_start() fails

Yes. Great! We should avoid host suspending while starting DbC.

>
>> +    ret = xhci_do_dbc_start(xhci);
>> +    spin_unlock(&dbc->lock);
>> +
>> +    if (!ret) {
>> +        pm_runtime_get_sync(xhci_to_hcd(xhci)->self.controller);
>> +        ret = mod_delayed_work(system_wq, &dbc->event_work, 1);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void xhci_dbc_stop(struct xhci_hcd *xhci)
>> +{
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +    struct dbc_port        *port = &dbc->port;
>> +
>> +    WARN_ON(!dbc);
>> +
>> +    cancel_delayed_work_sync(&dbc->event_work);
>> +
>> +    if (port->registered)
>> +        xhci_dbc_tty_unregister_device(xhci);
>> +
>> +    spin_lock(&dbc->lock);
>> +    xhci_do_dbc_stop(xhci);
>> +    spin_unlock(&dbc->lock);
>> +
>> +    pm_runtime_put_sync(xhci_to_hcd(xhci)->self.controller);
>> +}
>> +
>> +static void
>> +dbc_handle_port_status(struct xhci_hcd *xhci, union xhci_trb *event)
>> +{
>> +    u32            portsc;
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    portsc = readl(&dbc->regs->portsc);
>> +    if (portsc & DBC_PORTSC_CONN_CHANGE)
>> +        xhci_info(xhci, "DbC port connect change\n");
>> +
>> +    if (portsc & DBC_PORTSC_RESET_CHANGE)
>> +        xhci_info(xhci, "DbC port reset change\n");
>> +
>> +    if (portsc & DBC_PORTSC_LINK_CHANGE)
>> +        xhci_info(xhci, "DbC port link status change\n");
>> +
>> +    if (portsc & DBC_PORTSC_CONFIG_CHANGE)
>> +        xhci_info(xhci, "DbC config error change\n");
>> +
>
> Are these messages info or debug?

For information. Users could read this in dmesg when they
connect DbC to a host.

> If these messages are the only messages seen when connecting a
> debug host (and not constantly spamming dmesg) then they are ok.

There are no spam messages. They only print when connecting to a
debug host.

>
> A bit like usb core annoncing "new usb device detected"

Yes.

>
>> +    /* Port reset change bit will be cleared in other place: */
>> +    writel(portsc & ~DBC_PORTSC_RESET_CHANGE, &dbc->regs->portsc);
>> +}
>> +
>> +static void dbc_handle_tx_event(struct xhci_hcd *xhci, union xhci_trb *event)
>
> Ah, here you have the opportunity to properly name this function handle_xfer_event() or
> handle_transfer_event().
>
> The handle_tx_event() we use in xhci is confusing because of the tx/rx association.

Yes, I will clean it.

>
>> +{
>> +    struct dbc_ep        *dep;
>> +    struct xhci_ring    *ring;
>> +    int            ep_id;
>> +    int            status;
>> +    u32            comp_code;
>> +    size_t            remain_length;
>> +    struct dbc_request    *req = NULL, *r;
>> +
>> +    comp_code    = GET_COMP_CODE(le32_to_cpu(event->generic.field[2]));
>> +    remain_length    = EVENT_TRB_LEN(le32_to_cpu(event->generic.field[2]));
>> +    ep_id        = TRB_TO_EP_ID(le32_to_cpu(event->generic.field[3]));
>> +    dep        = (ep_id == EPID_OUT) ?
>> +                get_out_ep(xhci) : get_in_ep(xhci);
>> +    ring        = dep->ring;
>> +
>> +    switch (comp_code) {
>> +    case COMP_SUCCESS:
>> +        remain_length = 0;
>> +    /* FALLTHROUGH */
>> +    case COMP_SHORT_PACKET:
>> +        status = 0;
>> +        break;
>> +    case COMP_TRB_ERROR:
>> +    case COMP_BABBLE_DETECTED_ERROR:
>> +    case COMP_USB_TRANSACTION_ERROR:
>> +    case COMP_STALL_ERROR:
>> +        xhci_warn(xhci, "tx error %d detected\n", comp_code);
>> +        status = -comp_code;
>> +        break;
>> +    default:
>> +        xhci_err(xhci, "unknown tx error %d\n", comp_code);
>> +        status = -comp_code;
>> +        break;
>> +    }
>> +
>> +    /* Match the pending request: */
>> +    list_for_each_entry(r, &dep->list_pending, list_pending) {
>> +        if (r->trb_dma == event->trans_event.buffer) {
>> +            req = r;
>> +            break;
>> +        }
>> +    }
>
> I like that it matches the request even if they are out of order.
>
> Just out of curiosiry, was there some issue with transfer event not always
> matching the next entry in dep->list_pending ?

I didn't catch such issue. The only possibilities are 1) the dep->list_pending
is not listed in the same order as trbs in ring; 2) transfer error happens and
the event trb doesn't match the next entry well. Anyway, the conservative
way is not always considering only the next entry.

>
>> +
>> +    if (!req) {
>> +        xhci_warn(xhci, "no matched request\n");
>> +        return;
>> +    }
>> +
>> +    trace_xhci_dbc_handle_transfer(ring, &req->trb->generic);
>> +
>> +    ring->num_trbs_free++;
>> +    req->actual = req->length - remain_length;
>> +    xhci_dbc_giveback(req, status);
>> +}
>> +
>> +static enum evtreturn xhci_dbc_do_handle_events(struct xhci_dbc *dbc)
>> +{
>> +    dma_addr_t        deq;
>> +    struct dbc_ep        *dep;
>> +    union xhci_trb        *evt;
>> +    u32            ctrl, portsc;
>> +    struct xhci_hcd        *xhci = dbc->xhci;
>> +    bool            update_erdp = false;
>> +
>> +    /* DbC state machine: */
>> +    switch (dbc->state) {
>> +    case DS_DISABLED:
>> +    case DS_INITIALIZED:
>> +
>> +        return EVT_ERR;
>> +    case DS_ENABLED:
>> +        portsc = readl(&dbc->regs->portsc);
>> +        if (portsc & DBC_PORTSC_CONN_STATUS) {
>> +            dbc->state = DS_CONNECTED;
>> +            xhci_info(xhci, "DbC connected\n");
>> +        }
>> +
>> +        return EVT_DONE;
>> +    case DS_CONNECTED:
>> +        ctrl = readl(&dbc->regs->control);
>> +        if (ctrl & DBC_CTRL_DBC_RUN) {
>> +            dbc->state = DS_CONFIGURED;
>> +            xhci_info(xhci, "DbC configured\n");
>> +            portsc = readl(&dbc->regs->portsc);
>> +            writel(portsc, &dbc->regs->portsc);
>> +            return EVT_GSER;
>> +        }
>> +
>> +        return EVT_DONE;
>> +    case DS_CONFIGURED:
>> +        /* Handle cable unplug event: */
>> +        portsc = readl(&dbc->regs->portsc);
>> +        if (!(portsc & DBC_PORTSC_PORT_ENABLED) &&
>> +            !(portsc & DBC_PORTSC_CONN_STATUS)) {
>> +            xhci_info(xhci, "DbC cable unplugged\n");
>> +            dbc->state = DS_ENABLED;
>> +            xhci_dbc_flush_reqests(dbc);
>> +
>> +            return EVT_DISC;
>> +        }
>> +
>> +        /* Handle debug port reset event: */
>> +        if (portsc & DBC_PORTSC_RESET_CHANGE) {
>> +            xhci_info(xhci, "DbC port reset\n");
>> +            writel(portsc, &dbc->regs->portsc);
>> +            dbc->state = DS_ENABLED;
>> +            xhci_dbc_flush_reqests(dbc);
>> +
>> +            return EVT_DISC;
>> +        }
>> +
>> +        /* Handle endpoint stall event: */
>> +        ctrl = readl(&dbc->regs->control);
>> +        if ((ctrl & DBC_CTRL_HALT_IN_TR) ||
>> +            (ctrl & DBC_CTRL_HALT_OUT_TR)) {
>> +            xhci_info(xhci, "DbC Endpoint stall\n");
>> +            dbc->state = DS_STALLED;
>> +
>> +            if (ctrl & DBC_CTRL_HALT_IN_TR) {
>> +                dep = get_in_ep(xhci);
>> +                xhci_dbc_flush_endpoint_requests(dep);
>> +            }
>> +
>> +            if (ctrl & DBC_CTRL_HALT_OUT_TR) {
>> +                dep = get_out_ep(xhci);
>> +                xhci_dbc_flush_endpoint_requests(dep);
>> +            }
>> +
>> +            return EVT_DONE;
>> +        }
>> +
>> +        /* Clear DbC run change bit: */
>> +        if (ctrl & DBC_CTRL_DBC_RUN_CHANGE) {
>> +            writel(ctrl, &dbc->regs->control);
>> +            ctrl = readl(&dbc->regs->control);
>> +        }
>> +
>> +        break;
>> +    case DS_STALLED:
>> +        ctrl = readl(&dbc->regs->control);
>> +        if (!(ctrl & DBC_CTRL_HALT_IN_TR) &&
>> +            !(ctrl & DBC_CTRL_HALT_OUT_TR) &&
>> +            (ctrl & DBC_CTRL_DBC_RUN)) {
>> +            dbc->state = DS_CONFIGURED;
>> +            break;
>> +        }
>> +
>> +        return EVT_DONE;
>> +    default:
>> +        xhci_err(xhci, "Unknown DbC state %d\n", dbc->state);
>> +        break;
>> +    }
>> +
>> +    /* Handle the events in the event ring: */
>> +    evt = dbc->ring_evt->dequeue;
>> +    while ((le32_to_cpu(evt->event_cmd.flags) & TRB_CYCLE) ==
>> +            dbc->ring_evt->cycle_state) {
>> +        /*
>> +         * Add a barrier between reading the cycle flag and any
>> +         * reads of the event's flags/data below:
>> +         */
>> +        rmb();
>> +
>> +        trace_xhci_dbc_handle_event(dbc->ring_evt, &evt->generic);
>> +
>> +        switch (le32_to_cpu(evt->event_cmd.flags) & TRB_TYPE_BITMASK) {
>> +        case TRB_TYPE(TRB_PORT_STATUS):
>> +            dbc_handle_port_status(xhci, evt);
>> +            break;
>> +        case TRB_TYPE(TRB_TRANSFER):
>> +            dbc_handle_tx_event(xhci, evt);
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +
>> +        inc_deq(xhci, dbc->ring_evt);
>> +        evt = dbc->ring_evt->dequeue;
>> +        update_erdp = true;
>> +    }
>> +
>> +    /* Update event ring dequeue pointer: */
>> +    if (update_erdp) {
>> +        deq = xhci_trb_virt_to_dma(dbc->ring_evt->deq_seg,
>> +                       dbc->ring_evt->dequeue);
>> +        xhci_write_64(xhci, deq, &dbc->regs->erdp);
>> +    }
>> +
>> +    return EVT_DONE;
>> +}
>> +
>> +static void xhci_dbc_handle_events(struct work_struct *work)
>> +{
>> +    int            ret;
>> +    enum evtreturn        evtr;
>> +    struct xhci_dbc        *dbc;
>> +    struct xhci_hcd        *xhci;
>> +
>> +    dbc = container_of(to_delayed_work(work), struct xhci_dbc, event_work);
>> +    xhci = dbc->xhci;
>> +
>> +    spin_lock(&dbc->lock);
>> +    evtr = xhci_dbc_do_handle_events(dbc);
>> +    spin_unlock(&dbc->lock);
>> +
>> +    switch (evtr) {
>> +    case EVT_GSER:
>> +        ret = xhci_dbc_tty_register_device(xhci);
>> +        if (ret) {
>> +            xhci_err(xhci, "failed to alloc tty device\n");
>> +            break;
>> +        }
>> +
>> +        xhci_info(xhci, "DbC now attached to /dev/ttyDBC0\n");
>> +        mod_delayed_work(system_wq, &dbc->event_work, 1);
>> +        break;
>> +    case EVT_DISC:
>> +        xhci_dbc_tty_unregister_device(xhci);
>> +        mod_delayed_work(system_wq, &dbc->event_work, 1);
>> +        break;
>> +    case EVT_DONE:
>> +        mod_delayed_work(system_wq, &dbc->event_work, 1);
>> +        break;
>> +    default:
>> +        xhci_info(xhci, "stop handling dbc events\n");
>
>     return; here
>
>> +    }
>
> mod_delayed_work(); here, and remove it from above cases.

Yes, will clean it.

>
>> +}
>> +
>> +static void xhci_dbc_exit(struct xhci_hcd *xhci)
>> +{
>> +    unsigned long        flags;
>> +
>> +    spin_lock_irqsave(&xhci->lock, flags);
>> +    kfree(xhci->dbc);
>> +    xhci->dbc = NULL;
>> +    spin_unlock_irqrestore(&xhci->lock, flags);
>> +}
>> +
>> +static int xhci_dbc_init(struct xhci_hcd *xhci)
>> +{
>> +    u32            reg;
>> +    struct xhci_dbc        *dbc;
>> +    unsigned long        flags;
>> +    void __iomem        *base;
>> +    int            dbc_cap_offs;
>> +
>> +    base = &xhci->cap_regs->hc_capbase;
>> +    dbc_cap_offs = xhci_find_next_ext_cap(base, 0, XHCI_EXT_CAPS_DEBUG);
>> +    if (!dbc_cap_offs)
>> +        return -ENODEV;
>> +
>> +    dbc = kzalloc(sizeof(*dbc), GFP_KERNEL);
>> +    if (!dbc)
>> +        return -ENOMEM;
>> +
>> +    dbc->regs = base + dbc_cap_offs;
>> +
>> +    /* We will avoid using DbC in xhci driver if it's in use. */
>> +    reg = readl(&dbc->regs->control);
>> +    if (reg & DBC_CTRL_DBC_ENABLE) {
>> +        kfree(dbc);
>> +        return -EBUSY;
>> +    }
>> +
>> +    spin_lock_irqsave(&xhci->lock, flags);
>> +    if (xhci->dbc) {
>> +        spin_unlock_irqrestore(&xhci->lock, flags);
>> +        kfree(dbc);
>> +        return -EBUSY;
>> +    }
>> +    xhci->dbc = dbc;
>> +    spin_unlock_irqrestore(&xhci->lock, flags);
>> +
>> +    dbc->xhci = xhci;
>> +    INIT_DELAYED_WORK(&dbc->event_work, xhci_dbc_handle_events);
>> +    spin_lock_init(&dbc->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static ssize_t dbc_show(struct device *dev,
>> +            struct device_attribute *attr,
>> +            char *buf)
>> +{
>> +    const char        *p;
>> +    struct xhci_dbc        *dbc;
>> +    struct xhci_hcd        *xhci;
>> +
>> +    xhci = hcd_to_xhci(dev_get_drvdata(dev));
>> +    dbc = xhci->dbc;
>> +
>> +    switch (dbc->state) {
>> +    case DS_DISABLED:
>> +        p = "disabled";
>> +        break;
>> +    case DS_INITIALIZED:
>> +        p = "initialized";
>> +        break;
>> +    case DS_ENABLED:
>> +        p = "enabled";
>> +        break;
>> +    case DS_CONNECTED:
>> +        p = "connected";
>> +        break;
>> +    case DS_CONFIGURED:
>> +        p = "configured";
>> +        break;
>> +    case DS_STALLED:
>> +        p = "stalled";
>> +        break;
>> +    default:
>> +        p = "unknown";
>> +    }
>> +
>> +    return sprintf(buf, "%s\n", p);
>> +}
>> +
>> +static ssize_t dbc_store(struct device *dev,
>> +             struct device_attribute *attr,
>> +             const char *buf, size_t count)
>> +{
>> +    struct xhci_dbc        *dbc;
>> +    struct xhci_hcd        *xhci;
>> +
>> +    xhci = hcd_to_xhci(dev_get_drvdata(dev));
>> +    dbc = xhci->dbc;
>> +
>> +    if (!strncmp(buf, "enable", 6))
>> +        xhci_dbc_start(xhci);
>> +    else if (!strncmp(buf, "disable", 7))
>> +        xhci_dbc_stop(xhci);
>> +    else
>> +        return -EINVAL;
>> +
>> +    return count;
>> +}
>> +
>> +static DEVICE_ATTR(dbc, 0644, dbc_show, dbc_store);
>> +
>> +int dbc_create_sysfs_file(struct xhci_hcd *xhci)
>> +{
>> +    int            ret;
>> +    struct device        *dev = xhci_to_hcd(xhci)->self.controller;
>> +
>> +    ret = xhci_dbc_init(xhci);
>> +    if (ret)
>> +        return ret;
>> +
>> +    xhci_dbc_tty_register_driver(xhci);
>
> The function name "dbc_create_sysfs_file()" doesn't reveal anything about
> that is also registers the tty driver

Yes. Maybe xhci_dbc_init/exit() sounds better.

I will refine it.

>
>> +
>> +    return device_create_file(dev, &dev_attr_dbc);
>> +}
>> +
>> +void dbc_remove_sysfs_file(struct xhci_hcd *xhci)
>> +{
>> +    struct device        *dev = xhci_to_hcd(xhci)->self.controller;
>> +
>> +    device_remove_file(dev, &dev_attr_dbc);
>> +    xhci_dbc_tty_unregister_driver();
>> +    xhci_dbc_stop(xhci);
>> +    xhci_dbc_exit(xhci);
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +int xhci_dbc_suspend(struct xhci_hcd *xhci)
>> +{
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    if (!dbc)
>> +        return 0;
>> +
>> +    if (dbc->state == DS_CONFIGURED)
>> +        dbc->resume_required = 1;
>> +
>> +    xhci_dbc_stop(xhci);
>> +
>> +    return 0;
>> +}
>> +
>> +int xhci_dbc_resume(struct xhci_hcd *xhci)
>> +{
>> +    int            ret = 0;
>> +    struct xhci_dbc        *dbc = xhci->dbc;
>> +
>> +    if (!dbc)
>> +        return 0;
>> +
>> +    if (dbc->resume_required) {
>> +        dbc->resume_required = 0;
>> +        xhci_dbc_start(xhci);
>> +    }
>> +
>> +    return ret;
>
> So far the dbc driver looks really good, it's getting late,
> I need to reviewing the other files later.
>
> I can't say much about the TTY parts as I never needed to work on those
>
> -Mathias

Thanks for your time. I will change the code according to your comments.
The new version(v4) will come out after 4.15-rc1 is out.

Best regards,
Lu Baolu