Re: [PATCH] usb: musb: gadget: stall when SETUP packet size is invalid
While working on v2 - already figured out MUSB_CSR0_P_DATAEND bit also
needs to be set - I found a weird thing.
For debug purpose, I changed Line 803 in musb_gadget_ep0.c as
following to stall all SETUP packets.
-if (len != 8) {
+if (len == 8) {
Then the protocol analyzer shows total 16 CONTROL transfers, 12
GET_DESCRIPTOR, and 4 SET_ADDRESS, all have stall response from the
device. But the musb gadget received doubled of the transfers as in
the log below. Does anyone have any thought why the transfers are
doubled on musb side?
[ 24.936096] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 24.941955] packet: 80 06 00 01 00 00 40 00
[ 24.942230] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 24.948059] packet: 80 06 00 01 00 00 40 00
[ 24.948242] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 24.954040] packet: 80 06 00 01 00 00 40 00
[ 25.168090] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.173919] packet: 80 06 00 01 00 00 40 00
[ 25.174102] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.179931] packet: 80 06 00 01 00 00 40 00
[ 25.180114] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.185913] packet: 80 06 00 01 00 00 40 00
[ 25.512084] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.517913] packet: 80 06 00 01 00 00 40 00
[ 25.518127] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.523925] packet: 80 06 00 01 00 00 40 00
[ 25.524108] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.529937] packet: 80 06 00 01 00 00 40 00
[ 25.744079] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.749908] packet: 80 06 00 01 00 00 40 00
[ 25.750122] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.755920] packet: 80 06 00 01 00 00 40 00
[ 25.756103] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 25.761932] packet: 80 06 00 01 00 00 40 00
[ 26.088104] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 26.093933] packet: 00 05 45 00 00 00 00 00
[ 26.296081] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 26.301910] packet: 00 05 45 00 00 00 00 00
[ 26.616119] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 26.621948] packet: 00 05 46 00 00 00 00 00
[ 26.824096] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 26.829925] packet: 00 05 46 00 00 00 00 00
[ 27.357116] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 27.362945] packet: 80 06 00 01 00 00 40 00
[ 27.365112] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 27.370941] packet: 80 06 00 01 00 00 40 00
[ 27.373107] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 27.378936] packet: 80 06 00 01 00 00 40 00
[ 27.596130] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 27.601959] packet: 80 06 00 01 00 00 40 00
[ 27.604125] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 27.609954] packet: 80 06 00 01 00 00 40 00
[ 27.612121] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 27.617950] packet: 80 06 00 01 00 00 40 00
[ 28.001129] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.006988] packet: 80 06 00 01 00 00 40 00
[ 28.009124] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.014953] packet: 80 06 00 01 00 00 40 00
[ 28.017120] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.022949] packet: 80 06 00 01 00 00 40 00
[ 28.240142] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.245971] packet: 80 06 00 01 00 00 40 00
[ 28.248138] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.253967] packet: 80 06 00 01 00 00 40 00
[ 28.256134] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.261962] packet: 80 06 00 01 00 00 40 00
[ 28.589141] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.594970] packet: 00 05 6e 00 00 00 00 00
[ 28.800140] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 28.805969] packet: 00 05 6e 00 00 00 00 00
[ 29.177154] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 29.182983] packet: 00 05 6f 00 00 00 00 00
[ 29.388153] musb_g_ep0_irq 831: 0 SETUP packet len 8 != 8 ?
[ 29.393981] packet: 00 05 6f 00 00 00 00 00
Thanks,
-Bin.
On Fri, Nov 8, 2013 at 4:29 PM, Bin Liu wrote:
> On Fri, Nov 8, 2013 at 2:42 PM, Felipe Balbi wrote:
>> On Fri, Nov 08, 2013 at 02:20:24PM -0600, Bin Liu wrote:
>>
>> no commit log == no commit ;-)
>
> I am not sure what else needs to say other than the subject. But I
> will try to make some...
>
>>
>>> Signed-off-by: Bin Liu
>>> ---
>>> drivers/usb/musb/musb_gadget_ep0.c | 23 ++-
>>> 1 file changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/musb/musb_gadget_ep0.c
>>> b/drivers/usb/musb/musb_gadget_ep0.c
>>> index 3e9ec7c..f31b9b1 100644
>>> --- a/drivers/usb/musb/musb_gadget_ep0.c
>>> +++ b/drivers/usb/musb/musb_gadget_ep0.c
>>> @@ -664,6 +664,24 @@ __acquires(musb->lock)
>>> return retval;
>>> }
>>>
>>> +/* dump the EP0 fifo for debug */
>>> +static void musb_g_ep0_dump_fifo(struct musb *musb, int len)
>>> +{
>>
>> I would wrap this on #ifdef D
Re: [PATCH] usb: musb: gadget: stall when SETUP packet size is invalid
On Fri, Nov 08, 2013 at 04:29:23PM -0600, Bin Liu wrote: > On Fri, Nov 8, 2013 at 2:42 PM, Felipe Balbi wrote: > > On Fri, Nov 08, 2013 at 02:20:24PM -0600, Bin Liu wrote: > > > > no commit log == no commit ;-) > > I am not sure what else needs to say other than the subject. But I > will try to make some... For a start, say _why_ this is needed, not just what it does. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: gadget: stall when SETUP packet size is invalid
On Fri, Nov 8, 2013 at 2:42 PM, Felipe Balbi wrote:
> On Fri, Nov 08, 2013 at 02:20:24PM -0600, Bin Liu wrote:
>
> no commit log == no commit ;-)
I am not sure what else needs to say other than the subject. But I
will try to make some...
>
>> Signed-off-by: Bin Liu
>> ---
>> drivers/usb/musb/musb_gadget_ep0.c | 23 ++-
>> 1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/musb_gadget_ep0.c
>> b/drivers/usb/musb/musb_gadget_ep0.c
>> index 3e9ec7c..f31b9b1 100644
>> --- a/drivers/usb/musb/musb_gadget_ep0.c
>> +++ b/drivers/usb/musb/musb_gadget_ep0.c
>> @@ -664,6 +664,24 @@ __acquires(musb->lock)
>> return retval;
>> }
>>
>> +/* dump the EP0 fifo for debug */
>> +static void musb_g_ep0_dump_fifo(struct musb *musb, int len)
>> +{
>
> I would wrap this on #ifdef DEBUG
>
>> + u8 *t;
>
> u8 buf[20];
>
>> +
>> + if (len <= 0)
>> + return;
>> +
>> + t = kzalloc(len, GFP_KERNEL);
>
> this is called from IRQ handler, right ? It can't be GFP_KERNEL. Also,
> you might as well just have a 20 byte buffer in the stack itself. And
> only read up to 20 bytes, if it's more than 20 bytes we really don't
> need everything to know that the setup packet is definitely broken.
>
>> + if (!t)
>> + return;
>> +
>> + musb->ops->read_fifo(&musb->endpoints[0], len, t);
>
> ... read_fifo(&musb->endpointsp[0], min(len, 20), buf);
>
>> + print_hex_dump(KERN_ERR, "packet: ", DUMP_PREFIX_NONE,
>
> please make this KERN_DEBUG instead.
Agreed on all your comments. Will fix it in next version.
Thanks,
-Bin.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: gadget: stall when SETUP packet size is invalid
On Fri, Nov 08, 2013 at 02:20:24PM -0600, Bin Liu wrote:
no commit log == no commit ;-)
> Signed-off-by: Bin Liu
> ---
> drivers/usb/musb/musb_gadget_ep0.c | 23 ++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/musb_gadget_ep0.c
> b/drivers/usb/musb/musb_gadget_ep0.c
> index 3e9ec7c..f31b9b1 100644
> --- a/drivers/usb/musb/musb_gadget_ep0.c
> +++ b/drivers/usb/musb/musb_gadget_ep0.c
> @@ -664,6 +664,24 @@ __acquires(musb->lock)
> return retval;
> }
>
> +/* dump the EP0 fifo for debug */
> +static void musb_g_ep0_dump_fifo(struct musb *musb, int len)
> +{
I would wrap this on #ifdef DEBUG
> + u8 *t;
u8 buf[20];
> +
> + if (len <= 0)
> + return;
> +
> + t = kzalloc(len, GFP_KERNEL);
this is called from IRQ handler, right ? It can't be GFP_KERNEL. Also,
you might as well just have a 20 byte buffer in the stack itself. And
only read up to 20 bytes, if it's more than 20 bytes we really don't
need everything to know that the setup packet is definitely broken.
> + if (!t)
> + return;
> +
> + musb->ops->read_fifo(&musb->endpoints[0], len, t);
... read_fifo(&musb->endpointsp[0], min(len, 20), buf);
> + print_hex_dump(KERN_ERR, "packet: ", DUMP_PREFIX_NONE,
please make this KERN_DEBUG instead.
--
balbi
signature.asc
Description: Digital signature
[PATCH] usb: musb: gadget: stall when SETUP packet size is invalid
Signed-off-by: Bin Liu
---
drivers/usb/musb/musb_gadget_ep0.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_gadget_ep0.c
b/drivers/usb/musb/musb_gadget_ep0.c
index 3e9ec7c..f31b9b1 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -664,6 +664,24 @@ __acquires(musb->lock)
return retval;
}
+/* dump the EP0 fifo for debug */
+static void musb_g_ep0_dump_fifo(struct musb *musb, int len)
+{
+ u8 *t;
+
+ if (len <= 0)
+ return;
+
+ t = kzalloc(len, GFP_KERNEL);
+ if (!t)
+ return;
+
+ musb->ops->read_fifo(&musb->endpoints[0], len, t);
+ print_hex_dump(KERN_ERR, "packet: ", DUMP_PREFIX_NONE,
+ 16, 1, t, len, false);
+ kfree(t);
+}
+
/*
* Handle peripheral ep0 interrupt
*
@@ -813,7 +831,10 @@ setup:
if (len != 8) {
ERR("SETUP packet len %d != 8 ?\n", len);
- break;
+ musb_g_ep0_dump_fifo(musb, len);
+ musb->ackpend = MUSB_CSR0_P_SVDRXPKTRDY;
+ retval = IRQ_HANDLED;
+ goto stall;
}
musb_read_setup(musb, &setup);
retval = IRQ_HANDLED;
--
1.8.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
