[PATCH v2 0/4] usb: renesas_usbhs: add support for USB-DMAC

[PATCH v2 0/4] usb: renesas_usbhs: add support for USB-DMAC

[Yoshihiro Shimoda]

This patch set is based on Felipe's usb.git / testing/next branch.
(commit id = 6461d69d12508fe166be5c6c31d5a9da02a4dfb5)

To use the USB-DMAC, we have to add some device nodes for USB-DMAC
in dts file. If we don't have such device nodes, this driver will
use PIO.

Changes from v1:
 - Change local_irq_save/restore() area in patch 1.
 - Modify the commit log in patch 1.
 - fix some comments in patch 4.

Yoshihiro Shimoda (4):
  usb: renesas_usbhs: fix spinlock suspected in a gadget complete
    function
  usb: renesas_usbhs: add the channel number in dma-names
  usb: renesas_usbhs: fix the sequence in xfer_work()
  usb: renesas_usbhs: add support for USB-DMAC

 .../devicetree/bindings/usb/renesas_usbhs.txt      |    5 +-
 drivers/usb/renesas_usbhs/common.c                 |   19 ++
 drivers/usb/renesas_usbhs/common.h                 |    7 +
 drivers/usb/renesas_usbhs/fifo.c                   |  189 ++++++++++++++++++--
 drivers/usb/renesas_usbhs/fifo.h                   |    1 +
 drivers/usb/renesas_usbhs/mod_gadget.c             |   11 ++
 drivers/usb/renesas_usbhs/pipe.c                   |   39 ++++
 drivers/usb/renesas_usbhs/pipe.h                   |    1 +
 include/linux/usb/renesas_usbhs.h                  |    2 +
 9 files changed, 258 insertions(+), 16 deletions(-)

-- 
1.7.9.5

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

[PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Yoshihiro Shimoda]

According to the gadget.h, a "complete" function will always be called
with interrupts disabled. However, sometimes usbhsg_queue_pop() function
is called with interrupts enabled. So, this function should calls
local_irq_save() before this calls the usb_gadget_giveback_request().
Otherwise, there is possible to cause a spinlock suspected in a gadget
complete function.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/renesas_usbhs/mod_gadget.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index e0384af..104bddf 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -126,11 +126,22 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
 	struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
 	struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep);
 	struct device *dev = usbhsg_gpriv_to_dev(gpriv);
+	unsigned long flags;
 
 	dev_dbg(dev, "pipe %d : queue pop\n", usbhs_pipe_number(pipe));
 
 	ureq->req.status = status;
+	/*
+	 * According to the gadget.h, a "complete" function will always be
+	 * called with interrupts disabled. However, sometimes this function
+	 * is called with interrupts enabled. (e.g. complete a DMAC transfer or
+	 * write data and done is set immediately when PIO.) So, this function
+	 * should calls local_irq_save() before this calls the
+	 * usb_gadget_giveback_request().
+	 */
+	local_irq_save(flags);
 	usb_gadget_giveback_request(&uep->ep, &ureq->req);
+	local_irq_restore(flags);
 }
 
 static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
-- 
1.7.9.5

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

[PATCH v2 2/4] usb: renesas_usbhs: add the channel number in dma-names

[Yoshihiro Shimoda]

To connect the channel of USB-DMAC to USBHS DnFIFO number, this patch
adds this channel/FIFO number in dma-names. Otherwise, this driver
needs to add analysis code for device tree.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../devicetree/bindings/usb/renesas_usbhs.txt      |    5 ++++-
 drivers/usb/renesas_usbhs/fifo.c                   |   20 +++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
index 61b045b..dc2a18f 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
@@ -15,7 +15,10 @@ Optional properties:
   - phys: phandle + phy specifier pair
   - phy-names: must be "usb"
   - dmas: Must contain a list of references to DMA specifiers.
-  - dma-names : Must contain a list of DMA names, "tx" or "rx".
+  - dma-names : Must contain a list of DMA names:
+   - tx0 ... tx<n>
+   - rx0 ... rx<n>
+    - This <n> means DnFIFO in USBHS module.
 
 Example:
 	usbhs: usb@e6590000 {
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index d891bff..28d10eb 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -1069,23 +1069,29 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo)
 					    &fifo->rx_slave);
 }
 
-static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo)
+static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo,
+			       int channel)
 {
-	fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx");
+	char name[16];
+
+	snprintf(name, sizeof(name), "tx%d", channel);
+	fifo->tx_chan = dma_request_slave_channel_reason(dev, name);
 	if (IS_ERR(fifo->tx_chan))
 		fifo->tx_chan = NULL;
-	fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx");
+
+	snprintf(name, sizeof(name), "rx%d", channel);
+	fifo->rx_chan = dma_request_slave_channel_reason(dev, name);
 	if (IS_ERR(fifo->rx_chan))
 		fifo->rx_chan = NULL;
 }
 
-static void usbhsf_dma_init(struct usbhs_priv *priv,
-			    struct usbhs_fifo *fifo)
+static void usbhsf_dma_init(struct usbhs_priv *priv, struct usbhs_fifo *fifo,
+			    int channel)
 {
 	struct device *dev = usbhs_priv_to_dev(priv);
 
 	if (dev->of_node)
-		usbhsf_dma_init_dt(dev, fifo);
+		usbhsf_dma_init_dt(dev, fifo, channel);
 	else
 		usbhsf_dma_init_pdev(fifo);
 
@@ -1231,7 +1237,7 @@ do {									\
 			usbhs_get_dparam(priv, d##channel##_tx_id);	\
 	fifo->rx_slave.shdma_slave.slave_id =				\
 			usbhs_get_dparam(priv, d##channel##_rx_id);	\
-	usbhsf_dma_init(priv, fifo);					\
+	usbhsf_dma_init(priv, fifo, channel);				\
 } while (0)
 
 #define USBHS_DFIFO_INIT(priv, fifo, channel)				\
-- 
1.7.9.5

[PATCH v2 3/4] usb: renesas_usbhs: fix the sequence in xfer_work()

[Yoshihiro Shimoda]

This patch fixes the setup sequence in xfer_work(). Otherwise,
sometimes a usb transaction will get stuck.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/renesas_usbhs/fifo.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 28d10eb..b737661 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -822,10 +822,10 @@ static void xfer_work(struct work_struct *work)
 		fifo->name, usbhs_pipe_number(pipe), pkt->length, pkt->zero);
 
 	usbhs_pipe_running(pipe, 1);
-	usbhs_pipe_set_trans_count_if_bulk(pipe, pkt->trans);
-	usbhs_pipe_enable(pipe);
 	usbhsf_dma_start(pipe, fifo);
+	usbhs_pipe_set_trans_count_if_bulk(pipe, pkt->trans);
 	dma_async_issue_pending(chan);
+	usbhs_pipe_enable(pipe);
 }
 
 /*
-- 
1.7.9.5

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

[PATCH v2 4/4] usb: renesas_usbhs: add support for USB-DMAC

[Yoshihiro Shimoda]

Some Renesas SoCs have the USB-DMAC. It is able to terminate transfers
when a short packet is received, even if less bytes than the transfer
counter size have been received. Also, it is able to send a short
packet even if the packet size is not multiples of 8bytes.

Since the previous code has used the interruption of USBHS controller
when receiving packets even if this driver has used a dmac, a lot of
interruptions has happened. This patch will reduce such interruptions.

This patch allows to use the USB-DMAC on R-Car H2 and M2.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/renesas_usbhs/common.c |   19 +++++
 drivers/usb/renesas_usbhs/common.h |    7 ++
 drivers/usb/renesas_usbhs/fifo.c   |  165 ++++++++++++++++++++++++++++++++++--
 drivers/usb/renesas_usbhs/fifo.h   |    1 +
 drivers/usb/renesas_usbhs/pipe.c   |   39 +++++++++
 drivers/usb/renesas_usbhs/pipe.h   |    1 +
 include/linux/usb/renesas_usbhs.h  |    2 +
 7 files changed, 228 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
index 4cf77d3..0f7e850 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -276,6 +276,16 @@ int usbhs_set_device_config(struct usbhs_priv *priv, int devnum,
 }
 
 /*
+ *		interrupt functions
+ */
+void usbhs_xxxsts_clear(struct usbhs_priv *priv, u16 sts_reg, u16 bit)
+{
+	u16 pipe_mask = (u16)GENMASK(usbhs_get_dparam(priv, pipe_size), 0);
+
+	usbhs_write(priv, sts_reg, ~(1 << bit) & pipe_mask);
+}
+
+/*
  *		local functions
  */
 static void usbhsc_set_buswait(struct usbhs_priv *priv)
@@ -487,6 +497,15 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
 	if (gpio > 0)
 		dparam->enable_gpio = gpio;
 
+	switch (dparam->type) {
+	case USBHS_TYPE_R8A7790:
+	case USBHS_TYPE_R8A7791:
+		dparam->has_usb_dmac = 1;
+		break;
+	default:
+		break;
+	}
+
 	return info;
 }
 
diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
index fc96e92..8c5fc12 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -193,6 +193,7 @@ struct usbhs_priv;
 #define TYPE_BULK	(1 << 14)
 #define TYPE_INT	(2 << 14)
 #define TYPE_ISO	(3 << 14)
+#define BFRE		(1 << 10)	/* BRDY Interrupt Operation Spec. */
 #define DBLB		(1 << 9)	/* Double Buffer Mode */
 #define SHTNAK		(1 << 7)	/* Pipe Disable in Transfer End */
 #define DIR_OUT		(1 << 4)	/* Transfer Direction */
@@ -216,6 +217,7 @@ struct usbhs_priv;
 #define	ACLRM		(1 << 9)	/* Buffer Auto-Clear Mode */
 #define SQCLR		(1 << 8)	/* Toggle Bit Clear */
 #define SQSET		(1 << 7)	/* Toggle Bit Set */
+#define SQMON		(1 << 6)	/* Toggle Bit Check */
 #define PBUSY		(1 << 5)	/* Pipe Busy */
 #define PID_MASK	(0x3)		/* Response PID */
 #define  PID_NAK	0
@@ -324,6 +326,11 @@ int usbhs_set_device_config(struct usbhs_priv *priv, int devnum, u16 upphub,
 			   u16 hubport, u16 speed);
 
 /*
+ * interrupt functions
+ */
+void usbhs_xxxsts_clear(struct usbhs_priv *priv, u16 sts_reg, u16 bit);
+
+/*
  * data
  */
 struct usbhs_priv *usbhs_pdev_to_priv(struct platform_device *pdev);
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index b737661..8597cf9 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -813,7 +813,8 @@ static void xfer_work(struct work_struct *work)
 	desc->callback		= usbhsf_dma_complete;
 	desc->callback_param	= pipe;
 
-	if (dmaengine_submit(desc) < 0) {
+	pkt->cookie = dmaengine_submit(desc);
+	if (pkt->cookie < 0) {
 		dev_err(dev, "Failed to submit dma descriptor\n");
 		return;
 	}
@@ -838,6 +839,7 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)
 	struct usbhs_fifo *fifo;
 	int len = pkt->length - pkt->actual;
 	int ret;
+	uintptr_t align_mask;
 
 	if (usbhs_pipe_is_busy(pipe))
 		return 0;
@@ -847,10 +849,14 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)
 	    usbhs_pipe_is_dcp(pipe))
 		goto usbhsf_pio_prepare_push;
 
-	if (len & 0x7) /* 8byte alignment */
+	/* check data length if this driver don't use USB-DMAC */
+	if (!usbhs_get_dparam(priv, has_usb_dmac) && len & 0x7)
 		goto usbhsf_pio_prepare_push;
 
-	if ((uintptr_t)(pkt->buf + pkt->actual) & 0x7) /* 8byte alignment */
+	/* check buffer alignment */
+	align_mask = usbhs_get_dparam(priv, has_usb_dmac) ?
+					USBHS_USB_DMAC_XFER_SIZE - 1 : 0x7;
+	if ((uintptr_t)(pkt->buf + pkt->actual) & align_mask)
 		goto usbhsf_pio_prepare_push;
 
 	/* return at this time if the pipe is running */
@@ -924,7 +930,85 @@ struct usbhs_pkt_handle usbhs_fifo_dma_push_handler = {
 /*
  *		DMA pop handler
  */
-static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)
+
+static int usbhsf_dma_prepare_pop_with_rx_irq(struct usbhs_pkt *pkt,
+					      int *is_done)
+{
+	return usbhsf_prepare_pop(pkt, is_done);
+}
+
+static int usbhsf_dma_prepare_pop_with_usb_dmac(struct usbhs_pkt *pkt,
+						int *is_done)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+	struct usbhs_fifo *fifo;
+	int ret;
+
+	if (usbhs_pipe_is_busy(pipe))
+		return 0;
+
+	/* use PIO if packet is less than pio_dma_border or pipe is DCP */
+	if ((pkt->length < usbhs_get_dparam(priv, pio_dma_border)) ||
+	    usbhs_pipe_is_dcp(pipe))
+		goto usbhsf_pio_prepare_pop;
+
+	fifo = usbhsf_get_dma_fifo(priv, pkt);
+	if (!fifo)
+		goto usbhsf_pio_prepare_pop;
+
+	if ((uintptr_t)pkt->buf & (USBHS_USB_DMAC_XFER_SIZE - 1))
+		goto usbhsf_pio_prepare_pop;
+
+	usbhs_pipe_config_change_bfre(pipe, 1);
+
+	ret = usbhsf_fifo_select(pipe, fifo, 0);
+	if (ret < 0)
+		goto usbhsf_pio_prepare_pop;
+
+	if (usbhsf_dma_map(pkt) < 0)
+		goto usbhsf_pio_prepare_pop_unselect;
+
+	/* DMA */
+
+	/*
+	 * usbhs_fifo_dma_pop_handler :: prepare
+	 * enabled irq to come here.
+	 * but it is no longer needed for DMA. disable it.
+	 */
+	usbhsf_rx_irq_ctrl(pipe, 0);
+
+	pkt->trans = pkt->length;
+
+	INIT_WORK(&pkt->work, xfer_work);
+	schedule_work(&pkt->work);
+
+	return 0;
+
+usbhsf_pio_prepare_pop_unselect:
+	usbhsf_fifo_unselect(pipe, fifo);
+usbhsf_pio_prepare_pop:
+
+	/*
+	 * change handler to PIO
+	 */
+	pkt->handler = &usbhs_fifo_pio_pop_handler;
+	usbhs_pipe_config_change_bfre(pipe, 0);
+
+	return pkt->handler->prepare(pkt, is_done);
+}
+
+static int usbhsf_dma_prepare_pop(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	if (usbhs_get_dparam(priv, has_usb_dmac))
+		return usbhsf_dma_prepare_pop_with_usb_dmac(pkt, is_done);
+	else
+		return usbhsf_dma_prepare_pop_with_rx_irq(pkt, is_done);
+}
+
+static int usbhsf_dma_try_pop_with_rx_irq(struct usbhs_pkt *pkt, int *is_done)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;
 	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
@@ -993,7 +1077,16 @@ usbhsf_pio_prepare_pop:
 	return pkt->handler->try_run(pkt, is_done);
 }
 
-static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
+static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	BUG_ON(usbhs_get_dparam(priv, has_usb_dmac));
+
+	return usbhsf_dma_try_pop_with_rx_irq(pkt, is_done);
+}
+
+static int usbhsf_dma_pop_done_with_rx_irq(struct usbhs_pkt *pkt, int *is_done)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;
 	int maxp = usbhs_pipe_get_maxpacket(pipe);
@@ -1017,8 +1110,68 @@ static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
 	return 0;
 }
 
+static size_t usbhs_dma_calc_received_size(struct usbhs_pkt *pkt,
+					   struct dma_chan *chan, int dtln)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct dma_tx_state state;
+	size_t received_size;
+	int maxp = usbhs_pipe_get_maxpacket(pipe);
+
+	dmaengine_tx_status(chan, pkt->cookie, &state);
+	received_size = pkt->length - state.residue;
+
+	if (dtln) {
+		received_size -= USBHS_USB_DMAC_XFER_SIZE;
+		received_size &= ~(maxp - 1);
+		received_size += dtln;
+	}
+
+	return received_size;
+}
+
+static int usbhsf_dma_pop_done_with_usb_dmac(struct usbhs_pkt *pkt,
+					     int *is_done)
+{
+	struct usbhs_pipe *pipe = pkt->pipe;
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+	struct usbhs_fifo *fifo = usbhs_pipe_to_fifo(pipe);
+	struct dma_chan *chan = usbhsf_dma_chan_get(fifo, pkt);
+	int rcv_len;
+
+	/*
+	 * Since the driver disables rx_irq in DMA mode, the interrupt handler
+	 * cannot the BRDYSTS. So, the function clears it here because the
+	 * driver may use PIO mode next time.
+	 */
+	usbhs_xxxsts_clear(priv, BRDYSTS, usbhs_pipe_number(pipe));
+
+	rcv_len = usbhsf_fifo_rcv_len(priv, fifo);
+	usbhsf_fifo_clear(pipe, fifo);
+	pkt->actual = usbhs_dma_calc_received_size(pkt, chan, rcv_len);
+
+	usbhsf_dma_stop(pipe, fifo);
+	usbhsf_dma_unmap(pkt);
+	usbhsf_fifo_unselect(pipe, pipe->fifo);
+
+	/* The driver can assume the rx transaction is always "done" */
+	*is_done = 1;
+
+	return 0;
+}
+
+static int usbhsf_dma_pop_done(struct usbhs_pkt *pkt, int *is_done)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pkt->pipe);
+
+	if (usbhs_get_dparam(priv, has_usb_dmac))
+		return usbhsf_dma_pop_done_with_usb_dmac(pkt, is_done);
+	else
+		return usbhsf_dma_pop_done_with_rx_irq(pkt, is_done);
+}
+
 struct usbhs_pkt_handle usbhs_fifo_dma_pop_handler = {
-	.prepare	= usbhsf_prepare_pop,
+	.prepare	= usbhsf_dma_prepare_pop,
 	.try_run	= usbhsf_dma_try_pop,
 	.dma_done	= usbhsf_dma_pop_done
 };
diff --git a/drivers/usb/renesas_usbhs/fifo.h b/drivers/usb/renesas_usbhs/fifo.h
index f07037c1..04d3f8a 100644
--- a/drivers/usb/renesas_usbhs/fifo.h
+++ b/drivers/usb/renesas_usbhs/fifo.h
@@ -58,6 +58,7 @@ struct usbhs_pkt {
 		     struct usbhs_pkt *pkt);
 	struct work_struct work;
 	dma_addr_t dma;
+	dma_cookie_t cookie;
 	void *buf;
 	int length;
 	int trans;
diff --git a/drivers/usb/renesas_usbhs/pipe.c b/drivers/usb/renesas_usbhs/pipe.c
index 007f45a..4f9c335 100644
--- a/drivers/usb/renesas_usbhs/pipe.c
+++ b/drivers/usb/renesas_usbhs/pipe.c
@@ -84,6 +84,17 @@ static void __usbhsp_pipe_xxx_set(struct usbhs_pipe *pipe,
 		usbhs_bset(priv, pipe_reg, mask, val);
 }
 
+static u16 __usbhsp_pipe_xxx_get(struct usbhs_pipe *pipe,
+				 u16 dcp_reg, u16 pipe_reg)
+{
+	struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
+
+	if (usbhs_pipe_is_dcp(pipe))
+		return usbhs_read(priv, dcp_reg);
+	else
+		return usbhs_read(priv, pipe_reg);
+}
+
 /*
  *		DCPCFG/PIPECFG functions
  */
@@ -92,6 +103,11 @@ static void usbhsp_pipe_cfg_set(struct usbhs_pipe *pipe, u16 mask, u16 val)
 	__usbhsp_pipe_xxx_set(pipe, DCPCFG, PIPECFG, mask, val);
 }
 
+static u16 usbhsp_pipe_cfg_get(struct usbhs_pipe *pipe)
+{
+	return __usbhsp_pipe_xxx_get(pipe, DCPCFG, PIPECFG);
+}
+
 /*
  *		PIPEnTRN/PIPEnTRE functions
  */
@@ -616,6 +632,11 @@ void usbhs_pipe_data_sequence(struct usbhs_pipe *pipe, int sequence)
 	usbhsp_pipectrl_set(pipe, mask, val);
 }
 
+static int usbhs_pipe_get_data_sequence(struct usbhs_pipe *pipe)
+{
+	return !!(usbhsp_pipectrl_get(pipe) & SQMON);
+}
+
 void usbhs_pipe_clear(struct usbhs_pipe *pipe)
 {
 	if (usbhs_pipe_is_dcp(pipe)) {
@@ -626,6 +647,24 @@ void usbhs_pipe_clear(struct usbhs_pipe *pipe)
 	}
 }
 
+void usbhs_pipe_config_change_bfre(struct usbhs_pipe *pipe, int enable)
+{
+	int sequence;
+
+	if (usbhs_pipe_is_dcp(pipe))
+		return;
+
+	usbhsp_pipe_select(pipe);
+	/* check if the driver needs to change the BFRE value */
+	if (!(enable ^ !!(usbhsp_pipe_cfg_get(pipe) & BFRE)))
+		return;
+
+	sequence = usbhs_pipe_get_data_sequence(pipe);
+	usbhsp_pipe_cfg_set(pipe, BFRE, enable ? BFRE : 0);
+	usbhs_pipe_clear(pipe);
+	usbhs_pipe_data_sequence(pipe, sequence);
+}
+
 static struct usbhs_pipe *usbhsp_get_pipe(struct usbhs_priv *priv, u32 type)
 {
 	struct usbhs_pipe *pos, *pipe;
diff --git a/drivers/usb/renesas_usbhs/pipe.h b/drivers/usb/renesas_usbhs/pipe.h
index d24a059..b0bc7b6 100644
--- a/drivers/usb/renesas_usbhs/pipe.h
+++ b/drivers/usb/renesas_usbhs/pipe.h
@@ -97,6 +97,7 @@ void usbhs_pipe_set_trans_count_if_bulk(struct usbhs_pipe *pipe, int len);
 void usbhs_pipe_select_fifo(struct usbhs_pipe *pipe, struct usbhs_fifo *fifo);
 void usbhs_pipe_config_update(struct usbhs_pipe *pipe, u16 devsel,
 			      u16 epnum, u16 maxp);
+void usbhs_pipe_config_change_bfre(struct usbhs_pipe *pipe, int enable);
 
 #define usbhs_pipe_sequence_data0(pipe)	usbhs_pipe_data_sequence(pipe, 0)
 #define usbhs_pipe_sequence_data1(pipe)	usbhs_pipe_data_sequence(pipe, 1)
diff --git a/include/linux/usb/renesas_usbhs.h b/include/linux/usb/renesas_usbhs.h
index 9fd9e48..f06529c 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/linux/usb/renesas_usbhs.h
@@ -165,6 +165,8 @@ struct renesas_usbhs_driver_param {
 	 */
 	u32 has_otg:1; /* for controlling PWEN/EXTLP */
 	u32 has_sudmac:1; /* for SUDMAC */
+	u32 has_usb_dmac:1; /* for USB-DMAC */
+#define USBHS_USB_DMAC_XFER_SIZE	32	/* hardcode the xfer size */
 };
 
 #define USBHS_TYPE_R8A7790 1
-- 
1.7.9.5

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

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> According to the gadget.h, a "complete" function will always be called
> with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> is called with interrupts enabled. So, this function should calls
> local_irq_save() before this calls the usb_gadget_giveback_request().
> Otherwise, there is possible to cause a spinlock suspected in a gadget
> complete function.

I still feel uneasy about adding the call to local_irq_save(), as the need for
this is usually an indicator of another locking problem.

Unfortunately I know next to nothing about the USB gadget subsystem, so some
help from the USB gadget experts would be appreciated.

I had a look at other drivers, and it seems most drivers actually release
and reacquire a spinlock around the call to usb_gadget_giveback_request(),
i.e. they do:

    spin_unlock(...);
    usb_gadget_giveback_request(...);
    spin_lock();

This means they had already acquired the spinlock (and disabled interrupts!)
before, which looks much healthier to me.

There's only one driver that uses local_irq_save() (pxa27x_udc).

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/renesas_usbhs/mod_gadget.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index e0384af..104bddf 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -126,11 +126,22 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
>         struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
>         struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep);
>         struct device *dev = usbhsg_gpriv_to_dev(gpriv);
> +       unsigned long flags;
>
>         dev_dbg(dev, "pipe %d : queue pop\n", usbhs_pipe_number(pipe));
>
>         ureq->req.status = status;
> +       /*
> +        * According to the gadget.h, a "complete" function will always be
> +        * called with interrupts disabled. However, sometimes this function
> +        * is called with interrupts enabled. (e.g. complete a DMAC transfer or
> +        * write data and done is set immediately when PIO.) So, this function
> +        * should calls local_irq_save() before this calls the
> +        * usb_gadget_giveback_request().
> +        */
> +       local_irq_save(flags);
>         usb_gadget_giveback_request(&uep->ep, &ureq->req);
> +       local_irq_restore(flags);
>  }
>
>  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi Geert-san,

Thank you for the reply again!

> Hi Shimoda-san,
> 
> On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > According to the gadget.h, a "complete" function will always be called
> > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > is called with interrupts enabled. So, this function should calls
> > local_irq_save() before this calls the usb_gadget_giveback_request().
> > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > complete function.
> 
> I still feel uneasy about adding the call to local_irq_save(), as the need for
> this is usually an indicator of another locking problem.

I also think that I would like to avoid using local_irq_save().
But, I have no idea to resolve this issue for now.

> Unfortunately I know next to nothing about the USB gadget subsystem, so some
> help from the USB gadget experts would be appreciated.
> 
> I had a look at other drivers, and it seems most drivers actually release
> and reacquire a spinlock around the call to usb_gadget_giveback_request(),
> i.e. they do:
> 
>     spin_unlock(...);
>     usb_gadget_giveback_request(...);
>     spin_lock();
> 
> This means they had already acquired the spinlock (and disabled interrupts!)
> before, which looks much healthier to me.
> 
> There's only one driver that uses local_irq_save() (pxa27x_udc).

I had a look at the musb driver. It uses a dmaengine driver.
And, in the callback routine of the musb driver, it calls spin_lock_irqsave() at first.

cppi41_dma_callback
--> spin_lock_irqsave(&musb->lock, flags);
--> cppi41_trans_done
 --> musb_dma_completion
  --> musb_g_tx or musb_g_rx
   --> musb_g_giveback
    --> spin_unlock(&musb->lock);   # This means unlocked the spin, but disabled interrupts.
    --> usb_gadget_giveback_request
    --> spin_lock(&musb->lock);
 < snip >
--> spin_unlock_irqrestore(&musb->lock, flags);

So, about disabled interrupts before usb_gadget_giveback_request(),
it is similar with this patch.

Best regards,
Yoshihiro Shimoda

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi Geert-san again,

> Hi Geert-san,
> 
> Thank you for the reply again!
> 
> > Hi Shimoda-san,
> >
> > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > According to the gadget.h, a "complete" function will always be called
> > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > is called with interrupts enabled. So, this function should calls
> > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > complete function.
> >
> > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > this is usually an indicator of another locking problem.
> 
> I also think that I would like to avoid using local_irq_save().
> But, I have no idea to resolve this issue for now.

After I modified usb-dmac driver to use a tasklet instead of a kthread,
this issue disappeared.

My understanding is the followings:
- According to the backtrace below, during usbhsf_dma_complete() was running,
 a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729

- This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
  to do the ncm_tx_tasklet().

- After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
  ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.


So, I would like to recall this patch and I will resubmit this patch series as v3.

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> Hi Geert-san again,
> 
> > Hi Geert-san,
> > 
> > Thank you for the reply again!
> > 
> > > Hi Shimoda-san,
> > >
> > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > According to the gadget.h, a "complete" function will always be called
> > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > is called with interrupts enabled. So, this function should calls
> > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > complete function.
> > >
> > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > this is usually an indicator of another locking problem.
> > 
> > I also think that I would like to avoid using local_irq_save().
> > But, I have no idea to resolve this issue for now.
> 
> After I modified usb-dmac driver to use a tasklet instead of a kthread,
> this issue disappeared.
> 
> My understanding is the followings:
> - According to the backtrace below, during usbhsf_dma_complete() was running,
>  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> 
> - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
>   to do the ncm_tx_tasklet().
> 
> - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
>   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> 
> 
> So, I would like to recall this patch and I will resubmit this patch series as v3.

try something like below:

diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index e0384af77e56..e9d75d85be59 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
 /*
  *		queue push/pop
  */
-static void usbhsg_queue_pop(struct usbhsg_uep *uep,
+static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
 			     struct usbhsg_request *ureq,
 			     int status)
 {
@@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
 	usb_gadget_giveback_request(&uep->ep, &ureq->req);
 }
 
+static void usbhsg_queue_pop(struct usbhsg_uep *uep,
+			     struct usbhsg_request *ureq,
+			     int status)
+{
+	usbhs_lock(priv, flags);
+	__usbhsg_queue_pop(uep, ureq, status);
+	usbhs_unlock(priv, flags);
+}
+
 static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
 {
 	struct usbhs_pipe *pipe = pkt->pipe;


then, for cases where lock is already held you call __usbhsg_queue_pop()
and for all other cases, call usbhsg_queue_pop().

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> > Hi Geert-san again,
> >
> > > Hi Geert-san,
> > >
> > > Thank you for the reply again!
> > >
> > > > Hi Shimoda-san,
> > > >
> > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > According to the gadget.h, a "complete" function will always be called
> > > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > > is called with interrupts enabled. So, this function should calls
> > > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > > complete function.
> > > >
> > > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > > this is usually an indicator of another locking problem.
> > >
> > > I also think that I would like to avoid using local_irq_save().
> > > But, I have no idea to resolve this issue for now.
> >
> > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > this issue disappeared.
> >
> > My understanding is the followings:
> > - According to the backtrace below, during usbhsf_dma_complete() was running,
> >  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> > http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> >
> > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
> >   to do the ncm_tx_tasklet().
> >
> > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
> >   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> >
> >
> > So, I would like to recall this patch and I will resubmit this patch series as v3.
> 
> try something like below:
> 
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index e0384af77e56..e9d75d85be59 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
>  /*
>   *		queue push/pop
>   */
> -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
>  			     struct usbhsg_request *ureq,
>  			     int status)
>  {
> @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
>  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
>  }
> 
> +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> +			     struct usbhsg_request *ureq,
> +			     int status)
> +{
> +	usbhs_lock(priv, flags);
> +	__usbhsg_queue_pop(uep, ureq, status);
> +	usbhs_unlock(priv, flags);
> +}
> +
>  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
>  {
>  	struct usbhs_pipe *pipe = pkt->pipe;
> 
> 
> then, for cases where lock is already held you call __usbhsg_queue_pop()
> and for all other cases, call usbhsg_queue_pop().

Thank you for the suggestion. However, we cannot use this usbhsg_queue_pop() because
a gadget driver might call usb_ep_queue() in the "complete" function and
this driver calls usbhs_lock() in the usb_ep_queue().

Perhaps my explanation was bad, but this issue was caused by the following conditions:
 - I use the renesas_usbhs driver as udc.
 - I use an old usb-dmac driver that the callback runs on a kthread.
 - I use the ncm driver. In this environment, the ncm driver might cause a spinlock suspected.

As an old solution, I fixed the renesas_usbhs driver by this patch.
However, if I use a new usb-dmac driver that the callback runs on a tasklet,
I don't need this patch. (This is a new solution.)

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Thu, Mar 12, 2015 at 05:40:56AM +0000, yoshihiro shimoda wrote:
> Hi,
> 
> > On Thu, Mar 12, 2015 at 04:33:41AM +0000, yoshihiro shimoda wrote:
> > > Hi Geert-san again,
> > >
> > > > Hi Geert-san,
> > > >
> > > > Thank you for the reply again!
> > > >
> > > > > Hi Shimoda-san,
> > > > >
> > > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > > According to the gadget.h, a "complete" function will always be called
> > > > > > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > > > > > is called with interrupts enabled. So, this function should calls
> > > > > > local_irq_save() before this calls the usb_gadget_giveback_request().
> > > > > > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > > > > > complete function.
> > > > >
> > > > > I still feel uneasy about adding the call to local_irq_save(), as the need for
> > > > > this is usually an indicator of another locking problem.
> > > >
> > > > I also think that I would like to avoid using local_irq_save().
> > > > But, I have no idea to resolve this issue for now.
> > >
> > > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > > this issue disappeared.
> > >
> > > My understanding is the followings:
> > > - According to the backtrace below, during usbhsf_dma_complete() was running,
> > >  a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> > > http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> > >
> > > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
> > >   to do the ncm_tx_tasklet().
> > >
> > > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a tasklet,
> > >   ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete() was running.
> > >
> > >
> > > So, I would like to recall this patch and I will resubmit this patch series as v3.
> > 
> > try something like below:
> > 
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > index e0384af77e56..e9d75d85be59 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> >  /*
> >   *		queue push/pop
> >   */
> > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> >  			     struct usbhsg_request *ureq,
> >  			     int status)
> >  {
> > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> >  }
> > 
> > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > +			     struct usbhsg_request *ureq,
> > +			     int status)
> > +{
> > +	usbhs_lock(priv, flags);
> > +	__usbhsg_queue_pop(uep, ureq, status);
> > +	usbhs_unlock(priv, flags);
> > +}
> > +
> >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> >  {
> >  	struct usbhs_pipe *pipe = pkt->pipe;
> > 
> > 
> > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > and for all other cases, call usbhsg_queue_pop().
> 
> Thank you for the suggestion. However, we cannot use this
> usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> in the "complete" function and this driver calls usbhs_lock() in the
> usb_ep_queue().

right, in that case just call __usbhs_queue_pop() directly.

> Perhaps my explanation was bad, but this issue was caused by the
> following conditions:
>  - I use the renesas_usbhs driver as udc.
>  - I use an old usb-dmac driver that the callback runs on a kthread.
>  - I use the ncm driver. In this environment, the ncm driver might
>	cause a spinlock suspected.
> 
> As an old solution, I fixed the renesas_usbhs driver by this patch.
> However, if I use a new usb-dmac driver that the callback runs on a
> tasklet, I don't need this patch. (This is a new solution.)

then differentiate based on some revision register or something like
that ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> Hi,
> 
> On Thu, Mar 12, 2015 at 05:40:56AM +0000, yoshihiro shimoda wrote:
> > Hi,
> >
< snip >
> > >
> > > try something like below:
> > >
> > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > index e0384af77e56..e9d75d85be59 100644
> > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > >  /*
> > >   *		queue push/pop
> > >   */
> > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > >  			     struct usbhsg_request *ureq,
> > >  			     int status)
> > >  {
> > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > >  }
> > >
> > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > +			     struct usbhsg_request *ureq,
> > > +			     int status)
> > > +{
> > > +	usbhs_lock(priv, flags);
> > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > +	usbhs_unlock(priv, flags);
> > > +}
> > > +
> > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > >  {
> > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > >
> > >
> > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > and for all other cases, call usbhsg_queue_pop().
> >
> > Thank you for the suggestion. However, we cannot use this
> > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > in the "complete" function and this driver calls usbhs_lock() in the
> > usb_ep_queue().
> 
> right, in that case just call __usbhs_queue_pop() directly.

Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
because this driver cannot know that a gadget driver calls usb_ep_queue() or not
in the "complete" function.

> > Perhaps my explanation was bad, but this issue was caused by the
> > following conditions:
> >  - I use the renesas_usbhs driver as udc.
> >  - I use an old usb-dmac driver that the callback runs on a kthread.
> >  - I use the ncm driver. In this environment, the ncm driver might
> >	cause a spinlock suspected.
> >
> > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > However, if I use a new usb-dmac driver that the callback runs on a
> > tasklet, I don't need this patch. (This is a new solution.)
> 
> then differentiate based on some revision register or something like
> that ?

Sorry for the lack information. I am submitting this usb-dmac driver that
the callback runs on a tasklet now. This means that the driver is not
merged yet. So, I think that we don't need to differentiate.

Best regards,
Yoshihiro Shimoda

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

Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Felipe Balbi]

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

Hi,

On Fri, Mar 13, 2015 at 01:14:01AM +0000, yoshihiro shimoda wrote:
> > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > index e0384af77e56..e9d75d85be59 100644
> > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > > >  /*
> > > >   *		queue push/pop
> > > >   */
> > > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  			     struct usbhsg_request *ureq,
> > > >  			     int status)
> > > >  {
> > > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > > >  }
> > > >
> > > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +			     struct usbhsg_request *ureq,
> > > > +			     int status)
> > > > +{
> > > > +	usbhs_lock(priv, flags);
> > > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > > +	usbhs_unlock(priv, flags);
> > > > +}
> > > > +
> > > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > > >  {
> > > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > > >
> > > >
> > > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > > and for all other cases, call usbhsg_queue_pop().
> > >
> > > Thank you for the suggestion. However, we cannot use this
> > > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > > in the "complete" function and this driver calls usbhs_lock() in the
> > > usb_ep_queue().
> > 
> > right, in that case just call __usbhs_queue_pop() directly.
> 
> Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
> because this driver cannot know that a gadget driver calls usb_ep_queue() or not
> in the "complete" function.

but you don't need to know that. All you have to do is spin_unlock()
before calling usb_gadget_giveback_request(), look at
drivers/usb/dwc3/gadget.c::dwc3_gadget_giveback()

> > > Perhaps my explanation was bad, but this issue was caused by the
> > > following conditions:
> > >  - I use the renesas_usbhs driver as udc.
> > >  - I use an old usb-dmac driver that the callback runs on a kthread.
> > >  - I use the ncm driver. In this environment, the ncm driver might
> > >	cause a spinlock suspected.
> > >
> > > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > > However, if I use a new usb-dmac driver that the callback runs on a
> > > tasklet, I don't need this patch. (This is a new solution.)
> > 
> > then differentiate based on some revision register or something like
> > that ?
> 
> Sorry for the lack information. I am submitting this usb-dmac driver that
> the callback runs on a tasklet now. This means that the driver is not
> merged yet. So, I think that we don't need to differentiate.

But as soon as your driver gets merged, you will need to differentiate,
right ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[yoshihiro shimoda]

Hi,

> Hi,
> 
> On Fri, Mar 13, 2015 at 01:14:01AM +0000, yoshihiro shimoda wrote:
> > > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > index e0384af77e56..e9d75d85be59 100644
> > > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > > > >  /*
> > > > >   *		queue push/pop
> > > > >   */
> > > > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > >  			     struct usbhsg_request *ureq,
> > > > >  			     int status)
> > > > >  {
> > > > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > > > >  }
> > > > >
> > > > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > > +			     struct usbhsg_request *ureq,
> > > > > +			     int status)
> > > > > +{
> > > > > +	usbhs_lock(priv, flags);
> > > > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > > > +	usbhs_unlock(priv, flags);
> > > > > +}
> > > > > +
> > > > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > > > >  {
> > > > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > > > >
> > > > >
> > > > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > > > and for all other cases, call usbhsg_queue_pop().
> > > >
> > > > Thank you for the suggestion. However, we cannot use this
> > > > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > > > in the "complete" function and this driver calls usbhs_lock() in the
> > > > usb_ep_queue().
> > >
> > > right, in that case just call __usbhs_queue_pop() directly.
> >
> > Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
> > because this driver cannot know that a gadget driver calls usb_ep_queue() or not
> > in the "complete" function.
> 
> but you don't need to know that. All you have to do is spin_unlock()
> before calling usb_gadget_giveback_request(), look at
> drivers/usb/dwc3/gadget.c::dwc3_gadget_giveback()

Thank you for the suggestion. I understood it.

> > > > Perhaps my explanation was bad, but this issue was caused by the
> > > > following conditions:
> > > >  - I use the renesas_usbhs driver as udc.
> > > >  - I use an old usb-dmac driver that the callback runs on a kthread.
> > > >  - I use the ncm driver. In this environment, the ncm driver might
> > > >	cause a spinlock suspected.
> > > >
> > > > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > > > However, if I use a new usb-dmac driver that the callback runs on a
> > > > tasklet, I don't need this patch. (This is a new solution.)
> > >
> > > then differentiate based on some revision register or something like
> > > that ?
> >
> > Sorry for the lack information. I am submitting this usb-dmac driver that
> > the callback runs on a tasklet now. This means that the driver is not
> > merged yet. So, I think that we don't need to differentiate.
> 
> But as soon as your driver gets merged, you will need to differentiate,
> right ?

Right. So, I will submit v3 patch soon.

Best regards,
Yoshihiro Shimoda

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