[PATCH v2] usb: gadget: eem: fix echo command packet response issue

[PATCH v2] usb: gadget: eem: fix echo command packet response issue

[Linyu Yuan]

From: Linyu Yuan <linyyuan@codeaurora.com>

when receive eem echo command, it will send a response,
but queue this response to the usb request which allocate
from gadget device endpoint zero,
and transmit the request to IN endpoint of eem interface.

on dwc3 gadget, it will trigger following warning in function
__dwc3_gadget_ep_queue(),

	if (WARN(req->dep != dep, "request %pK belongs to '%s'\n",
				&req->request, req->dep->name))
		return -EINVAL;

fix it by allocating a usb request from IN endpoint of eem interface,
and transmit the usb request to same IN endpoint of eem interface.

Signed-off-by: Linyu Yuan <linyyuan@codeaurora.com>
---

v2: fix mail format and expand commit message

 drivers/usb/gadget/function/f_eem.c | 44 +++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/f_eem.c b/drivers/usb/gadget/function/f_eem.c
index 2cd9942707b4..7de355c63189 100644
--- a/drivers/usb/gadget/function/f_eem.c
+++ b/drivers/usb/gadget/function/f_eem.c
@@ -30,6 +30,11 @@ struct f_eem {
 	u8				ctrl_id;
 };
 
+struct in_context {
+	struct sk_buff	*skb;
+	struct usb_ep	*ep;
+};
+
 static inline struct f_eem *func_to_eem(struct usb_function *f)
 {
 	return container_of(f, struct f_eem, port.func);
@@ -320,9 +325,12 @@ static int eem_bind(struct usb_configuration *c, struct usb_function *f)
 
 static void eem_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 {
-	struct sk_buff *skb = (struct sk_buff *)req->context;
+	struct in_context *ctx = req->context;
 
-	dev_kfree_skb_any(skb);
+	dev_kfree_skb_any(ctx->skb);
+	kfree(req->buf);
+	usb_ep_free_request(ctx->ep, req);
+	kfree(ctx);
 }
 
 /*
@@ -410,7 +418,9 @@ static int eem_unwrap(struct gether *port,
 		 * b15:		bmType (0 == data, 1 == command)
 		 */
 		if (header & BIT(15)) {
-			struct usb_request	*req = cdev->req;
+			struct usb_request	*req;
+			struct in_context	*ctx;
+			struct usb_ep		*ep;
 			u16			bmEEMCmd;
 
 			/* EEM command packet format:
@@ -439,13 +449,37 @@ static int eem_unwrap(struct gether *port,
 				skb_trim(skb2, len);
 				put_unaligned_le16(BIT(15) | BIT(11) | len,
 							skb_push(skb2, 2));
+
+				ep = port->in_ep;
+				req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+				if (!req)
+					goto freeskb;
+
+				ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+				if (!ctx)
+					goto freereq;
+				ctx->skb = skb2;
+				ctx->ep = ep;
+
+				req->buf = kmalloc(skb2->len, GFP_KERNEL);
+				if (!req->buf)
+					goto freectx;
+
 				skb_copy_bits(skb2, 0, req->buf, skb2->len);
 				req->length = skb2->len;
 				req->complete = eem_cmd_complete;
 				req->zero = 1;
-				req->context = skb2;
-				if (usb_ep_queue(port->in_ep, req, GFP_ATOMIC))
+				req->context = ctx;
+				if (usb_ep_queue(ep, req, GFP_ATOMIC)) {
 					DBG(cdev, "echo response queue fail\n");
+					kfree(req->buf);
+freectx:
+					kfree(ctx);
+freereq:
+					usb_ep_free_request(ep, req);
+freeskb:
+					dev_kfree_skb_any(skb2);
+				}
 				break;
 
 			case 1:  /* echo response */
-- 
2.25.1

Re: [PATCH v2] usb: gadget: eem: fix echo command packet response issue

[Greg Kroah-Hartman]

On Tue, Jun 15, 2021 at 07:41:33AM +0800, Linyu Yuan wrote:
> From: Linyu Yuan <linyyuan@codeaurora.com>
> 
> when receive eem echo command, it will send a response,
> but queue this response to the usb request which allocate
> from gadget device endpoint zero,
> and transmit the request to IN endpoint of eem interface.
> 
> on dwc3 gadget, it will trigger following warning in function
> __dwc3_gadget_ep_queue(),
> 
> 	if (WARN(req->dep != dep, "request %pK belongs to '%s'\n",
> 				&req->request, req->dep->name))
> 		return -EINVAL;
> 
> fix it by allocating a usb request from IN endpoint of eem interface,
> and transmit the usb request to same IN endpoint of eem interface.
> 
> Signed-off-by: Linyu Yuan <linyyuan@codeaurora.com>
> ---
> 
> v2: fix mail format and expand commit message
> 
>  drivers/usb/gadget/function/f_eem.c | 44 +++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_eem.c b/drivers/usb/gadget/function/f_eem.c
> index 2cd9942707b4..7de355c63189 100644
> --- a/drivers/usb/gadget/function/f_eem.c
> +++ b/drivers/usb/gadget/function/f_eem.c
> @@ -30,6 +30,11 @@ struct f_eem {
>  	u8				ctrl_id;
>  };
>  
> +struct in_context {
> +	struct sk_buff	*skb;
> +	struct usb_ep	*ep;
> +};
> +
>  static inline struct f_eem *func_to_eem(struct usb_function *f)
>  {
>  	return container_of(f, struct f_eem, port.func);
> @@ -320,9 +325,12 @@ static int eem_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  static void eem_cmd_complete(struct usb_ep *ep, struct usb_request *req)
>  {
> -	struct sk_buff *skb = (struct sk_buff *)req->context;
> +	struct in_context *ctx = req->context;
>  
> -	dev_kfree_skb_any(skb);
> +	dev_kfree_skb_any(ctx->skb);
> +	kfree(req->buf);
> +	usb_ep_free_request(ctx->ep, req);
> +	kfree(ctx);
>  }
>  
>  /*
> @@ -410,7 +418,9 @@ static int eem_unwrap(struct gether *port,
>  		 * b15:		bmType (0 == data, 1 == command)
>  		 */
>  		if (header & BIT(15)) {
> -			struct usb_request	*req = cdev->req;
> +			struct usb_request	*req;
> +			struct in_context	*ctx;
> +			struct usb_ep		*ep;
>  			u16			bmEEMCmd;
>  
>  			/* EEM command packet format:
> @@ -439,13 +449,37 @@ static int eem_unwrap(struct gether *port,
>  				skb_trim(skb2, len);
>  				put_unaligned_le16(BIT(15) | BIT(11) | len,
>  							skb_push(skb2, 2));
> +
> +				ep = port->in_ep;
> +				req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +				if (!req)
> +					goto freeskb;
> +
> +				ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> +				if (!ctx)
> +					goto freereq;
> +				ctx->skb = skb2;
> +				ctx->ep = ep;
> +
> +				req->buf = kmalloc(skb2->len, GFP_KERNEL);
> +				if (!req->buf)
> +					goto freectx;
> +
>  				skb_copy_bits(skb2, 0, req->buf, skb2->len);
>  				req->length = skb2->len;
>  				req->complete = eem_cmd_complete;
>  				req->zero = 1;
> -				req->context = skb2;
> -				if (usb_ep_queue(port->in_ep, req, GFP_ATOMIC))
> +				req->context = ctx;
> +				if (usb_ep_queue(ep, req, GFP_ATOMIC)) {
>  					DBG(cdev, "echo response queue fail\n");
> +					kfree(req->buf);
> +freectx:
> +					kfree(ctx);
> +freereq:
> +					usb_ep_free_request(ep, req);
> +freeskb:
> +					dev_kfree_skb_any(skb2);

As much fun as it is jumping to the middle of a case statement, are you
sure you want to maintain this?

And you do not return an error?  Why not?  What happens then?

thanks,

greg k-h