Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Felipe Balbi <felipe.balbi@linux.intel.com>
To: Pawel Laszczak <pawell@cadence.com>, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	hdegoede@redhat.com, heikki.krogerus@linux.intel.com,
	robh+dt@kernel.org, rogerq@ti.com, linux-kernel@vger.kernel.org,
	jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com,
	sureshp@cadence.com, peter.chen@nxp.com, jpawar@cadence.com,
	kurahul@cadence.com, Pawel Laszczak <pawell@cadence.com>
Subject: Re: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
Date: Mon, 08 Jul 2019 10:11:12 +0300
Message-ID: <877e8tm25r.fsf@linux.intel.com> (raw)
In-Reply-To: <1562324238-16655-6-git-send-email-pawell@cadence.com>

[-- Attachment #1: Type: text/plain, Size: 11502 bytes --]


Hi,

Pawel Laszczak <pawell@cadence.com> writes:
> +static void cdns3_gadget_config(struct cdns3_device *priv_dev)
> +{
> +	struct cdns3_usb_regs __iomem *regs = priv_dev->regs;
> +	u32 reg;
> +
> +	cdns3_ep0_config(priv_dev);
> +
> +	/* enable interrupts for endpoint 0 (in and out) */
> +	writel(EP_IEN_EP_OUT0 | EP_IEN_EP_IN0, &regs->ep_ien);
> +
> +	/*
> +	 *Driver need modify LFPS minimal U1 Exit time for 0x00024505 revision

comment style

> +	 * of controller
> +	 */
> +	if (priv_dev->dev_ver == DEV_VER_TI_V1) {

this version is really only for TI? And there's another only for NXP?

+#define DEV_VER_NXP_V1		0x00024502
+#define DEV_VER_TI_V1		0x00024509
+#define DEV_VER_V2		0x0002450C
+#define DEV_VER_V3		0x0002450d

How do you actually decode this?

> +static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
> +{
> +	struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
> +	struct cdns3_endpoint *priv_ep;
> +	u32 bEndpointAddress;
> +	struct usb_ep *ep;
> +	int ret = 0;
> +
> +	priv_dev->gadget_driver = NULL;
> +
> +	priv_dev->onchip_used_size = 0;
> +	priv_dev->out_mem_is_allocated = 0;
> +	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
> +
> +	list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) {
> +		priv_ep = ep_to_cdns3_ep(ep);
> +		bEndpointAddress = priv_ep->num | priv_ep->dir;
> +		cdns3_select_ep(priv_dev, bEndpointAddress);
> +		writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd);
> +		ret = cdns3_handshake(&priv_dev->regs->ep_cmd,
> +				      EP_CMD_EPRST, 0, 100);
> +		cdns3_free_trb_pool(priv_ep);

are you sure you want to free your trb pool when a gadget driver is
unloaded? One can easily fragment memory by constantly loading and
unloading a gadget driver, no?

How about you allocate the trb poll during cdns3 load and free it when
cdns3 is unloaded?

> +static int cdns3_gadget_start(struct cdns3 *cdns)
> +{
> +	struct cdns3_device *priv_dev;
> +	u32 max_speed;
> +	int ret;
> +
> +	priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
> +	if (!priv_dev)
> +		return -ENOMEM;
> +
> +	cdns->gadget_dev = priv_dev;
> +	priv_dev->sysdev = cdns->dev;
> +	priv_dev->dev = cdns->dev;
> +	priv_dev->regs = cdns->dev_regs;
> +
> +	device_property_read_u16(priv_dev->dev, "cdns,on-chip-buff-size",
> +				 &priv_dev->onchip_buffers);
> +
> +	if (priv_dev->onchip_buffers <=  0) {
> +		u32 reg = readl(&priv_dev->regs->usb_cap2);
> +
> +		priv_dev->onchip_buffers = USB_CAP2_ACTUAL_MEM_SIZE(reg);
> +	}
> +
> +	if (!priv_dev->onchip_buffers)
> +		priv_dev->onchip_buffers = 256;
> +
> +	max_speed = usb_get_maximum_speed(cdns->dev);
> +
> +	/* Check the maximum_speed parameter */
> +	switch (max_speed) {
> +	case USB_SPEED_FULL:
> +	case USB_SPEED_HIGH:
> +	case USB_SPEED_SUPER:
> +		break;
> +	default:
> +		dev_err(cdns->dev, "invalid maximum_speed parameter %d\n",
> +			max_speed);
> +		/* fall through */
> +	case USB_SPEED_UNKNOWN:
> +		/* default to superspeed */
> +		max_speed = USB_SPEED_SUPER;
> +		break;
> +	}
> +
> +	/* fill gadget fields */
> +	priv_dev->gadget.max_speed = max_speed;
> +	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
> +	priv_dev->gadget.ops = &cdns3_gadget_ops;
> +	priv_dev->gadget.name = "usb-ss-gadget";
> +	priv_dev->gadget.sg_supported = 1;
> +
> +	spin_lock_init(&priv_dev->lock);
> +	INIT_WORK(&priv_dev->pending_status_wq,
> +		  cdns3_pending_setup_status_handler);
> +
> +	/* initialize endpoint container */
> +	INIT_LIST_HEAD(&priv_dev->gadget.ep_list);
> +	INIT_LIST_HEAD(&priv_dev->aligned_buf_list);
> +
> +	ret = cdns3_init_eps(priv_dev);
> +	if (ret) {
> +		dev_err(priv_dev->dev, "Failed to create endpoints\n");
> +		goto err1;
> +	}
> +
> +	/* allocate memory for setup packet buffer */
> +	priv_dev->setup_buf = dma_alloc_coherent(priv_dev->sysdev, 8,
> +						 &priv_dev->setup_dma, GFP_DMA);
> +	if (!priv_dev->setup_buf) {
> +		ret = -ENOMEM;
> +		goto err2;
> +	}
> +
> +	priv_dev->dev_ver = readl(&priv_dev->regs->usb_cap6);
> +
> +	dev_dbg(priv_dev->dev, "Device Controller version: %08x\n",
> +		readl(&priv_dev->regs->usb_cap6));
> +	dev_dbg(priv_dev->dev, "USB Capabilities:: %08x\n",
> +		readl(&priv_dev->regs->usb_cap1));
> +	dev_dbg(priv_dev->dev, "On-Chip memory cnfiguration: %08x\n",
> +		readl(&priv_dev->regs->usb_cap2));
> +
> +	priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
> +
> +	priv_dev->zlp_buf = kzalloc(CDNS3_EP_ZLP_BUF_SIZE, GFP_KERNEL);
> +	if (!priv_dev->zlp_buf) {
> +		ret = -ENOMEM;
> +		goto err3;
> +	}
> +
> +	/* add USB gadget device */
> +	ret = usb_add_gadget_udc(priv_dev->dev, &priv_dev->gadget);
> +	if (ret < 0) {
> +		dev_err(priv_dev->dev,
> +			"Failed to register USB device controller\n");
> +		goto err4;
> +	}
> +
> +	return 0;
> +err4:
> +	kfree(priv_dev->zlp_buf);
> +err3:
> +	dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup_buf,
> +			  priv_dev->setup_dma);
> +err2:
> +	cdns3_free_all_eps(priv_dev);
> +err1:
> +	cdns->gadget_dev = NULL;
> +	return ret;
> +}
> +
> +static int __cdns3_gadget_init(struct cdns3 *cdns)
> +{
> +	struct cdns3_device *priv_dev;
> +	int ret = 0;
> +
> +	cdns3_drd_switch_gadget(cdns, 1);
> +	pm_runtime_get_sync(cdns->dev);
> +
> +	ret = cdns3_gadget_start(cdns);
> +	if (ret)
> +		return ret;
> +
> +	priv_dev = cdns->gadget_dev;
> +	ret = devm_request_threaded_irq(cdns->dev, cdns->dev_irq,
> +					cdns3_device_irq_handler,
> +					cdns3_device_thread_irq_handler,
> +					IRQF_SHARED, dev_name(cdns->dev), cdns);

copied handlers here for commenting. Note that you don't have
IRQF_ONESHOT:

> +static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
> +{
> +	struct cdns3_device *priv_dev;
> +	struct cdns3 *cdns = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	priv_dev = cdns->gadget_dev;
> +	spin_lock_irqsave(&priv_dev->lock, flags);

the top half handler runs in hardirq context. You don't need any locks
here. Also IRQs are *already* disabled, you don't need to disable them again.

> +
> +	/* check USB device interrupt */
> +	reg = readl(&priv_dev->regs->usb_ists);
> +
> +	if (reg) {
> +		writel(reg, &priv_dev->regs->usb_ists);
> +		cdns3_check_usb_interrupt_proceed(priv_dev, reg);
> +		ret = IRQ_HANDLED;

now, because you _don't_ mask this interrupt, you're gonna have
issues. Say we actually get both device and endpoint interrupts while
the thread is already running with previous endpoint interrupts. Now
we're gonna reenter the top half, because device interrupts are *not*
masked, which will read usb_ists and handle it here.

Then it will ahead and read ep_ists and wake the thread that's already
running.

This hasn't really been tested, has it?

> +static irqreturn_t cdns3_device_thread_irq_handler(int irq, void *data)
> +{
> +	struct cdns3_device *priv_dev;
> +	struct cdns3 *cdns = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	unsigned long flags;
> +	u32 ep_ien;
> +	int bit;
> +	u32 reg;
> +
> +	priv_dev = cdns->gadget_dev;
> +	spin_lock_irqsave(&priv_dev->lock, flags);

Ideally, the _irqsave() here wouldn't be necessary (since this device's
interrupts are already masked), but removing _irqsave() causes problem
with networking gadgets. Just thought I'd leave a note here, nothing to
change in this handler.

> +	reg = readl(&priv_dev->regs->ep_ists);
> +
> +	/* handle default endpoint OUT */
> +	if (reg & EP_ISTS_EP_OUT0) {
> +		cdns3_check_ep0_interrupt_proceed(priv_dev, USB_DIR_OUT);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	/* handle default endpoint IN */
> +	if (reg & EP_ISTS_EP_IN0) {
> +		cdns3_check_ep0_interrupt_proceed(priv_dev, USB_DIR_IN);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	/* check if interrupt from non default endpoint, if no exit */
> +	reg &= ~(EP_ISTS_EP_OUT0 | EP_ISTS_EP_IN0);
> +	if (!reg)
> +		goto irqend;
> +
> +	for_each_set_bit(bit, (unsigned long *)&reg,
> +			 sizeof(u32) * BITS_PER_BYTE) {
> +		priv_dev->shadow_ep_en |= BIT(bit);
> +		cdns3_check_ep_interrupt_proceed(priv_dev->eps[bit]);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	if (priv_dev->run_garbage_colector) {

wait, what?

ps: correct spelling is "collector" ;-)

> +		struct cdns3_aligned_buf *buf, *tmp;
> +
> +		list_for_each_entry_safe(buf, tmp, &priv_dev->aligned_buf_list,
> +					 list) {
> +			if (!buf->in_use) {
> +				list_del(&buf->list);
> +
> +				spin_unlock_irqrestore(&priv_dev->lock, flags);

creates the possibility of a race condition

> +				dma_free_coherent(priv_dev->sysdev, buf->size,
> +						  buf->buf,
> +						  buf->dma);
> +				spin_lock_irqsave(&priv_dev->lock, flags);
> +
> +				kfree(buf);

why do you even need this "garbage collector"?

> +static int cdns3_gadget_suspend(struct cdns3 *cdns, bool do_wakeup)
> +{
> +	cdns3_gadget_exit(cdns);

so on suspend you completely teardown the entire controller? This means
that a mounted mass storage gadget will, actually, disconnect from the
host, no?

> diff --git a/drivers/usb/cdns3/trace.h b/drivers/usb/cdns3/trace.h
> new file mode 100644
> index 000000000000..1cc2abca320c
> --- /dev/null
> +++ b/drivers/usb/cdns3/trace.h
> @@ -0,0 +1,447 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * USBSS device controller driver.
> + * Trace support header file.
> + *
> + * Copyright (C) 2018 Cadence.
> + *
> + * Author: Pawel Laszczak <pawell@cadence.com>
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM cdns3
> +
> +#if !defined(__LINUX_CDNS3_TRACE) || defined(TRACE_HEADER_MULTI_READ)
> +#define __LINUX_CDNS3_TRACE
> +
> +#include <linux/types.h>
> +#include <linux/tracepoint.h>
> +#include <asm/byteorder.h>
> +#include <linux/usb/ch9.h>
> +#include "core.h"
> +#include "gadget.h"
> +#include "debug.h"
> +
> +#define CDNS3_MSG_MAX	500
> +
> +TRACE_EVENT(cdns3_log,
> +	TP_PROTO(struct cdns3_device *priv_dev, struct va_format *vaf),
> +	TP_ARGS(priv_dev, vaf),
> +	TP_STRUCT__entry(
> +		__string(name, dev_name(priv_dev->dev))
> +		__dynamic_array(char, msg, CDNS3_MSG_MAX)
> +	),
> +	TP_fast_assign(
> +		__assign_str(name, dev_name(priv_dev->dev));
> +		vsnprintf(__get_str(msg), CDNS3_MSG_MAX, vaf->fmt, *vaf->va);
> +	),
> +	TP_printk("%s: %s", __get_str(name), __get_str(msg))
> +);
> +
> +DECLARE_EVENT_CLASS(cdns3_log_doorbell,
> +	TP_PROTO(const char *ep_name, u32 ep_trbaddr),
> +	TP_ARGS(ep_name, ep_trbaddr),
> +	TP_STRUCT__entry(
> +		__string(name, ep_name)
> +		__field(u32, ep_trbaddr)
> +	),
> +	TP_fast_assign(
> +		__assign_str(name, ep_name);
> +		__entry->ep_trbaddr = ep_trbaddr;
> +	),
> +	TP_printk("//Ding Dong %s, ep_trbaddr %08x", __get_str(name),

nit-picking but... the event name will be printed on trace log. You
don't need this "Ding Dong" string here :-p

> +	TP_printk("%s: req: %p, req buff %p, length: %u/%u %s%s%s, status: %d,"
> +		  " trb: [start:%d, end:%d: virt addr %pa], flags:%x ",
> +		__get_str(name), __entry->req, __entry->buf, __entry->actual,
> +		__entry->length,
> +		__entry->zero ? "zero | " : "",
> +		__entry->short_not_ok ? "short | " : "",
> +		__entry->no_interrupt ? "no int" : "",

I guess you didn't really think the formatting through. Think about what
happens if you get a request with only zero flag or only short flag. How
will this log look like?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

  parent reply index

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-05 10:57 [PATCH v9 0/6] Introduced new Cadence USBSS " Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 1/6] dt-bindings: add binding for USBSS-DRD controller Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 2/6] usb:gadget Separated decoding functions from dwc3 driver Pawel Laszczak
2019-07-05 11:27   ` Greg KH
2019-07-05 11:39     ` Pawel Laszczak
2019-07-05 11:49       ` Greg KH
2019-07-05 12:03         ` Pawel Laszczak
2019-07-05 11:39   ` Felipe Balbi
2019-07-05 11:44     ` Pawel Laszczak
2019-08-07 10:17       ` Roger Quadros
2019-08-07 10:22         ` Felipe Balbi
2019-08-08  3:07           ` Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 3/6] usb:gadget Patch simplify usb_decode_set_clear_feature function Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 4/6] usb:gadget Simplify usb_decode_get_set_descriptor function Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver Pawel Laszczak
2019-07-05 11:55   ` Felipe Balbi
2019-07-07 17:56     ` Pawel Laszczak
2019-07-08  6:29       ` Felipe Balbi
2019-07-08 10:59         ` Pawel Laszczak
2019-07-08 11:15           ` Felipe Balbi
2019-07-08 11:45             ` Pawel Laszczak
2019-07-08 11:59               ` Felipe Balbi
2019-07-08 12:39                 ` Pawel Laszczak
2019-07-09  4:23         ` Pawel Laszczak
2019-07-09  6:36           ` Felipe Balbi
2019-07-09  7:07             ` Pawel Laszczak
2019-07-09  7:22               ` Felipe Balbi
2019-07-09  7:29                 ` Pawel Laszczak
2019-07-10  8:25       ` Pawel Laszczak
2019-07-08  7:11   ` Felipe Balbi [this message]
2019-07-15 11:00     ` Pawel Laszczak
2019-08-07 10:34       ` Felipe Balbi
2019-08-10 20:39         ` Pawel Laszczak
2019-08-12  1:55           ` Peter Chen
2019-08-12  4:43             ` Pawel Laszczak
2019-08-12  5:24           ` Felipe Balbi
2019-08-12  7:13             ` Pawel Laszczak
2019-08-12  8:19               ` Felipe Balbi
2019-08-12  9:13                 ` Pawel Laszczak
2019-08-12  9:45                   ` Felipe Balbi
2019-08-12 10:26                     ` Pawel Laszczak
2019-08-12 10:34                       ` Felipe Balbi
2019-08-12 14:20                         ` Alan Stern
2019-07-05 10:57 ` [PATCH v9 6/6] usb:cdns3 Fix for stuck packets in on-chip OUT buffer Pawel Laszczak

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877e8tm25r.fsf@linux.intel.com \
    --to=felipe.balbi@linux.intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=jbergsagel@ti.com \
    --cc=jpawar@cadence.com \
    --cc=kurahul@cadence.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=nm@ti.com \
    --cc=nsekhar@ti.com \
    --cc=pawell@cadence.com \
    --cc=peter.chen@nxp.com \
    --cc=robh+dt@kernel.org \
    --cc=rogerq@ti.com \
    --cc=sureshp@cadence.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox