linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] usb: gadget: aspeed: Bug fixes
@ 2019-07-06  0:53 Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 01/10] usb: gadget: aspeed: Don't set port enable change bit on reset Benjamin Herrenschmidt
                   ` (9 more replies)
  0 siblings, 10 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern

So I finally got back an Aspeed eval board, and thus resumed maintaining
and handling bug reports for this driver.

This is a series that fixes a number of enumeration related issues with
a variety of hosts, OSes, and circumstances (ie, plugging/unplugging at
funny times, etc...).

I also added dummy support for the TT requests, it doesn't hurt as some
host seem to send them even when TT support isn't advertized.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 01/10] usb: gadget: aspeed: Don't set port enable change bit on reset
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 02/10] usb: gadget: aspeed: Cleanup EP0 state on port reset Benjamin Herrenschmidt
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

This bit should be only set when the port enable goes down, for
example, on errors. Not when it gets set after a port reset. Some
USB stacks seem to be sensitive to this and fails enumeration.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/hub.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 7c040f56100e..0755115fd90d 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -449,8 +449,15 @@ static void ast_vhub_change_port_stat(struct ast_vhub *vhub,
 		       USB_PORT_STAT_C_OVERCURRENT |
 		       USB_PORT_STAT_C_RESET |
 		       USB_PORT_STAT_C_L1;
-		p->change |= chg;
 
+		/*
+		 * We only set USB_PORT_STAT_C_ENABLE if we are disabling
+		 * the port as per USB spec, otherwise MacOS gets upset
+		 */
+		if (p->status & USB_PORT_STAT_ENABLE)
+			chg &= ~USB_PORT_STAT_C_ENABLE;
+
+		p->change = chg;
 		ast_vhub_update_hub_ep1(vhub, port);
 	}
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 02/10] usb: gadget: aspeed: Cleanup EP0 state on port reset
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 01/10] usb: gadget: aspeed: Don't set port enable change bit on reset Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  8:11   ` Sergei Shtylyov
  2019-07-06  0:53 ` [PATCH 03/10] usb: gadget: aspeed: Fix EP0 stall handling Benjamin Herrenschmidt
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

Otherwise, we can have a stale state after a disconnect and reconnect
causing errors on the first SETUP packet to the device.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/dev.c  | 3 +++
 drivers/usb/gadget/udc/aspeed-vhub/ep0.c  | 9 +++++++++
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 1 +
 3 files changed, 13 insertions(+)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
index 6b1b16b17d7d..678bbdbd0971 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
@@ -55,6 +55,9 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev *d)
 	if (d->enabled)
 		return;
 
+	/* Cleanup EP0 state */
+	ast_vhub_reset_ep0(d);
+
 	/* Enable device and its EP0 interrupts */
 	reg = VHUB_DEV_EN_ENABLE_PORT |
 		VHUB_DEV_EN_EP0_IN_ACK_IRQEN |
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
index e2927fb083cf..5054c6343ead 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
@@ -459,6 +459,15 @@ static const struct usb_ep_ops ast_vhub_ep0_ops = {
 	.free_request	= ast_vhub_free_request,
 };
 
+void ast_vhub_reset_ep0(struct ast_vhub_dev *dev)
+{
+	struct ast_vhub_ep *ep = &dev->ep0;
+
+	ast_vhub_nuke(ep, -EIO);
+        ep->ep0.state = ep0_state_token;
+}
+
+
 void ast_vhub_init_ep0(struct ast_vhub *vhub, struct ast_vhub_ep *ep,
 		       struct ast_vhub_dev *dev)
 {
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
index 4ed03d33a5a9..2e7ef387f4f0 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
+++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
@@ -507,6 +507,7 @@ void ast_vhub_init_hw(struct ast_vhub *vhub);
 /* ep0.c */
 void ast_vhub_ep0_handle_ack(struct ast_vhub_ep *ep, bool in_ack);
 void ast_vhub_ep0_handle_setup(struct ast_vhub_ep *ep);
+void ast_vhub_reset_ep0(struct ast_vhub_dev *dev);
 void ast_vhub_init_ep0(struct ast_vhub *vhub, struct ast_vhub_ep *ep,
 		       struct ast_vhub_dev *dev);
 int ast_vhub_reply(struct ast_vhub_ep *ep, char *ptr, int len);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 03/10] usb: gadget: aspeed: Fix EP0 stall handling
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 01/10] usb: gadget: aspeed: Don't set port enable change bit on reset Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 02/10] usb: gadget: aspeed: Cleanup EP0 state on port reset Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 04/10] usb: gadget: aspeed: Improve debugging when nuking Benjamin Herrenschmidt
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

When stalling EP0, we need to wait for an ACK interrupt,
otherwise we may get out of sync on the next setup packet
data phase. Also we need to ignore the direction when
processing that interrupt as the HW reports a potential
mismatch.

Implement this by adding a stall state to EP0. This fixes
some reported issues with mass storage and some hosts.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/ep0.c  | 48 +++++++++++++++--------
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  1 +
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
index 5054c6343ead..d27d97b97b5c 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
@@ -105,18 +105,20 @@ void ast_vhub_ep0_handle_setup(struct ast_vhub_ep *ep)
 	       (crq.bRequestType & USB_DIR_IN) ? "in" : "out",
 	       ep->ep0.state);
 
-	/* Check our state, cancel pending requests if needed */
-	if (ep->ep0.state != ep0_state_token) {
+	/*
+	 * Check our state, cancel pending requests if needed
+	 *
+	 * Note: Under some circumstances, we can get a new setup
+	 * packet while waiting for the stall ack, just accept it.
+	 *
+	 * In any case, a SETUP packet in wrong state should have
+	 * reset the HW state machine, so let's just log, nuke
+	 * requests, move on.
+	 */
+	if (ep->ep0.state != ep0_state_token &&
+	    ep->ep0.state != ep0_state_stall) {
 		EPDBG(ep, "wrong state\n");
 		ast_vhub_nuke(ep, -EIO);
-
-		/*
-		 * Accept the packet regardless, this seems to happen
-		 * when stalling a SETUP packet that has an OUT data
-		 * phase.
-		 */
-		ast_vhub_nuke(ep, 0);
-		goto stall;
 	}
 
 	/* Calculate next state for EP0 */
@@ -165,7 +167,7 @@ void ast_vhub_ep0_handle_setup(struct ast_vhub_ep *ep)
  stall:
 	EPDBG(ep, "stalling\n");
 	writel(VHUB_EP0_CTRL_STALL, ep->ep0.ctlstat);
-	ep->ep0.state = ep0_state_status;
+	ep->ep0.state = ep0_state_stall;
 	ep->ep0.dir_in = false;
 	return;
 
@@ -299,8 +301,8 @@ void ast_vhub_ep0_handle_ack(struct ast_vhub_ep *ep, bool in_ack)
 		if ((ep->ep0.dir_in && (stat & VHUB_EP0_TX_BUFF_RDY)) ||
 		    (!ep->ep0.dir_in && (stat & VHUB_EP0_RX_BUFF_RDY)) ||
 		    (ep->ep0.dir_in != in_ack)) {
+			/* In that case, ignore interrupt */
 			dev_warn(dev, "irq state mismatch");
-			stall = true;
 			break;
 		}
 		/*
@@ -335,12 +337,22 @@ void ast_vhub_ep0_handle_ack(struct ast_vhub_ep *ep, bool in_ack)
 			dev_warn(dev, "status direction mismatch\n");
 			stall = true;
 		}
+		break;
+	case ep0_state_stall:
+		/*
+		 * There shouldn't be any request left, but nuke just in case
+		 * otherwise the stale request will block subsequent ones
+		 */
+		ast_vhub_nuke(ep, -EIO);
+		break;
 	}
 
-	/* Reset to token state */
-	ep->ep0.state = ep0_state_token;
-	if (stall)
+	/* Reset to token state or stall */
+	if (stall) {
 		writel(VHUB_EP0_CTRL_STALL, ep->ep0.ctlstat);
+		ep->ep0.state = ep0_state_stall;
+	} else
+		ep->ep0.state = ep0_state_token;
 }
 
 static int ast_vhub_ep0_queue(struct usb_ep* u_ep, struct usb_request *u_req,
@@ -390,8 +402,12 @@ static int ast_vhub_ep0_queue(struct usb_ep* u_ep, struct usb_request *u_req,
 	spin_lock_irqsave(&vhub->lock, flags);
 
 	/* EP0 can only support a single request at a time */
-	if (!list_empty(&ep->queue) || ep->ep0.state == ep0_state_token) {
+	if (!list_empty(&ep->queue) ||
+	    ep->ep0.state == ep0_state_token ||
+	    ep->ep0.state == ep0_state_stall) {
 		dev_warn(dev, "EP0: Request in wrong state\n");
+	        EPVDBG(ep, "EP0: list_empty=%d state=%d\n",
+		       list_empty(&ep->queue), ep->ep0.state);
 		spin_unlock_irqrestore(&vhub->lock, flags);
 		return -EBUSY;
 	}
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
index 2e7ef387f4f0..00f922604a1c 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
+++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
@@ -257,6 +257,7 @@ enum ep0_state {
 	ep0_state_token,
 	ep0_state_data,
 	ep0_state_status,
+	ep0_state_stall,
 };
 
 /*
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 04/10] usb: gadget: aspeed: Improve debugging when nuking
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
                   ` (2 preceding siblings ...)
  2019-07-06  0:53 ` [PATCH 03/10] usb: gadget: aspeed: Fix EP0 stall handling Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 05/10] usb: gadget: aspeed: Don't reject requests on suspended devices Benjamin Herrenschmidt
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

When nuking requests, it's useful to display how many were
actually nuked. It has proven handy when debugging issues
where EP0 went in a wrong state.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
index db3628be38c0..0c77cd488c48 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
@@ -65,13 +65,16 @@ void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req,
 void ast_vhub_nuke(struct ast_vhub_ep *ep, int status)
 {
 	struct ast_vhub_req *req;
-
-	EPDBG(ep, "Nuking\n");
+	int count = 0;
 
 	/* Beware, lock will be dropped & req-acquired by done() */
 	while (!list_empty(&ep->queue)) {
 		req = list_first_entry(&ep->queue, struct ast_vhub_req, queue);
 		ast_vhub_done(ep, req, status);
+		count++;
+	}
+	if (count) {
+		EPDBG(ep, "Nuked %d request(s)\n", count);
 	}
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 05/10] usb: gadget: aspeed: Don't reject requests on suspended devices
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
                   ` (3 preceding siblings ...)
  2019-07-06  0:53 ` [PATCH 04/10] usb: gadget: aspeed: Improve debugging when nuking Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 06/10] usb: gadget: aspeed: Check suspend/resume callback existence Benjamin Herrenschmidt
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

A disconnect may just suspend the hub in absence of a physical
disconnect detection. If we start rejecting requests, the mass
storage function gets into a spin trying to requeue the same
request for ever and hangs.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/dev.c | 13 +++++++++----
 drivers/usb/gadget/udc/aspeed-vhub/ep0.c |  2 +-
 drivers/usb/gadget/udc/aspeed-vhub/epn.c |  2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
index 678bbdbd0971..71e2416858fd 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
@@ -204,14 +204,19 @@ int ast_vhub_std_dev_request(struct ast_vhub_ep *ep,
 	u16 wValue, wIndex;
 
 	/* No driver, we shouldn't be enabled ... */
-	if (!d->driver || !d->enabled || d->suspended) {
+	if (!d->driver || !d->enabled) {
 		EPDBG(ep,
-		      "Device is wrong state driver=%p enabled=%d"
-		      " suspended=%d\n",
-		      d->driver, d->enabled, d->suspended);
+		      "Device is wrong state driver=%p enabled=%d\n",
+		      d->driver, d->enabled);
 		return std_req_stall;
 	}
 
+	/*
+	 * Note: we used to reject/stall requests while suspended,
+	 * we don't do that anymore as we seem to have cases of
+	 * mass storage getting very upset.
+	 */
+
 	/* First packet, grab speed */
 	if (d->gadget.speed == USB_SPEED_UNKNOWN) {
 		d->gadget.speed = ep->vhub->speed;
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
index d27d97b97b5c..8a17df19826b 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
@@ -379,7 +379,7 @@ static int ast_vhub_ep0_queue(struct usb_ep* u_ep, struct usb_request *u_req,
 		return -EINVAL;
 
 	/* Disabled device */
-	if (ep->dev && (!ep->dev->enabled || ep->dev->suspended))
+	if (ep->dev && !ep->dev->enabled)
 		return -ESHUTDOWN;
 
 	/* Data, no buffer and not internal ? */
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 35941dc125f9..7475c74aa5c5 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -352,7 +352,7 @@ static int ast_vhub_epn_queue(struct usb_ep* u_ep, struct usb_request *u_req,
 
 	/* Endpoint enabled ? */
 	if (!ep->epn.enabled || !u_ep->desc || !ep->dev || !ep->d_idx ||
-	    !ep->dev->enabled || ep->dev->suspended) {
+	    !ep->dev->enabled) {
 		EPDBG(ep, "Enqueuing request on wrong or disabled EP\n");
 		return -ESHUTDOWN;
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 06/10] usb: gadget: aspeed: Check suspend/resume callback existence
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
                   ` (4 preceding siblings ...)
  2019-07-06  0:53 ` [PATCH 05/10] usb: gadget: aspeed: Don't reject requests on suspended devices Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 07/10] usb: gadget: aspeed: Rework the reset logic Benjamin Herrenschmidt
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

.. before calling them

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
index 71e2416858fd..5f7e3b6de531 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
@@ -458,7 +458,7 @@ static const struct usb_gadget_ops ast_vhub_udc_ops = {
 void ast_vhub_dev_suspend(struct ast_vhub_dev *d)
 {
 	d->suspended = true;
-	if (d->driver) {
+	if (d->driver && d->driver->suspend) {
 		spin_unlock(&d->vhub->lock);
 		d->driver->suspend(&d->gadget);
 		spin_lock(&d->vhub->lock);
@@ -468,7 +468,7 @@ void ast_vhub_dev_suspend(struct ast_vhub_dev *d)
 void ast_vhub_dev_resume(struct ast_vhub_dev *d)
 {
 	d->suspended = false;
-	if (d->driver) {
+	if (d->driver && d->driver->resume) {
 		spin_unlock(&d->vhub->lock);
 		d->driver->resume(&d->gadget);
 		spin_lock(&d->vhub->lock);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 07/10] usb: gadget: aspeed: Rework the reset logic
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
                   ` (5 preceding siblings ...)
  2019-07-06  0:53 ` [PATCH 06/10] usb: gadget: aspeed: Check suspend/resume callback existence Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 08/10] usb: gadget: aspeed: Remove unused "suspended" flag Benjamin Herrenschmidt
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

We had some dodgy code using the speed setting to decide whether a
port reset would reset the device or just enable it.

Instead, if the device is disabled and has a gadget attached, a
reset will enable it. If it's already enabled, a reset will
reset it.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/dev.c | 58 +++++++++++-------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
index 5f7e3b6de531..79d3cb6bd2e7 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
@@ -50,7 +50,7 @@ void ast_vhub_dev_irq(struct ast_vhub_dev *d)
 
 static void ast_vhub_dev_enable(struct ast_vhub_dev *d)
 {
-	u32 reg, hmsk;
+	u32 reg, hmsk, i;
 
 	if (d->enabled)
 		return;
@@ -76,6 +76,20 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev *d)
 	/* Set EP0 DMA buffer address */
 	writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
 
+	/* Clear stall on all EPs */
+	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
+		struct ast_vhub_ep *ep = d->epns[i];
+
+		if (ep && (ep->epn.stalled || ep->epn.wedged)) {
+			ep->epn.stalled = false;
+			ep->epn.wedged = false;
+			ast_vhub_update_epn_stall(ep);
+		}
+	}
+
+	/* Additional cleanups */
+	d->wakeup_en = false;
+	d->suspended = false;
 	d->enabled = true;
 }
 
@@ -477,46 +491,28 @@ void ast_vhub_dev_resume(struct ast_vhub_dev *d)
 
 void ast_vhub_dev_reset(struct ast_vhub_dev *d)
 {
-	/*
-	 * If speed is not set, we enable the port. If it is,
-	 * send reset to the gadget and reset "speed".
-	 *
-	 * Speed is an indication that we have got the first
-	 * setup packet to the device.
-	 */
-	if (d->gadget.speed == USB_SPEED_UNKNOWN && !d->enabled) {
-		DDBG(d, "Reset at unknown speed of disabled device, enabling...\n");
-		ast_vhub_dev_enable(d);
-		d->suspended = false;
+	/* No driver, just disable the device and return */
+	if (!d->driver) {
+		ast_vhub_dev_disable(d);
+		return;
 	}
-	if (d->gadget.speed != USB_SPEED_UNKNOWN && d->driver) {
-		unsigned int i;
 
-		DDBG(d, "Reset at known speed of bound device, resetting...\n");
+	/* If the port isn't enabled, just enable it */
+	if (!d->enabled) {
+		DDBG(d, "Reset of disabled device, enabling...\n");
+		ast_vhub_dev_enable(d);
+	} else {
+		DDBG(d, "Reset of enabled device, resetting...\n");
 		spin_unlock(&d->vhub->lock);
-		d->driver->reset(&d->gadget);
+		usb_gadget_udc_reset(&d->gadget, d->driver);
 		spin_lock(&d->vhub->lock);
 
 		/*
-		 * Disable/re-enable HW, this will clear the address
+		 * Disable and maybe re-enable HW, this will clear the address
 		 * and speed setting.
 		 */
 		ast_vhub_dev_disable(d);
 		ast_vhub_dev_enable(d);
-
-		/* Clear stall on all EPs */
-		for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
-			struct ast_vhub_ep *ep = d->epns[i];
-
-			if (ep && ep->epn.stalled) {
-				ep->epn.stalled = false;
-				ast_vhub_update_epn_stall(ep);
-			}
-		}
-
-		/* Additional cleanups */
-		d->wakeup_en = false;
-		d->suspended = false;
 	}
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 08/10] usb: gadget: aspeed: Remove unused "suspended" flag
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
                   ` (6 preceding siblings ...)
  2019-07-06  0:53 ` [PATCH 07/10] usb: gadget: aspeed: Rework the reset logic Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 09/10] usb: Add definitions for the USB2.0 hub TT requests Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 10/10] usb: gadget: aspeed: Implement dummy " Benjamin Herrenschmidt
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

The state bit in the hub is sufficient

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/dev.c  | 4 ----
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 1 -
 2 files changed, 5 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
index 79d3cb6bd2e7..4008e7a51188 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
@@ -89,7 +89,6 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev *d)
 
 	/* Additional cleanups */
 	d->wakeup_en = false;
-	d->suspended = false;
 	d->enabled = true;
 }
 
@@ -110,7 +109,6 @@ static void ast_vhub_dev_disable(struct ast_vhub_dev *d)
 	writel(0, d->regs + AST_VHUB_DEV_EN_CTRL);
 	d->gadget.speed = USB_SPEED_UNKNOWN;
 	d->enabled = false;
-	d->suspended = false;
 }
 
 static int ast_vhub_dev_feature(struct ast_vhub_dev *d,
@@ -471,7 +469,6 @@ static const struct usb_gadget_ops ast_vhub_udc_ops = {
 
 void ast_vhub_dev_suspend(struct ast_vhub_dev *d)
 {
-	d->suspended = true;
 	if (d->driver && d->driver->suspend) {
 		spin_unlock(&d->vhub->lock);
 		d->driver->suspend(&d->gadget);
@@ -481,7 +478,6 @@ void ast_vhub_dev_suspend(struct ast_vhub_dev *d)
 
 void ast_vhub_dev_resume(struct ast_vhub_dev *d)
 {
-	d->suspended = false;
 	if (d->driver && d->driver->resume) {
 		spin_unlock(&d->vhub->lock);
 		d->driver->resume(&d->gadget);
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
index 00f922604a1c..761919e220d3 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
+++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
@@ -354,7 +354,6 @@ struct ast_vhub_dev {
 	struct usb_gadget_driver	*driver;
 	bool				registered : 1;
 	bool				wakeup_en : 1;
-	bool				suspended : 1;
 	bool				enabled : 1;
 
 	/* Endpoint structures */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 09/10] usb: Add definitions for the USB2.0 hub TT requests
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
                   ` (7 preceding siblings ...)
  2019-07-06  0:53 ` [PATCH 08/10] usb: gadget: aspeed: Remove unused "suspended" flag Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  2019-07-06  0:53 ` [PATCH 10/10] usb: gadget: aspeed: Implement dummy " Benjamin Herrenschmidt
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 include/linux/usb/hcd.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index bb57b5af4700..094ae7c71198 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -587,6 +587,10 @@ extern void usb_ep0_reinit(struct usb_device *);
 #define GetPortStatus		HUB_CLASS_REQ(USB_DIR_IN, USB_RT_PORT, USB_REQ_GET_STATUS)
 #define SetHubFeature		HUB_CLASS_REQ(USB_DIR_OUT, USB_RT_HUB, USB_REQ_SET_FEATURE)
 #define SetPortFeature		HUB_CLASS_REQ(USB_DIR_OUT, USB_RT_PORT, USB_REQ_SET_FEATURE)
+#define ClearTTBuffer		HUB_CLASS_REQ(USB_DIR_OUT, USB_RT_PORT, HUB_CLEAR_TT_BUFFER)
+#define ResetTT			HUB_CLASS_REQ(USB_DIR_OUT, USB_RT_PORT, HUB_RESET_TT)
+#define GetTTState		HUB_CLASS_REQ(USB_DIR_IN, USB_RT_PORT, HUB_GET_TT_STATE)
+#define StopTT			HUB_CLASS_REQ(USB_DIR_OUT, USB_RT_PORT, HUB_STOP_TT)
 
 
 /*-------------------------------------------------------------------------*/
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 10/10] usb: gadget: aspeed: Implement dummy hub TT requests
  2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
                   ` (8 preceding siblings ...)
  2019-07-06  0:53 ` [PATCH 09/10] usb: Add definitions for the USB2.0 hub TT requests Benjamin Herrenschmidt
@ 2019-07-06  0:53 ` Benjamin Herrenschmidt
  9 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06  0:53 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Joel Stanley, Alan Stern, Benjamin Herrenschmidt

We just accept them instead of stalling and return
zeros on GetTTState.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/udc/aspeed-vhub/hub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 0755115fd90d..19b3517e04c0 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -730,6 +730,12 @@ enum std_req_rc ast_vhub_class_hub_request(struct ast_vhub_ep *ep,
 	case ClearPortFeature:
 		EPDBG(ep, "ClearPortFeature(%d,%d)\n", wIndex & 0xf, wValue);
 		return ast_vhub_clr_port_feature(ep, wIndex & 0xf, wValue);
+	case ClearTTBuffer:
+	case ResetTT:
+	case StopTT:
+		return std_req_complete;
+	case GetTTState:
+		return ast_vhub_simple_reply(ep, 0, 0, 0, 0);
 	default:
 		EPDBG(ep, "Unknown class request\n");
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 02/10] usb: gadget: aspeed: Cleanup EP0 state on port reset
  2019-07-06  0:53 ` [PATCH 02/10] usb: gadget: aspeed: Cleanup EP0 state on port reset Benjamin Herrenschmidt
@ 2019-07-06  8:11   ` Sergei Shtylyov
  2019-07-06 12:34     ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 13+ messages in thread
From: Sergei Shtylyov @ 2019-07-06  8:11 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, linux-usb; +Cc: balbi, Joel Stanley, Alan Stern

Hello!

On 06.07.2019 3:53, Benjamin Herrenschmidt wrote:

> Otherwise, we can have a stale state after a disconnect and reconnect
> causing errors on the first SETUP packet to the device.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[...]
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> index e2927fb083cf..5054c6343ead 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> @@ -459,6 +459,15 @@ static const struct usb_ep_ops ast_vhub_ep0_ops = {
>   	.free_request	= ast_vhub_free_request,
>   };
>   
> +void ast_vhub_reset_ep0(struct ast_vhub_dev *dev)
> +{
> +	struct ast_vhub_ep *ep = &dev->ep0;
> +
> +	ast_vhub_nuke(ep, -EIO);
> +        ep->ep0.state = ep0_state_token;

    This line is indented with spaces, previous with a tab.

> +}
> +
> +
>   void ast_vhub_init_ep0(struct ast_vhub *vhub, struct ast_vhub_ep *ep,
>   		       struct ast_vhub_dev *dev)
>   {
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 02/10] usb: gadget: aspeed: Cleanup EP0 state on port reset
  2019-07-06  8:11   ` Sergei Shtylyov
@ 2019-07-06 12:34     ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-06 12:34 UTC (permalink / raw)
  To: Sergei Shtylyov, linux-usb; +Cc: balbi, Joel Stanley, Alan Stern

On Sat, 2019-07-06 at 11:11 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 06.07.2019 3:53, Benjamin Herrenschmidt wrote:
> 
> > Otherwise, we can have a stale state after a disconnect and
> > reconnect
> > causing errors on the first SETUP packet to the device.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> [...]
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> > index e2927fb083cf..5054c6343ead 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> > @@ -459,6 +459,15 @@ static const struct usb_ep_ops
> > ast_vhub_ep0_ops = {
> >   	.free_request	= ast_vhub_free_request,
> >   };
> >   
> > +void ast_vhub_reset_ep0(struct ast_vhub_dev *dev)
> > +{
> > +	struct ast_vhub_ep *ep = &dev->ep0;
> > +
> > +	ast_vhub_nuke(ep, -EIO);
> > +        ep->ep0.state = ep0_state_token;
> 
>     This line is indented with spaces, previous with a tab.

Thanks, will clean up.

Cheers,
Ben.
> 
> > +}
> > +
> > +
> >   void ast_vhub_init_ep0(struct ast_vhub *vhub, struct ast_vhub_ep
> > *ep,
> >   		       struct ast_vhub_dev *dev)
> >   {
> 
> [...]
> 
> MBR, Sergei


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-07-06 12:35 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-06  0:53 [PATCH 00/10] usb: gadget: aspeed: Bug fixes Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 01/10] usb: gadget: aspeed: Don't set port enable change bit on reset Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 02/10] usb: gadget: aspeed: Cleanup EP0 state on port reset Benjamin Herrenschmidt
2019-07-06  8:11   ` Sergei Shtylyov
2019-07-06 12:34     ` Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 03/10] usb: gadget: aspeed: Fix EP0 stall handling Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 04/10] usb: gadget: aspeed: Improve debugging when nuking Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 05/10] usb: gadget: aspeed: Don't reject requests on suspended devices Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 06/10] usb: gadget: aspeed: Check suspend/resume callback existence Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 07/10] usb: gadget: aspeed: Rework the reset logic Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 08/10] usb: gadget: aspeed: Remove unused "suspended" flag Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 09/10] usb: Add definitions for the USB2.0 hub TT requests Benjamin Herrenschmidt
2019-07-06  0:53 ` [PATCH 10/10] usb: gadget: aspeed: Implement dummy " Benjamin Herrenschmidt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).