[PATCH 1/2] usb: mtu3: fix ep0's stall of out data stage

[PATCH 1/2] usb: mtu3: fix ep0's stall of out data stage

[Chunfeng Yun]

It happens when enable uvc function, the flow as below:
the controller switch to data stage, then call
    -> foward_to_driver() -> composite_setup() -> uvc_function_setup(),
it send out an event to user layer to notify it call
    -> ioctl() -> uvc_send_response() -> usb_ep_queue(),
but before the user call ioctl to queue ep0's buffer, the host already send
out data, but the controller find that no buffer is queued to receive data,
it send out STALL handshake.

To fix the issue, don't send out ACK of setup stage to switch to out data
stage until the buffer is available.

Cc: <Stable@vger.kernel.org>
Reported-by: Min Guo <min.guo@mediatek.com>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3.h            |  4 ++++
 drivers/usb/mtu3/mtu3_gadget_ep0.c | 22 +++++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
index 2d7b57e07eee..6b64ad17724d 100644
--- a/drivers/usb/mtu3/mtu3.h
+++ b/drivers/usb/mtu3/mtu3.h
@@ -318,6 +318,9 @@ static inline struct ssusb_mtk *dev_to_ssusb(struct device *dev)
  *		for GET_STATUS and SET_SEL
  * @setup_buf: ep0 response buffer for GET_STATUS and SET_SEL requests
  * @u3_capable: is capable of supporting USB3
+ * @delayed_setup: delay the setup stage to avoid STALL handshake in
+ *		out data stage due to the class driver doesn't queue buffer
+ *		before the host send out data
  */
 struct mtu3 {
 	spinlock_t lock;
@@ -360,6 +363,7 @@ struct mtu3 {
 	unsigned connected:1;
 	unsigned async_callbacks:1;
 	unsigned separate_fifo:1;
+	unsigned delayed_setup:1;
 
 	u8 address;
 	u8 test_mode_nr;
diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c b/drivers/usb/mtu3/mtu3_gadget_ep0.c
index e4fd1bb14a55..f7a71cc83e15 100644
--- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
+++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
@@ -162,6 +162,19 @@ static void ep0_do_status_stage(struct mtu3 *mtu)
 	mtu3_writel(mbase, U3D_EP0CSR, value | EP0_SETUPPKTRDY | EP0_DATAEND);
 }
 
+/* delay sending out ACK of setup stage to wait for OUT buffer queued */
+static void ep0_setup_stage_send_ack(struct mtu3 *mtu)
+{
+	void __iomem *mbase = mtu->mac_base;
+	u32 value;
+
+	if (mtu->delayed_setup) {
+		value = mtu3_readl(mbase, U3D_EP0CSR) & EP0_W1C_BITS;
+		mtu3_writel(mbase, U3D_EP0CSR, value | EP0_SETUPPKTRDY);
+		mtu->delayed_setup = 0;
+	}
+}
+
 static int ep0_queue(struct mtu3_ep *mep0, struct mtu3_request *mreq);
 
 static void ep0_dummy_complete(struct usb_ep *ep, struct usb_request *req)
@@ -628,8 +641,9 @@ static void ep0_read_setup(struct mtu3 *mtu, struct usb_ctrlrequest *setup)
 			csr | EP0_SETUPPKTRDY | EP0_DPHTX);
 		mtu->ep0_state = MU3D_EP0_STATE_TX;
 	} else {
-		mtu3_writel(mtu->mac_base, U3D_EP0CSR,
-			(csr | EP0_SETUPPKTRDY) & (~EP0_DPHTX));
+		mtu3_writel(mtu->mac_base, U3D_EP0CSR, csr & ~EP0_DPHTX);
+		/* send ACK when the buffer is queued */
+		mtu->delayed_setup = 1;
 		mtu->ep0_state = MU3D_EP0_STATE_RX;
 	}
 }
@@ -804,9 +818,11 @@ static int ep0_queue(struct mtu3_ep *mep, struct mtu3_request *mreq)
 
 	switch (mtu->ep0_state) {
 	case MU3D_EP0_STATE_SETUP:
-	case MU3D_EP0_STATE_RX:	/* control-OUT data */
 	case MU3D_EP0_STATE_TX:	/* control-IN data */
 		break;
+	case MU3D_EP0_STATE_RX:	/* control-OUT data */
+		ep0_setup_stage_send_ack(mtu);
+		break;
 	default:
 		dev_err(mtu->dev, "%s, error in ep0 state %s\n", __func__,
 			decode_ep0_state(mtu));
-- 
2.18.0

[PATCH 2/2] usb: mtu3: fix failed runtime suspend in host only mode

[Chunfeng Yun]

When the dr_mode is "host", after the host enter runtime suspend,
the mtu3 can't do it, because the mtu3's device wakeup function is
not enabled, instead it's enabled in gadget init function, to fix
the issue, init wakeup early in mtu3's probe()

Fixes: 6b587394c65c ("usb: mtu3: support suspend/resume for dual-role mode")
Reported-by: Tianping Fang <tianping.fang@mediatek.com>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_core.c | 2 --
 drivers/usb/mtu3/mtu3_plat.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_core.c b/drivers/usb/mtu3/mtu3_core.c
index 0ca173af87bb..a3a6282893d0 100644
--- a/drivers/usb/mtu3/mtu3_core.c
+++ b/drivers/usb/mtu3/mtu3_core.c
@@ -978,8 +978,6 @@ int ssusb_gadget_init(struct ssusb_mtk *ssusb)
 		goto irq_err;
 	}
 
-	device_init_wakeup(dev, true);
-
 	/* power down device IP for power saving by default */
 	mtu3_stop(mtu);
 
diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
index 4cb65346789d..d78ae52b4e26 100644
--- a/drivers/usb/mtu3/mtu3_plat.c
+++ b/drivers/usb/mtu3/mtu3_plat.c
@@ -356,6 +356,8 @@ static int mtu3_probe(struct platform_device *pdev)
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
+	device_init_wakeup(dev, true);
+
 	ret = ssusb_rscs_init(ssusb);
 	if (ret)
 		goto comm_init_err;
-- 
2.18.0

Re: [PATCH 1/2] usb: mtu3: fix ep0's stall of out data stage

[AngeloGioacchino Del Regno]

Il 28/09/22 11:17, Chunfeng Yun ha scritto:
> It happens when enable uvc function, the flow as below:
> the controller switch to data stage, then call
>      -> foward_to_driver() -> composite_setup() -> uvc_function_setup(),
> it send out an event to user layer to notify it call
>      -> ioctl() -> uvc_send_response() -> usb_ep_queue(),
> but before the user call ioctl to queue ep0's buffer, the host already send
> out data, but the controller find that no buffer is queued to receive data,
> it send out STALL handshake.
> 
> To fix the issue, don't send out ACK of setup stage to switch to out data
> stage until the buffer is available.
> 
> Cc: <Stable@vger.kernel.org>
> Reported-by: Min Guo <min.guo@mediatek.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>   drivers/usb/mtu3/mtu3.h            |  4 ++++
>   drivers/usb/mtu3/mtu3_gadget_ep0.c | 22 +++++++++++++++++++---
>   2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
> index 2d7b57e07eee..6b64ad17724d 100644
> --- a/drivers/usb/mtu3/mtu3.h
> +++ b/drivers/usb/mtu3/mtu3.h
> @@ -318,6 +318,9 @@ static inline struct ssusb_mtk *dev_to_ssusb(struct device *dev)
>    *		for GET_STATUS and SET_SEL
>    * @setup_buf: ep0 response buffer for GET_STATUS and SET_SEL requests
>    * @u3_capable: is capable of supporting USB3
> + * @delayed_setup: delay the setup stage to avoid STALL handshake in
> + *		out data stage due to the class driver doesn't queue buffer
> + *		before the host send out data
>    */
>   struct mtu3 {
>   	spinlock_t lock;
> @@ -360,6 +363,7 @@ struct mtu3 {
>   	unsigned connected:1;
>   	unsigned async_callbacks:1;
>   	unsigned separate_fifo:1;
> +	unsigned delayed_setup:1;
>   
>   	u8 address;
>   	u8 test_mode_nr;
> diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> index e4fd1bb14a55..f7a71cc83e15 100644
> --- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> +++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> @@ -162,6 +162,19 @@ static void ep0_do_status_stage(struct mtu3 *mtu)
>   	mtu3_writel(mbase, U3D_EP0CSR, value | EP0_SETUPPKTRDY | EP0_DATAEND);
>   }
>   
> +/* delay sending out ACK of setup stage to wait for OUT buffer queued */
> +static void ep0_setup_stage_send_ack(struct mtu3 *mtu)
> +{
> +	void __iomem *mbase = mtu->mac_base;
> +	u32 value;
> +
> +	if (mtu->delayed_setup) {
> +		value = mtu3_readl(mbase, U3D_EP0CSR) & EP0_W1C_BITS;
> +		mtu3_writel(mbase, U3D_EP0CSR, value | EP0_SETUPPKTRDY);
> +		mtu->delayed_setup = 0;
> +	}
> +}
> +
>   static int ep0_queue(struct mtu3_ep *mep0, struct mtu3_request *mreq);
>   
>   static void ep0_dummy_complete(struct usb_ep *ep, struct usb_request *req)
> @@ -628,8 +641,9 @@ static void ep0_read_setup(struct mtu3 *mtu, struct usb_ctrlrequest *setup)
>   			csr | EP0_SETUPPKTRDY | EP0_DPHTX);
>   		mtu->ep0_state = MU3D_EP0_STATE_TX;
>   	} else {
> -		mtu3_writel(mtu->mac_base, U3D_EP0CSR,
> -			(csr | EP0_SETUPPKTRDY) & (~EP0_DPHTX));
> +		mtu3_writel(mtu->mac_base, U3D_EP0CSR, csr & ~EP0_DPHTX);
> +		/* send ACK when the buffer is queued */
> +		mtu->delayed_setup = 1;

I don't think that you need this variable: you're calling the function
ep0_setup_stage_send_ack() only when ep0_state == MU3D_EP0_STATE_RX in
ep0_queue()...

..so you'll never get a call to ep0_setup_stage_send_ack() with delayed_setup == 0!

Regards,
Angelo

>   		mtu->ep0_state = MU3D_EP0_STATE_RX;
>   	}
>   }
> @@ -804,9 +818,11 @@ static int ep0_queue(struct mtu3_ep *mep, struct mtu3_request *mreq)
>   
>   	switch (mtu->ep0_state) {
>   	case MU3D_EP0_STATE_SETUP:
> -	case MU3D_EP0_STATE_RX:	/* control-OUT data */
>   	case MU3D_EP0_STATE_TX:	/* control-IN data */
>   		break;
> +	case MU3D_EP0_STATE_RX:	/* control-OUT data */
> +		ep0_setup_stage_send_ack(mtu);
> +		break;
>   	default:
>   		dev_err(mtu->dev, "%s, error in ep0 state %s\n", __func__,
>   			decode_ep0_state(mtu));

Re: [PATCH 2/2] usb: mtu3: fix failed runtime suspend in host only mode

[AngeloGioacchino Del Regno]

Il 28/09/22 11:17, Chunfeng Yun ha scritto:
> When the dr_mode is "host", after the host enter runtime suspend,
> the mtu3 can't do it, because the mtu3's device wakeup function is
> not enabled, instead it's enabled in gadget init function, to fix
> the issue, init wakeup early in mtu3's probe()
> 
> Fixes: 6b587394c65c ("usb: mtu3: support suspend/resume for dual-role mode")
> Reported-by: Tianping Fang <tianping.fang@mediatek.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 1/2] usb: mtu3: fix ep0's stall of out data stage

[Alan Stern]

On Wed, Sep 28, 2022 at 05:17:20PM +0800, Chunfeng Yun wrote:
> It happens when enable uvc function, the flow as below:
> the controller switch to data stage, then call
>     -> foward_to_driver() -> composite_setup() -> uvc_function_setup(),
> it send out an event to user layer to notify it call
>     -> ioctl() -> uvc_send_response() -> usb_ep_queue(),
> but before the user call ioctl to queue ep0's buffer, the host already send
> out data, but the controller find that no buffer is queued to receive data,
> it send out STALL handshake.
> 
> To fix the issue, don't send out ACK of setup stage to switch to out data
> stage until the buffer is available.

You might find it is better to use the delayed_status routines already 
present in the Gadget core.  Instead of delaying the response to the 
Setup packet of the second control transfer, delay the status response 
to the first control transfer.

This approach has the advantage of working even when the second transfer 
is not control but something else, such as bulk.

Also it agrees better with the way the USB spec intends control 
transfers to work.  The UDC is not supposed to complete the status stage 
of a control transfer until the gadget has fully processed the 
transfer's information and is ready to go forward.

Alan Stern

Re: [PATCH 1/2] usb: mtu3: fix ep0's stall of out data stage

[Chunfeng Yun]

On Wed, 2022-09-28 at 11:30 -0400, Alan Stern wrote:
> On Wed, Sep 28, 2022 at 05:17:20PM +0800, Chunfeng Yun wrote:
> > It happens when enable uvc function, the flow as below:
> > the controller switch to data stage, then call
> >     -> foward_to_driver() -> composite_setup() ->
> > uvc_function_setup(),
> > it send out an event to user layer to notify it call
> >     -> ioctl() -> uvc_send_response() -> usb_ep_queue(),
> > but before the user call ioctl to queue ep0's buffer, the host
> > already send
> > out data, but the controller find that no buffer is queued to
> > receive data,
> > it send out STALL handshake.
> > 
> > To fix the issue, don't send out ACK of setup stage to switch to
> > out data
> > stage until the buffer is available.
> 
> You might find it is better to use the delayed_status routines
> already 
> present in the Gadget core.  Instead of delaying the response to the 
> Setup packet of the second control transfer, delay the status
> response 
> to the first control transfer.
Ok, I'll try to use delayed_status to handle this issue.

Thanks a lot.

> 
> This approach has the advantage of working even when the second
> transfer 
> is not control but something else, such as bulk.
> 
> Also it agrees better with the way the USB spec intends control 
> transfers to work.  The UDC is not supposed to complete the status
> stage 
> of a control transfer until the gadget has fully processed the 
> transfer's information and is ready to go forward.
> 
> Alan Stern

Re: [PATCH 1/2] usb: mtu3: fix ep0's stall of out data stage

[Chunfeng Yun]

On Wed, 2022-09-28 at 15:43 +0200, AngeloGioacchino Del Regno wrote:
> Il 28/09/22 11:17, Chunfeng Yun ha scritto:
> > It happens when enable uvc function, the flow as below:
> > the controller switch to data stage, then call
> >      -> foward_to_driver() -> composite_setup() ->
> > uvc_function_setup(),
> > it send out an event to user layer to notify it call
> >      -> ioctl() -> uvc_send_response() -> usb_ep_queue(),
> > but before the user call ioctl to queue ep0's buffer, the host
> > already send
> > out data, but the controller find that no buffer is queued to
> > receive data,
> > it send out STALL handshake.
> > 
> > To fix the issue, don't send out ACK of setup stage to switch to
> > out data
> > stage until the buffer is available.
> > 
> > Cc: <Stable@vger.kernel.org>
> > Reported-by: Min Guo <min.guo@mediatek.com>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >   drivers/usb/mtu3/mtu3.h            |  4 ++++
> >   drivers/usb/mtu3/mtu3_gadget_ep0.c | 22 +++++++++++++++++++---
> >   2 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
> > index 2d7b57e07eee..6b64ad17724d 100644
> > --- a/drivers/usb/mtu3/mtu3.h
> > +++ b/drivers/usb/mtu3/mtu3.h
> > @@ -318,6 +318,9 @@ static inline struct ssusb_mtk
> > *dev_to_ssusb(struct device *dev)
> >    *		for GET_STATUS and SET_SEL
> >    * @setup_buf: ep0 response buffer for GET_STATUS and SET_SEL
> > requests
> >    * @u3_capable: is capable of supporting USB3
> > + * @delayed_setup: delay the setup stage to avoid STALL handshake
> > in
> > + *		out data stage due to the class driver doesn't queue
> > buffer
> > + *		before the host send out data
> >    */
> >   struct mtu3 {
> >   	spinlock_t lock;
> > @@ -360,6 +363,7 @@ struct mtu3 {
> >   	unsigned connected:1;
> >   	unsigned async_callbacks:1;
> >   	unsigned separate_fifo:1;
> > +	unsigned delayed_setup:1;
> >   
> >   	u8 address;
> >   	u8 test_mode_nr;
> > diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > index e4fd1bb14a55..f7a71cc83e15 100644
> > --- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > +++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > @@ -162,6 +162,19 @@ static void ep0_do_status_stage(struct mtu3
> > *mtu)
> >   	mtu3_writel(mbase, U3D_EP0CSR, value | EP0_SETUPPKTRDY |
> > EP0_DATAEND);
> >   }
> >   
> > +/* delay sending out ACK of setup stage to wait for OUT buffer
> > queued */
> > +static void ep0_setup_stage_send_ack(struct mtu3 *mtu)
> > +{
> > +	void __iomem *mbase = mtu->mac_base;
> > +	u32 value;
> > +
> > +	if (mtu->delayed_setup) {
> > +		value = mtu3_readl(mbase, U3D_EP0CSR) & EP0_W1C_BITS;
> > +		mtu3_writel(mbase, U3D_EP0CSR, value |
> > EP0_SETUPPKTRDY);
> > +		mtu->delayed_setup = 0;
> > +	}
> > +}
> > +
> >   static int ep0_queue(struct mtu3_ep *mep0, struct mtu3_request
> > *mreq);
> >   
> >   static void ep0_dummy_complete(struct usb_ep *ep, struct
> > usb_request *req)
> > @@ -628,8 +641,9 @@ static void ep0_read_setup(struct mtu3 *mtu,
> > struct usb_ctrlrequest *setup)
> >   			csr | EP0_SETUPPKTRDY | EP0_DPHTX);
> >   		mtu->ep0_state = MU3D_EP0_STATE_TX;
> >   	} else {
> > -		mtu3_writel(mtu->mac_base, U3D_EP0CSR,
> > -			(csr | EP0_SETUPPKTRDY) & (~EP0_DPHTX));
> > +		mtu3_writel(mtu->mac_base, U3D_EP0CSR, csr &
> > ~EP0_DPHTX);
> > +		/* send ACK when the buffer is queued */
> > +		mtu->delayed_setup = 1;
> 
> I don't think that you need this variable: you're calling the
> function
> ep0_setup_stage_send_ack() only when ep0_state == MU3D_EP0_STATE_RX
> in
> ep0_queue()...
> 
> ..so you'll never get a call to ep0_setup_stage_send_ack() with
> delayed_setup == 0!
I'll abandon this patch, and try to use delayed_status as suggested by
Alan, thanks a lot

> 
> Regards,
> Angelo
> 
> >   		mtu->ep0_state = MU3D_EP0_STATE_RX;
> >   	}
> >   }
> > @@ -804,9 +818,11 @@ static int ep0_queue(struct mtu3_ep *mep,
> > struct mtu3_request *mreq)
> >   
> >   	switch (mtu->ep0_state) {
> >   	case MU3D_EP0_STATE_SETUP:
> > -	case MU3D_EP0_STATE_RX:	/* control-OUT data */
> >   	case MU3D_EP0_STATE_TX:	/* control-IN data */
> >   		break;
> > +	case MU3D_EP0_STATE_RX:	/* control-OUT data */
> > +		ep0_setup_stage_send_ack(mtu);
> > +		break;
> >   	default:
> >   		dev_err(mtu->dev, "%s, error in ep0 state %s\n",
> > __func__,
> >   			decode_ep0_state(mtu));
> 
> 
>