[PATCH v2 0/3] usb: cdns3: three bug fixes for v5.10

[PATCH v2 0/3] usb: cdns3: three bug fixes for v5.10

[Peter Chen]

Changes for v2:
- Move position of explicit cast to unsigned int before ((p) << 24) [Patch 1/3]
- No changes for other patches.

Pawel Laszczak (1):
  usb: cdns3: Fix on-chip memory overflow issue

Peter Chen (2):
  usb: cdns3: gadget: suspicious implicit sign extension
  usb: cdns3: gadget: own the lock wrongly at the suspend routine

 drivers/usb/cdns3/ep0.c    |  65 ++++++++++++----------
 drivers/usb/cdns3/gadget.c | 111 +++++++++++++++++++++----------------
 drivers/usb/cdns3/gadget.h |   5 +-
 3 files changed, 100 insertions(+), 81 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] usb: cdns3: gadget: suspicious implicit sign extension

[Peter Chen]

The code:
trb->length = cpu_to_le32(TRB_BURST_LEN(priv_ep->trb_burst_size)
	       	| TRB_LEN(length));

TRB_BURST_LEN(priv_ep->trb_burst_size) may be overflow for int 32 if
priv_ep->trb_burst_size is equal or larger than 0x80;

Below is the Coverity warning:
sign_extension: Suspicious implicit sign extension: priv_ep->trb_burst_size
with type u8 (8 bits, unsigned) is promoted in priv_ep->trb_burst_size << 24
to type int (32 bits, signed), then sign-extended to type unsigned long
(64 bits, unsigned). If priv_ep->trb_burst_size << 24 is greater than 0x7FFFFFFF,
the upper bits of the result will all be 1.

To fix it, it needs to add an explicit cast to unsigned int type for ((p) << 24).

Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
Changes for v2:
- Move position of explicit cast to unsigned int before ((p) << 24) [Patch 1/3]

 drivers/usb/cdns3/gadget.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
index 1ccecd237530..737377913788 100644
--- a/drivers/usb/cdns3/gadget.h
+++ b/drivers/usb/cdns3/gadget.h
@@ -1072,7 +1072,7 @@ struct cdns3_trb {
 #define TRB_TDL_SS_SIZE_GET(p)	(((p) & GENMASK(23, 17)) >> 17)
 
 /* transfer_len bitmasks - bits 31:24 */
-#define TRB_BURST_LEN(p)	(((p) << 24) & GENMASK(31, 24))
+#define TRB_BURST_LEN(p)	((unsigned int)((p) << 24) & GENMASK(31, 24))
 #define TRB_BURST_LEN_GET(p)	(((p) & GENMASK(31, 24)) >> 24)
 
 /* Data buffer pointer bitmasks*/
-- 
2.17.1

[PATCH v2 2/3] usb: cdns3: gadget: own the lock wrongly at the suspend routine

[Peter Chen]

When the system goes to suspend, if the controller is at device mode with
cable connecting to host, the call stack is: cdns3_suspend->
cdns3_gadget_suspend -> cdns3_disconnect_gadget, after cdns3_disconnect_gadget
is called, it owns lock wrongly, it causes the system being deadlock after
resume due to at cdns3_device_thread_irq_handler, it tries to get the lock,
but can't get it forever.

To fix it, we delete the unlock-lock operations at cdns3_disconnect_gadget,
and do it at the caller.

Fixes: b1234e3b3b26 ("usb: cdns3: add runtime PM support")
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/gadget.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 6ff3aa3db497..c127af6c0fe8 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1744,11 +1744,8 @@ static int cdns3_check_ep_interrupt_proceed(struct cdns3_endpoint *priv_ep)
 
 static void cdns3_disconnect_gadget(struct cdns3_device *priv_dev)
 {
-	if (priv_dev->gadget_driver && priv_dev->gadget_driver->disconnect) {
-		spin_unlock(&priv_dev->lock);
+	if (priv_dev->gadget_driver && priv_dev->gadget_driver->disconnect)
 		priv_dev->gadget_driver->disconnect(&priv_dev->gadget);
-		spin_lock(&priv_dev->lock);
-	}
 }
 
 /**
@@ -1783,7 +1780,9 @@ static void cdns3_check_usb_interrupt_proceed(struct cdns3_device *priv_dev,
 
 	/* Disconnection detected */
 	if (usb_ists & (USB_ISTS_DIS2I | USB_ISTS_DISI)) {
+		spin_unlock(&priv_dev->lock);
 		cdns3_disconnect_gadget(priv_dev);
+		spin_lock(&priv_dev->lock);
 		priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
 		usb_gadget_set_state(&priv_dev->gadget, USB_STATE_NOTATTACHED);
 		cdns3_hw_reset_eps_config(priv_dev);
@@ -3266,7 +3265,9 @@ static int cdns3_gadget_suspend(struct cdns3 *cdns, bool do_wakeup)
 {
 	struct cdns3_device *priv_dev = cdns->gadget_dev;
 
+	spin_unlock(&cdns->lock);
 	cdns3_disconnect_gadget(priv_dev);
+	spin_lock(&cdns->lock);
 
 	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
 	usb_gadget_set_state(&priv_dev->gadget, USB_STATE_NOTATTACHED);
-- 
2.17.1

[PATCH v2 3/3] usb: cdns3: Fix on-chip memory overflow issue

[Peter Chen]

From: Pawel Laszczak <pawell@cadence.com>

Patch fixes issue caused setting On-chip memory overflow bit in usb_sts
register. The issue occurred because EP_CFG register was set twice
before USB_STS.CFGSTS was set. Every write operation on EP_CFG.BUFFERING
causes that controller increases internal counter holding the number
of reserved on-chip buffers. First time this register was updated in
function cdns3_ep_config before delegating SET_CONFIGURATION request
to class driver and again it was updated when class wanted to enable
endpoint.  This patch fixes this issue by configuring endpoints
enabled by class driver in cdns3_gadget_ep_enable and others just
before status stage.

Cc: <stable@vger.kernel.org> #v5.8+
Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
Reported-and-tested-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/ep0.c    |  65 ++++++++++++-----------
 drivers/usb/cdns3/gadget.c | 102 +++++++++++++++++++++----------------
 drivers/usb/cdns3/gadget.h |   3 +-
 3 files changed, 94 insertions(+), 76 deletions(-)

diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
index 4761c852d9c4..d3121a32cc68 100644
--- a/drivers/usb/cdns3/ep0.c
+++ b/drivers/usb/cdns3/ep0.c
@@ -137,48 +137,36 @@ static int cdns3_req_ep0_set_configuration(struct cdns3_device *priv_dev,
 					   struct usb_ctrlrequest *ctrl_req)
 {
 	enum usb_device_state device_state = priv_dev->gadget.state;
-	struct cdns3_endpoint *priv_ep;
 	u32 config = le16_to_cpu(ctrl_req->wValue);
 	int result = 0;
-	int i;
 
 	switch (device_state) {
 	case USB_STATE_ADDRESS:
-		/* Configure non-control EPs */
-		for (i = 0; i < CDNS3_ENDPOINTS_MAX_COUNT; i++) {
-			priv_ep = priv_dev->eps[i];
-			if (!priv_ep)
-				continue;
-
-			if (priv_ep->flags & EP_CLAIMED)
-				cdns3_ep_config(priv_ep);
-		}
-
 		result = cdns3_ep0_delegate_req(priv_dev, ctrl_req);
 
-		if (result)
-			return result;
-
-		if (!config) {
-			cdns3_hw_reset_eps_config(priv_dev);
-			usb_gadget_set_state(&priv_dev->gadget,
-					     USB_STATE_ADDRESS);
-		}
+		if (result || !config)
+			goto reset_config;
 
 		break;
 	case USB_STATE_CONFIGURED:
 		result = cdns3_ep0_delegate_req(priv_dev, ctrl_req);
+		if (!config && !result)
+			goto reset_config;
 
-		if (!config && !result) {
-			cdns3_hw_reset_eps_config(priv_dev);
-			usb_gadget_set_state(&priv_dev->gadget,
-					     USB_STATE_ADDRESS);
-		}
 		break;
 	default:
-		result = -EINVAL;
+		return -EINVAL;
 	}
 
+	return 0;
+
+reset_config:
+	if (result != USB_GADGET_DELAYED_STATUS)
+		cdns3_hw_reset_eps_config(priv_dev);
+
+	usb_gadget_set_state(&priv_dev->gadget,
+			     USB_STATE_ADDRESS);
+
 	return result;
 }
 
@@ -705,6 +693,7 @@ static int cdns3_gadget_ep0_queue(struct usb_ep *ep,
 	unsigned long flags;
 	int ret = 0;
 	u8 zlp = 0;
+	int i;
 
 	spin_lock_irqsave(&priv_dev->lock, flags);
 	trace_cdns3_ep0_queue(priv_dev, request);
@@ -720,6 +709,17 @@ static int cdns3_gadget_ep0_queue(struct usb_ep *ep,
 		u32 val;
 
 		cdns3_select_ep(priv_dev, 0x00);
+
+		/*
+		 * Configure all non-control EPs which are not enabled by class driver
+		 */
+		for (i = 0; i < CDNS3_ENDPOINTS_MAX_COUNT; i++) {
+			priv_ep = priv_dev->eps[i];
+			if (priv_ep && priv_ep->flags & EP_CLAIMED &&
+			    !(priv_ep->flags & EP_ENABLED))
+				cdns3_ep_config(priv_ep, 0);
+		}
+
 		cdns3_set_hw_configuration(priv_dev);
 		cdns3_ep0_complete_setup(priv_dev, 0, 1);
 		/* wait until configuration set */
@@ -811,6 +811,7 @@ void cdns3_ep0_config(struct cdns3_device *priv_dev)
 	struct cdns3_usb_regs __iomem *regs;
 	struct cdns3_endpoint *priv_ep;
 	u32 max_packet_size = 64;
+	u32 ep_cfg;
 
 	regs = priv_dev->regs;
 
@@ -842,8 +843,10 @@ void cdns3_ep0_config(struct cdns3_device *priv_dev)
 				       BIT(0) | BIT(16));
 	}
 
-	writel(EP_CFG_ENABLE | EP_CFG_MAXPKTSIZE(max_packet_size),
-	       &regs->ep_cfg);
+	ep_cfg = EP_CFG_ENABLE | EP_CFG_MAXPKTSIZE(max_packet_size);
+
+	if (!(priv_ep->flags & EP_CONFIGURED))
+		writel(ep_cfg, &regs->ep_cfg);
 
 	writel(EP_STS_EN_SETUPEN | EP_STS_EN_DESCMISEN | EP_STS_EN_TRBERREN,
 	       &regs->ep_sts_en);
@@ -851,8 +854,10 @@ void cdns3_ep0_config(struct cdns3_device *priv_dev)
 	/* init ep in */
 	cdns3_select_ep(priv_dev, USB_DIR_IN);
 
-	writel(EP_CFG_ENABLE | EP_CFG_MAXPKTSIZE(max_packet_size),
-	       &regs->ep_cfg);
+	if (!(priv_ep->flags & EP_CONFIGURED))
+		writel(ep_cfg, &regs->ep_cfg);
+
+	priv_ep->flags |= EP_CONFIGURED;
 
 	writel(EP_STS_EN_SETUPEN | EP_STS_EN_TRBERREN, &regs->ep_sts_en);
 
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index c127af6c0fe8..5fa89baa53da 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -296,6 +296,8 @@ static void cdns3_ep_stall_flush(struct cdns3_endpoint *priv_ep)
  */
 void cdns3_hw_reset_eps_config(struct cdns3_device *priv_dev)
 {
+	int i;
+
 	writel(USB_CONF_CFGRST, &priv_dev->regs->usb_conf);
 
 	cdns3_allow_enable_l1(priv_dev, 0);
@@ -304,6 +306,10 @@ void cdns3_hw_reset_eps_config(struct cdns3_device *priv_dev)
 	priv_dev->out_mem_is_allocated = 0;
 	priv_dev->wait_for_setup = 0;
 	priv_dev->using_streams = 0;
+
+	for (i = 0; i < CDNS3_ENDPOINTS_MAX_COUNT; i++)
+		if (priv_dev->eps[i])
+			priv_dev->eps[i]->flags &= ~EP_CONFIGURED;
 }
 
 /**
@@ -1976,27 +1982,6 @@ static int cdns3_ep_onchip_buffer_reserve(struct cdns3_device *priv_dev,
 	return 0;
 }
 
-static void cdns3_stream_ep_reconfig(struct cdns3_device *priv_dev,
-				     struct cdns3_endpoint *priv_ep)
-{
-	if (!priv_ep->use_streams || priv_dev->gadget.speed < USB_SPEED_SUPER)
-		return;
-
-	if (priv_dev->dev_ver >= DEV_VER_V3) {
-		u32 mask = BIT(priv_ep->num + (priv_ep->dir ? 16 : 0));
-
-		/*
-		 * Stream capable endpoints are handled by using ep_tdl
-		 * register. Other endpoints use TDL from TRB feature.
-		 */
-		cdns3_clear_register_bit(&priv_dev->regs->tdl_from_trb, mask);
-	}
-
-	/*  Enable Stream Bit TDL chk and SID chk */
-	cdns3_set_register_bit(&priv_dev->regs->ep_cfg, EP_CFG_STREAM_EN |
-			       EP_CFG_TDL_CHK | EP_CFG_SID_CHK);
-}
-
 static void cdns3_configure_dmult(struct cdns3_device *priv_dev,
 				  struct cdns3_endpoint *priv_ep)
 {
@@ -2034,8 +2019,9 @@ static void cdns3_configure_dmult(struct cdns3_device *priv_dev,
 /**
  * cdns3_ep_config Configure hardware endpoint
  * @priv_ep: extended endpoint object
+ * @enable: set EP_CFG_ENABLE bit in ep_cfg register.
  */
-void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
+int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
 {
 	bool is_iso_ep = (priv_ep->type == USB_ENDPOINT_XFER_ISOC);
 	struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
@@ -2096,7 +2082,7 @@ void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
 		break;
 	default:
 		/* all other speed are not supported */
-		return;
+		return -EINVAL;
 	}
 
 	if (max_packet_size == 1024)
@@ -2106,11 +2092,33 @@ void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
 	else
 		priv_ep->trb_burst_size = 16;
 
-	ret = cdns3_ep_onchip_buffer_reserve(priv_dev, buffering + 1,
-					     !!priv_ep->dir);
-	if (ret) {
-		dev_err(priv_dev->dev, "onchip mem is full, ep is invalid\n");
-		return;
+	/* onchip buffer is only allocated before configuration */
+	if (!priv_dev->hw_configured_flag) {
+		ret = cdns3_ep_onchip_buffer_reserve(priv_dev, buffering + 1,
+						     !!priv_ep->dir);
+		if (ret) {
+			dev_err(priv_dev->dev, "onchip mem is full, ep is invalid\n");
+			return ret;
+		}
+	}
+
+	if (enable)
+		ep_cfg |= EP_CFG_ENABLE;
+
+	if (priv_ep->use_streams && priv_dev->gadget.speed >= USB_SPEED_SUPER) {
+		if (priv_dev->dev_ver >= DEV_VER_V3) {
+			u32 mask = BIT(priv_ep->num + (priv_ep->dir ? 16 : 0));
+
+			/*
+			 * Stream capable endpoints are handled by using ep_tdl
+			 * register. Other endpoints use TDL from TRB feature.
+			 */
+			cdns3_clear_register_bit(&priv_dev->regs->tdl_from_trb,
+						 mask);
+		}
+
+		/*  Enable Stream Bit TDL chk and SID chk */
+		ep_cfg |=  EP_CFG_STREAM_EN | EP_CFG_TDL_CHK | EP_CFG_SID_CHK;
 	}
 
 	ep_cfg |= EP_CFG_MAXPKTSIZE(max_packet_size) |
@@ -2120,9 +2128,12 @@ void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
 
 	cdns3_select_ep(priv_dev, bEndpointAddress);
 	writel(ep_cfg, &priv_dev->regs->ep_cfg);
+	priv_ep->flags |= EP_CONFIGURED;
 
 	dev_dbg(priv_dev->dev, "Configure %s: with val %08x\n",
 		priv_ep->name, ep_cfg);
+
+	return 0;
 }
 
 /* Find correct direction for HW endpoint according to description */
@@ -2263,7 +2274,7 @@ static int cdns3_gadget_ep_enable(struct usb_ep *ep,
 	u32 bEndpointAddress;
 	unsigned long flags;
 	int enable = 1;
-	int ret;
+	int ret = 0;
 	int val;
 
 	priv_ep = ep_to_cdns3_ep(ep);
@@ -2302,6 +2313,17 @@ static int cdns3_gadget_ep_enable(struct usb_ep *ep,
 	bEndpointAddress = priv_ep->num | priv_ep->dir;
 	cdns3_select_ep(priv_dev, bEndpointAddress);
 
+	/*
+	 * For some versions of controller at some point during ISO OUT traffic
+	 * DMA reads Transfer Ring for the EP which has never got doorbell.
+	 * This issue was detected only on simulation, but to avoid this issue
+	 * driver add protection against it. To fix it driver enable ISO OUT
+	 * endpoint before setting DRBL. This special treatment of ISO OUT
+	 * endpoints are recommended by controller specification.
+	 */
+	if (priv_ep->type == USB_ENDPOINT_XFER_ISOC  && !priv_ep->dir)
+		enable = 0;
+
 	if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) {
 		/*
 		 * Enable stream support (SS mode) related interrupts
@@ -2312,13 +2334,17 @@ static int cdns3_gadget_ep_enable(struct usb_ep *ep,
 				EP_STS_EN_SIDERREN | EP_STS_EN_MD_EXITEN |
 				EP_STS_EN_STREAMREN;
 			priv_ep->use_streams = true;
-			cdns3_stream_ep_reconfig(priv_dev, priv_ep);
+			ret = cdns3_ep_config(priv_ep, enable);
 			priv_dev->using_streams |= true;
 		}
+	} else {
+		ret = cdns3_ep_config(priv_ep, enable);
 	}
 
-	ret = cdns3_allocate_trb_pool(priv_ep);
+	if (ret)
+		goto exit;
 
+	ret = cdns3_allocate_trb_pool(priv_ep);
 	if (ret)
 		goto exit;
 
@@ -2348,20 +2374,6 @@ static int cdns3_gadget_ep_enable(struct usb_ep *ep,
 
 	writel(reg, &priv_dev->regs->ep_sts_en);
 
-	/*
-	 * For some versions of controller at some point during ISO OUT traffic
-	 * DMA reads Transfer Ring for the EP which has never got doorbell.
-	 * This issue was detected only on simulation, but to avoid this issue
-	 * driver add protection against it. To fix it driver enable ISO OUT
-	 * endpoint before setting DRBL. This special treatment of ISO OUT
-	 * endpoints are recommended by controller specification.
-	 */
-	if (priv_ep->type == USB_ENDPOINT_XFER_ISOC  && !priv_ep->dir)
-		enable = 0;
-
-	if (enable)
-		cdns3_set_register_bit(&priv_dev->regs->ep_cfg, EP_CFG_ENABLE);
-
 	ep->desc = desc;
 	priv_ep->flags &= ~(EP_PENDING_REQUEST | EP_STALLED | EP_STALL_PENDING |
 			    EP_QUIRK_ISO_OUT_EN | EP_QUIRK_EXTRA_BUF_EN);
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
index 737377913788..21fa461c518e 100644
--- a/drivers/usb/cdns3/gadget.h
+++ b/drivers/usb/cdns3/gadget.h
@@ -1159,6 +1159,7 @@ struct cdns3_endpoint {
 #define EP_QUIRK_EXTRA_BUF_DET	BIT(12)
 #define EP_QUIRK_EXTRA_BUF_EN	BIT(13)
 #define EP_TDLCHK_EN		BIT(15)
+#define EP_CONFIGURED		BIT(16)
 	u32			flags;
 
 	struct cdns3_request	*descmis_req;
@@ -1360,7 +1361,7 @@ void cdns3_gadget_giveback(struct cdns3_endpoint *priv_ep,
 int cdns3_init_ep0(struct cdns3_device *priv_dev,
 		   struct cdns3_endpoint *priv_ep);
 void cdns3_ep0_config(struct cdns3_device *priv_dev);
-void cdns3_ep_config(struct cdns3_endpoint *priv_ep);
+int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable);
 void cdns3_check_ep0_interrupt_proceed(struct cdns3_device *priv_dev, int dir);
 int __cdns3_gadget_wakeup(struct cdns3_device *priv_dev);
 
-- 
2.17.1

Re: [PATCH v2 2/3] usb: cdns3: gadget: own the lock wrongly at the suspend routine

[Felipe Balbi]

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

Peter Chen <peter.chen@nxp.com> writes:

> When the system goes to suspend, if the controller is at device mode with
> cable connecting to host, the call stack is: cdns3_suspend->
> cdns3_gadget_suspend -> cdns3_disconnect_gadget, after cdns3_disconnect_gadget
> is called, it owns lock wrongly, it causes the system being deadlock after
> resume due to at cdns3_device_thread_irq_handler, it tries to get the lock,
> but can't get it forever.
>
> To fix it, we delete the unlock-lock operations at cdns3_disconnect_gadget,
> and do it at the caller.
>
> Fixes: b1234e3b3b26 ("usb: cdns3: add runtime PM support")
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

comment from previous thread is still valid. Missing __releases() and
__acquires() annotation.

-- 
balbi

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

Re: [PATCH v2 3/3] usb: cdns3: Fix on-chip memory overflow issue

[Felipe Balbi]

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


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> From: Pawel Laszczak <pawell@cadence.com>
>
> Patch fixes issue caused setting On-chip memory overflow bit in usb_sts
> register. The issue occurred because EP_CFG register was set twice
> before USB_STS.CFGSTS was set. Every write operation on EP_CFG.BUFFERING
> causes that controller increases internal counter holding the number
> of reserved on-chip buffers. First time this register was updated in
> function cdns3_ep_config before delegating SET_CONFIGURATION request
> to class driver and again it was updated when class wanted to enable
> endpoint.  This patch fixes this issue by configuring endpoints
> enabled by class driver in cdns3_gadget_ep_enable and others just
> before status stage.
>
> Cc: <stable@vger.kernel.org> #v5.8+
> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> Reported-and-tested-by: Peter Chen <peter.chen@nxp.com>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

comment from previous thread is still valid. Far too extensive change to
qualify for stable or -rc. Sure there isn't a minimal patch possible?

-- 
balbi

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

RE: [PATCH v2 3/3] usb: cdns3: Fix on-chip memory overflow issue

[Pawel Laszczak]

Hi,

>
>Peter Chen <peter.chen@nxp.com> writes:
>> From: Pawel Laszczak <pawell@cadence.com>
>>
>> Patch fixes issue caused setting On-chip memory overflow bit in usb_sts
>> register. The issue occurred because EP_CFG register was set twice
>> before USB_STS.CFGSTS was set. Every write operation on EP_CFG.BUFFERING
>> causes that controller increases internal counter holding the number
>> of reserved on-chip buffers. First time this register was updated in
>> function cdns3_ep_config before delegating SET_CONFIGURATION request
>> to class driver and again it was updated when class wanted to enable
>> endpoint.  This patch fixes this issue by configuring endpoints
>> enabled by class driver in cdns3_gadget_ep_enable and others just
>> before status stage.
>>
>> Cc: <stable@vger.kernel.org> #v5.8+
>> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>> Reported-and-tested-by: Peter Chen <peter.chen@nxp.com>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> Signed-off-by: Peter Chen <peter.chen@nxp.com>
>
>comment from previous thread is still valid. Far too extensive change to
>qualify for stable or -rc. Sure there isn't a minimal patch possible?
>

I think that it's the only way to fix this issue. This fix require to displacement 
of code fragments and it's the reason why this patch is such extensive.

Regards,
Pawel
 

Re: [PATCH v2 0/3] usb: cdns3: three bug fixes for v5.10

[Greg KH]

On Thu, Oct 22, 2020 at 08:55:02AM +0800, Peter Chen wrote:
> Changes for v2:
> - Move position of explicit cast to unsigned int before ((p) << 24) [Patch 1/3]
> - No changes for other patches.
> 
> Pawel Laszczak (1):
>   usb: cdns3: Fix on-chip memory overflow issue
> 
> Peter Chen (2):
>   usb: cdns3: gadget: suspicious implicit sign extension
>   usb: cdns3: gadget: own the lock wrongly at the suspend routine

Are you going to send me patches to queue up for this driver any time
soon?  I've seen lots of different ones fly by, but no "please take
this" type of hint to me, so I have no idea what to do at all...

thanks,

greg k-h

RE: [PATCH v2 0/3] usb: cdns3: three bug fixes for v5.10

[Peter Chen]

 
> On Thu, Oct 22, 2020 at 08:55:02AM +0800, Peter Chen wrote:
> > Changes for v2:
> > - Move position of explicit cast to unsigned int before ((p) << 24)
> > [Patch 1/3]
> > - No changes for other patches.
> >
> > Pawel Laszczak (1):
> >   usb: cdns3: Fix on-chip memory overflow issue
> >
> > Peter Chen (2):
> >   usb: cdns3: gadget: suspicious implicit sign extension
> >   usb: cdns3: gadget: own the lock wrongly at the suspend routine
> 
> Are you going to send me patches to queue up for this driver any time soon?
> I've seen lots of different ones fly by, but no "please take this" type of hint to
> me, so I have no idea what to do at all...
> 

I will queue them up after reviewing, thanks.

Peter

Re: [PATCH v2 2/3] usb: cdns3: gadget: own the lock wrongly at the suspend routine

[Peter Chen]

On 20-10-27 11:26:56, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
> 
> > When the system goes to suspend, if the controller is at device mode with
> > cable connecting to host, the call stack is: cdns3_suspend->
> > cdns3_gadget_suspend -> cdns3_disconnect_gadget, after cdns3_disconnect_gadget
> > is called, it owns lock wrongly, it causes the system being deadlock after
> > resume due to at cdns3_device_thread_irq_handler, it tries to get the lock,
> > but can't get it forever.
> >
> > To fix it, we delete the unlock-lock operations at cdns3_disconnect_gadget,
> > and do it at the caller.
> >
> > Fixes: b1234e3b3b26 ("usb: cdns3: add runtime PM support")
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> 
> comment from previous thread is still valid. Missing __releases() and
> __acquires() annotation.
> 

Thanks, will add __must_hold sparse checker

-- 

Thanks,
Peter Chen