Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221110080006.3563429-1-albertccwang@google.com>
 <20221110080006.3563429-4-albertccwang@google.com> <Y2yzg2v2AL6MsKvy@kroah.com>
In-Reply-To: <Y2yzg2v2AL6MsKvy@kroah.com>
From:   Albert Wang <albertccwang@google.com>
Date:   Thu, 24 Nov 2022 14:47:22 +0800
Message-ID: <CANqn-rj++p_rSkZxa5rpRXQ-9or-_18VzaE_M1vjq4aVNsrAKg@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] usb: host: add the xhci offload hooks implementations
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     mathias.nyman@intel.com, badhri@google.com, howardyen@google.com,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Nov 10, 2022 at 4:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 10, 2022 at 04:00:06PM +0800, Albert Wang wrote:
> > Add the offload hooks implementations which are used in the xHCI driver
> > for vendor offload function, and some functions will call to
> > co-processor driver for further offload operations.
>
> Where is the users for these hooks?  We can't add code that doesn't have
> users as stated before.
>
> > Signed-off-by: Albert Wang <albertccwang@google.com>
> > Signed-off-by: Howard Yen <howardyen@google.com>
> > ---
> > Changes in v2:
> > - New in v2
> >
> >  drivers/usb/host/xhci-offload-impl.c | 492 +++++++++++++++++++++++++++
> >  1 file changed, 492 insertions(+)
> >  create mode 100644 drivers/usb/host/xhci-offload-impl.c
> >
> > diff --git a/drivers/usb/host/xhci-offload-impl.c b/drivers/usb/host/xhci-offload-impl.c
> > new file mode 100644
> > index 000000000000..90e546d63fbe
> > --- /dev/null
> > +++ b/drivers/usb/host/xhci-offload-impl.c
> > @@ -0,0 +1,492 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 Google Corp.
>
> I don't think it's 2020 anymore :)

Thanks, I will correct it.

>
> > + *
> > + * Author:
> > + *  Howard.Yen <howardyen@google.com>
> > + */
> > +
> > +#include <linux/dmapool.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/of.h>
> > +#include <linux/of_reserved_mem.h>
> > +#include <linux/pm_wakeup.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/usb/hcd.h>
> > +
> > +#include "xhci.h"
> > +#include "xhci-plat.h"
> > +
> > +enum usb_offload_op_mode {
> > +     USB_OFFLOAD_STOP,
> > +     USB_OFFLOAD_DRAM
> > +};
> > +
> > +enum usb_state {
> > +     USB_DISCONNECTED,
> > +     USB_CONNECTED
> > +};
> > +
> > +enum usb_offload_msg {
> > +     SET_DCBAA_PTR,
> > +     SETUP_DONE,
> > +     SET_ISOC_TR_INFO,
> > +     SYNC_CONN_STAT,
> > +     SET_OFFLOAD_STATE
> > +};
> > +
> > +struct conn_stat_args {
> > +     u16 bus_id;
> > +     u16 dev_num;
> > +     u16 slot_id;
> > +     u32 conn_stat;
> > +};
> > +
> > +struct get_isoc_tr_info_args {
> > +     u16 ep_id;
> > +     u16 dir;
> > +     u32 type;
> > +     u32 num_segs;
> > +     u32 seg_ptr;
> > +     u32 max_packet;
> > +     u32 deq_ptr;
> > +     u32 enq_ptr;
> > +     u32 cycle_state;
> > +     u32 num_trbs_free;
> > +};
> > +
> > +struct xhci_offload_data {
> > +     struct xhci_hcd *xhci;
> > +
> > +     bool usb_accessory_enabled;
> > +     bool usb_audio_offload;
> > +     bool dt_direct_usb_access;
> > +     bool offload_state;
> > +
> > +     enum usb_offload_op_mode op_mode;
> > +};
> > +
> > +static struct xhci_offload_data *offload_data;
> > +struct xhci_offload_data *xhci_get_offload_data(void)
> > +{
> > +     return offload_data;
> > +}
> > +
> > +/*
> > + * Determine if an USB device is a compatible devices:
> > + *     True: Devices are audio class and they contain ISOC endpoint
> > + *    False: Devices are not audio class or they're audio class but no ISOC endpoint or
> > + *           they have at least one interface is video class
> > + */
> > +static bool is_compatible_with_usb_audio_offload(struct usb_device *udev)
> > +{
> > +     struct usb_endpoint_descriptor *epd;
> > +     struct usb_host_config *config;
> > +     struct usb_host_interface *alt;
> > +     struct usb_interface_cache *intfc;
> > +     int i, j, k;
> > +     bool is_audio = false;
> > +
> > +     config = udev->config;
> > +     for (i = 0; i < config->desc.bNumInterfaces; i++) {
> > +             intfc = config->intf_cache[i];
> > +             for (j = 0; j < intfc->num_altsetting; j++) {
> > +                     alt = &intfc->altsetting[j];
> > +
> > +                     if (alt->desc.bInterfaceClass == USB_CLASS_VIDEO) {
> > +                             is_audio = false;
> > +                             goto out;
> > +                     }
> > +
> > +                     if (alt->desc.bInterfaceClass == USB_CLASS_AUDIO) {
> > +                             for (k = 0; k < alt->desc.bNumEndpoints; k++) {
> > +                                     epd = &alt->endpoint[k].desc;
> > +                                     if (usb_endpoint_xfer_isoc(epd)) {
> > +                                             is_audio = true;
> > +                                             break;
> > +                                     }
> > +                             }
> > +                     }
> > +             }
> > +     }
> > +
> > +out:
> > +     return is_audio;
> > +}
> > +
> > +/*
> > + * check the usb device including the video class:
> > + *     True: Devices contain video class
> > + *    False: Device doesn't contain video class
> > + */
> > +static bool is_usb_video_device(struct usb_device *udev)
> > +{
> > +     struct usb_host_config *config;
> > +     struct usb_host_interface *alt;
> > +     struct usb_interface_cache *intfc;
> > +     int i, j;
> > +     bool is_video = false;
> > +
> > +     if (!udev || !udev->config)
> > +             return is_video;
> > +
> > +     config = udev->config;
> > +
> > +     for (i = 0; i < config->desc.bNumInterfaces; i++) {
> > +             intfc = config->intf_cache[i];
> > +             for (j = 0; j < intfc->num_altsetting; j++) {
> > +                     alt = &intfc->altsetting[j];
> > +
> > +                     if (alt->desc.bInterfaceClass == USB_CLASS_VIDEO) {
> > +                             is_video = true;
> > +                             goto out;
> > +                     }
> > +             }
> > +     }
> > +
> > +out:
> > +     return is_video;
> > +}
> > +
> > +/*
> > + * This is the driver call to co-processor for offload operations.
> > + */
> > +int offload_driver_call(enum usb_offload_msg msg, void *ptr)
> > +{
> > +     enum usb_offload_msg offload_msg;
> > +     void *argptr;
> > +
> > +     offload_msg = msg;
> > +     argptr = ptr;
>
> Don't just silence compiler warnings for no reason.
>
> Again, this does not actually do anything at all.  So how can we accept
> this code?
>

This is the driver call to our co-processor which is a specific
hardware, so I don't submit it
and make it silent here.

> > +
> > +     return 0;
> > +}
> > +
> > +static int xhci_sync_conn_stat(unsigned int bus_id, unsigned int dev_num, unsigned int slot_id,
> > +                             unsigned int conn_stat)
> > +{
> > +     struct conn_stat_args conn_args;
> > +
> > +     conn_args.bus_id = bus_id;
> > +     conn_args.dev_num = dev_num;
> > +     conn_args.slot_id = slot_id;
> > +     conn_args.conn_stat = conn_stat;
> > +
> > +     return offload_driver_call(SYNC_CONN_STAT, &conn_args);
> > +}
> > +
> > +static int usb_host_mode_state_notify(enum usb_state usb_state)
> > +{
> > +     return xhci_sync_conn_stat(0, 0, 0, usb_state);
> > +}
> > +
> > +static int xhci_udev_notify(struct notifier_block *self, unsigned long action,
> > +                             void *dev)
> > +{
> > +     struct usb_device *udev = dev;
> > +     struct xhci_offload_data *offload_data = xhci_get_offload_data();
> > +
> > +     switch (action) {
> > +     case USB_DEVICE_ADD:
> > +             if (is_compatible_with_usb_audio_offload(udev)) {
> > +                     dev_dbg(&udev->dev, "Compatible with usb audio offload\n");
> > +                     if (offload_data->op_mode == USB_OFFLOAD_DRAM) {
> > +                             xhci_sync_conn_stat(udev->bus->busnum, udev->devnum, udev->slot_id,
> > +                                                 USB_CONNECTED);
> > +                     }
> > +             }
> > +             offload_data->usb_accessory_enabled = false;
> > +             break;
> > +     case USB_DEVICE_REMOVE:
> > +             if (is_compatible_with_usb_audio_offload(udev) &&
> > +                 (offload_data->op_mode == USB_OFFLOAD_DRAM)) {
> > +                     xhci_sync_conn_stat(udev->bus->busnum, udev->devnum, udev->slot_id,
> > +                                         USB_DISCONNECTED);
> > +             }
> > +             offload_data->usb_accessory_enabled = false;
> > +             break;
> > +     }
> > +
> > +     return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block xhci_udev_nb = {
> > +     .notifier_call = xhci_udev_notify,
> > +};
> > +
> > +static int usb_audio_offload_init(struct xhci_hcd *xhci)
> > +{
> > +     struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
> > +     struct xhci_offload_data *offload_data = xhci_get_offload_data();
> > +     int ret;
> > +     u32 out_val;
> > +
> > +     offload_data = kzalloc(sizeof(struct xhci_offload_data), GFP_KERNEL);
> > +     if (!offload_data)
> > +             return -ENOMEM;
> > +
> > +     if (!of_property_read_u32(dev->of_node, "offload", &out_val))
> > +             offload_data->usb_audio_offload = (out_val == 1) ? true : false;
> > +
> > +     ret = of_reserved_mem_device_init(dev);
> > +     if (ret) {
> > +             dev_err(dev, "Could not get reserved memory\n");
> > +             kfree(offload_data);
> > +             return ret;
> > +     }
> > +
> > +     offload_data->dt_direct_usb_access =
> > +             of_property_read_bool(dev->of_node, "direct-usb-access") ? true : false;
> > +     if (!offload_data->dt_direct_usb_access)
> > +             dev_warn(dev, "Direct USB accesconfirm ifs is not supported\n");
> > +
> > +     offload_data->offload_state = true;
> > +
> > +     usb_register_notify(&xhci_udev_nb);
> > +     offload_data->op_mode = USB_OFFLOAD_DRAM;
> > +     offload_data->xhci = xhci;
> > +
> > +     return 0;
> > +}
> > +
> > +static void usb_audio_offload_cleanup(struct xhci_hcd *xhci)
> > +{
> > +     struct xhci_offload_data *offload_data = xhci_get_offload_data();
> > +
> > +     offload_data->usb_audio_offload = false;
> > +     offload_data->op_mode = USB_OFFLOAD_STOP;
> > +     offload_data->xhci = NULL;
> > +
> > +     usb_unregister_notify(&xhci_udev_nb);
> > +
> > +     /* Notification for xhci driver removing */
> > +     usb_host_mode_state_notify(USB_DISCONNECTED);
> > +
> > +     kfree(offload_data);
> > +     offload_data = NULL;
>
> Why are you setting a stack variable to NULL?
Thanks, I will remove it.

>
> Anyway, this looks much better overall than the previous submissions,
> but we need a real user of this code otherwise it can not be accepted.
> Please submit the driver that uses this api as part of your next
> submission.
>

We define and use those hook apis in the common xhci driver to offload
some memory
manipulation to a co-processor. So these apis will be executed to
allocate or free memory,
like dcbaa, transfer ring, in the co-processor memory space when
offlooffload_driver_callad enabled. The file,
xhci-offload-impl.c, shows how we use those xHCI hook apis for the
offload implementation.

Here is the flow diagram:
xHCI common driver        xHCI offload implement driver
co-processor driver
hooks
                    offload_driver_call()
----------------------------
----------------------------------------
--------------------------------------------------------------
offload_init                         usb_audio_offload_init
offload_cleanup                 usb_audio_offload_cleanup
is_offload_enabled             is_offload_enabled
alloc_dcbaa                        alloc_dcbaa
       offload_driver_call(SET_DCBAA_PTR, &dcbaa_ptr);

                       offload_driver_call(SETUP_DONE, NULL);
free_dcbaa                         free_dcbaa
alloc_transfer_ring             alloc_transfer_ring
   offload_driver_call(SET_ISOC_TR_INFO, &tr_info);
free_transfer_ring              free_transfer_ring
usb_offload_skip_urb        offload_skip_urb

Thanks!

> thanks,
>
> greg k-h