Message-ID: <Y2yzg2v2AL6MsKvy@kroah.com>
Date:   Thu, 10 Nov 2022 09:17:07 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Albert Wang <albertccwang@...gle.com>
Cc:     mathias.nyman@...el.com, badhri@...gle.com, howardyen@...gle.com,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 3/3] usb: host: add the xhci offload hooks
 implementations

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@...gle.com>
> Signed-off-by: Howard Yen <howardyen@...gle.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 :)

> + *
> + * Author:
> + *  Howard.Yen <howardyen@...gle.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?

> +
> +	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 access 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?

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.

thanks,

greg k-h

Powered by blists - more mailing lists