Re: [PATCH] usb: gadget: uvc: Missing files for configfs interface

[Petr Cvek]

Dne 31.3.2017 v 11:01 Laurent Pinchart napsal(a):
> Hi Felipe and Petr,
> 
> On Tuesday 28 Mar 2017 16:48:46 Felipe Balbi wrote:
>> Petr Cvek <petr.c...@tul.cz> writes:
>>> Dne 7.3.2017 v 06:58 Laurent Pinchart napsal(a):
>>>> On Tuesday 07 Mar 2017 00:57:20 Petr Cvek wrote:
>>>>> Commit 76e0da34c7ce ("usb-gadget/uvc: use per-attribute show and store
>>>>> methods") caused a stringification of an undefined macro argument
>>>>> "aname", so three UVC parameters (streaming_interval,
>>>>> streaming_maxpacket and streaming_maxburst) were named "aname".
>>>>>
>>>>> Add the definition of "aname" to the main macro and name the filenames
>>>>> as originaly intended.
>>>>
>>>> Why don't you just
>>>>
>>>> - UVC_ATTR(f_uvc_opts_, cname, aname)
>>>> + UVC_ATTR(f_uvc_opts_, cname, cname)
>>>>
>>>> in the definition of the UVCG_OPTS_ATTR() macro ?
>>>
>>> Hi,
>>>
>>> In a fact I did it for my first testing version. But then I realized
>>> two things. First one is that someone may want to rename these three
>>> files (now or in the future). The second one is that this bug was
>>> caused by original author, who probably assumed the UVCG_OPTS_ATTR
>>> macro had "aname" argument as others UVCG_* macros and didn't check. I
>>> assumed that too and only after I saw three "aname" files with the
>>> same path I realized where is the problem.
>>>
>>> So it's more like a human error prone type of a code. But if you think
>>> "cname" is enough I can send PATCH v2.
> 
> I think it would be, otherwise we end up passing the same argument twice, 
> which seems a bit useless to me. If we ever need to rename those files we can 
> always change the code later.

... or if the variables, which need to be renamed (and userapi filenames to be 
kept).

> 
> It's no big deal, but I have a preference for my proposal.
> 

Any way I've sent your suggested version, you can choose whichever you like ;-).

cheers,
Petr
--
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

[PATCH v2] usb: gadget: uvc: Missing files for configfs interface

[Petr Cvek]

Commit 76e0da34c7ce ("usb-gadget/uvc: use per-attribute show and store
methods") caused a stringification of an undefined macro argument "aname",
so three UVC parameters (streaming_interval, streaming_maxpacket and
streaming_maxburst) were named "aname".

Fix the definition to use "cname", name of variables.

Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/function/uvc_configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index 4e037d2a7a60..5ce2ef324346 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2168,7 +2168,7 @@ end:  
\
return ret; \
 }  \
\
-UVC_ATTR(f_uvc_opts_, cname, aname)
+UVC_ATTR(f_uvc_opts_, cname, cname)
 
 #define identity_conv(x) (x)
 
-- 
2.11.0

--
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: [solution] usb: gadget: uvc: configfs: fails to enumerate in HOST

[Petr Cvek]

Dne 8.3.2017 v 11:46 Ganesh Biradar napsal(a):
> Hi Petr,
> 
> I saw your patch and at the same time I saw Laurent suggestion.
> 
> I have tried both patches both are working but I'm going with Laurent 
> suggestion
> 
> UVC_ATTR(f_uvc_opts_, cname, cname).
> 

That's OK, they are equivalent (as I explained in my reply to Laurent 
suggestion).

> I'm going with unofficial uvc-gadget.
> 
> Before I can test usb uvc gadget  not enumerating.

And there was recently reverted patch [1]. But if you are really using 4.4 then 
it should be fine with you.

[1] https://www.spinics.net/lists/linux-usb/msg154179.html

> 
> Regards,
> Ganesh Biradar
> 

Cheers,
Petr

--
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

[solution] usb: gadget: uvc: configfs: fails to enumerate in HOST

[Petr Cvek]

Hi,

I'm not subscribed to linux-usb mailing list so I found your post just by a 
pure luck. I've sent the patch for:

streaming_maxburst streaming_maxpacket streaming_interval

problem earlier this week. Try this patch [1]:

BTW please CC me if you will have another discusion in your thread (I will try 
to not forget it myself if I will continue in my thread).

P.S. Which userspace do you using, original uvc-gadget or unofficial repo [2]?

[1] https://www.spinics.net/lists/linux-usb/msg154417.html
[2] https://github.com/madscientist42/uvc-gadget

--
[PATCH] usb: gadget: uvc: Missing files for configfs interface

Commit 76e0da34c7ce ("usb-gadget/uvc: use per-attribute show and store
methods") caused a stringification of an undefined macro argument "aname",
so three UVC parameters (streaming_interval, streaming_maxpacket and
streaming_maxburst) were named "aname".

Add the definition of "aname" to the main macro and name the filenames as
originaly intended.

Signed-off-by: Petr Cvek <petr.cvek@xx>
---
 drivers/usb/gadget/function/uvc_configfs.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index 4e037d2a7a60..3a36b2e85788 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2125,7 +2125,7 @@ static struct configfs_item_operations uvc_item_ops = {
.release= uvc_attr_release,
 };
 
-#define UVCG_OPTS_ATTR(cname, conv, str2u, uxx, vnoc, limit)   \
+#define UVCG_OPTS_ATTR(cname, aname, conv, str2u, uxx, vnoc, limit)\
 static ssize_t f_uvc_opts_##cname##_show(  \
struct config_item *item, char *page)   \
 {  \
@@ -2172,12 +2172,12 @@ UVC_ATTR(f_uvc_opts_, cname, aname)
 
 #define identity_conv(x) (x)
 
-UVCG_OPTS_ATTR(streaming_interval, identity_conv, kstrtou8, u8, identity_conv,
-  16);
-UVCG_OPTS_ATTR(streaming_maxpacket, le16_to_cpu, kstrtou16, u16, le16_to_cpu,
-  3072);
-UVCG_OPTS_ATTR(streaming_maxburst, identity_conv, kstrtou8, u8, identity_conv,
-  15);
+UVCG_OPTS_ATTR(streaming_interval, streaming_interval, identity_conv,
+  kstrtou8, u8, identity_conv, 16);
+UVCG_OPTS_ATTR(streaming_maxpacket, streaming_maxpacket, le16_to_cpu,
+  kstrtou16, u16, le16_to_cpu, 3072);
+UVCG_OPTS_ATTR(streaming_maxburst, streaming_maxburst, identity_conv,
+  kstrtou8, u8, identity_conv, 15);
 
 #undef identity_conv
--
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

usb: gadget: pxa27x_udc: race condition with g_webcam

[Petr Cvek]

Hi,

During the testing of the g_webcam on PXA27x (HTC Magician machine) I found a 
race condition. In the situations when the UDC receives a SETUP DATA packet 
sooner than it is "requested" by usb_ep_queue() it will block itself from the 
reading the FIFO.

Below I suggest the fix, but it is very dirty code (so just RFC) on a very 
buggy hardware (which I understand poorly even after reading comments [1] in 
the code :-D ). And I only repaired a path for the SETUP OUT packets. So any 
improvements are welcomed.

The call tree is:

USB host generates a SETUP packet and sends it to the PXA27x UDC
IRQ
handle_ep0()
handle_ep0_ctrl_req() - sets PXA UDC stage to OUT_DATA_STAGE
uvc_function_setup()
v4l2_event_queue() - event for userspace driver
... userspace driver (uvc-gadget.c) can sleep, or just takes long to 
proceed

... meanwhile USB host sends OUT packet
IRQ
handle_ep0()
case OUT_DATA_STAGE:
if (epout_has_pkt(ep) && req)
//fails as there is no req queued yet (==NULL)
interrupt gets masked

ioctl UVCIOC_SEND_RESPONSE - if there is a data stage
... switch to kernel
uvc_send_response()
usb_ep_queue()
pxa_ep_queue()
case OUT_DATA_STAGE:
if ((length == 0) || !epout_has_pkt(ep))
//fails, there is length, EP0 has a packet

Now there is an unreceived OUT packet stuck in the FIFO and the PXA UDC will 
never call usb_gadget_giveback_request() and the subsequent 
uvc_function_ep0_complete(). Which means the userspace will not get 
UVC_EVENT_DATA enqueued by v4l2_event_queue() and the host uvcvideo will 
timeout after a while.

I was able to make the g_webcam work after a few changes.

First, forcing the driver to read the packet, if there are data.

pxa_ep_queue():
case OUT_DATA_STAGE:
-   if ((length == 0) || !epout_has_pkt(ep))
+   if ((length == 0) || epout_has_pkt(ep))

... which is probably a bad idea in the long run. BTW why PXA UDC is trying to 
read empty FIFO? Or it is some clever call to handshake something (or some HW 
workaround, there is something here [1])?

This change caused an unwanted interrupt if there are multiple packet to be 
received (UVC SETUP can have like 26 bytes of data) which would mess the state 
machine, so I wrapped the condition inside an irq spinlock:

case OUT_DATA_STAGE:
spin_lock_irqsave(>lock, flags);
if ((length == 0) || epout_has_pkt(ep)){
if (read_ep0_fifo(ep, req)) {
ep0_end_out_req(ep, req, ); //must UNLOCK 
inside
}
}
spin_unlock_irqrestore(>lock, flags);
break;

and in read_ep0_fifo() I've put a mask on the interrupt (UDC got a packet).

while (epout_has_pkt(ep)) {
count = read_packet(ep, req);
ep_write_UDCCSR(ep, UDCCSR0_OPC);
+   udc_writel(ep->dev, UDCISR0, 1);
inc_ep_stats_bytes(ep, count, !USB_DIR_IN);

Now the OUT packet can be read (bypassing IRQ handler), but the state machine 
can bite itself in the ass if a new SETUP packet is received:

ep0_end_out_req()
ep_end_out_req()
req_done() - enabled interrupts
usb_gadget_giveback_request()
uvc_function_ep0_complete()
v4l2_event_queue() - UVC_EVENT_DATA to userspace

if the interrupt is generated now it will be in the state OUT_STATUS_STAGE, 
because:

static void ep0_end_out_req(...)
{
set_ep0state(ep->dev, OUT_STATUS_STAGE);
ep_end_out_req(ep, req, pflags);
ep0_idle(ep->dev);
}

... this cause handle_ep0() to warn with:

"should never get in OUT_STATUS_STAGE state here!!!"
-> ep0_idle()

although this is just a warning it can be averted by just switching to 
WAIT_FOR_SETUP before usb_gadget_giveback_request().

With these changes I was able to send .jpg images with a modified (320x240, 
JPEG) g_webcam and with a modified uvc-gadget based on this commit [2].

P.S. This modification works with g_ether too.
P.P.S. I had to add g_webcam endpoints definitions too (first and second 
arguments depends on an actual configuration).
/*g_webcam*/
PXA_EP_IN_INT(7,1, 3, 0, 0),
PXA_EP_IN_ISO(8,4, 3, 1, 1),

[1] 
http://elixir.free-electrons.com/source/drivers/usb/gadget/udc/pxa27x_udc.c?v=4.10#L2004
[2] 
https://github.com/madscientist42/uvc-gadget/commit/e127ec3a3022e1090b3b741b48f661670f5dade2

--
Regards,
Petr
--
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

[PATCH] usb: gadget: uvc: Missing files for configfs interface

[Petr Cvek]

Commit 76e0da34c7ce ("usb-gadget/uvc: use per-attribute show and store
methods") caused a stringification of an undefined macro argument "aname",
so three UVC parameters (streaming_interval, streaming_maxpacket and
streaming_maxburst) were named "aname".

Add the definition of "aname" to the main macro and name the filenames as
originaly intended.

Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/function/uvc_configfs.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index 4e037d2a7a60..3a36b2e85788 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2125,7 +2125,7 @@ static struct configfs_item_operations uvc_item_ops = {
.release= uvc_attr_release,
 };
 
-#define UVCG_OPTS_ATTR(cname, conv, str2u, uxx, vnoc, limit)   \
+#define UVCG_OPTS_ATTR(cname, aname, conv, str2u, uxx, vnoc, limit)\
 static ssize_t f_uvc_opts_##cname##_show(  \
struct config_item *item, char *page)   \
 {  \
@@ -2172,12 +2172,12 @@ UVC_ATTR(f_uvc_opts_, cname, aname)
 
 #define identity_conv(x) (x)
 
-UVCG_OPTS_ATTR(streaming_interval, identity_conv, kstrtou8, u8, identity_conv,
-  16);
-UVCG_OPTS_ATTR(streaming_maxpacket, le16_to_cpu, kstrtou16, u16, le16_to_cpu,
-  3072);
-UVCG_OPTS_ATTR(streaming_maxburst, identity_conv, kstrtou8, u8, identity_conv,
-  15);
+UVCG_OPTS_ATTR(streaming_interval, streaming_interval, identity_conv,
+  kstrtou8, u8, identity_conv, 16);
+UVCG_OPTS_ATTR(streaming_maxpacket, streaming_maxpacket, le16_to_cpu,
+  kstrtou16, u16, le16_to_cpu, 3072);
+UVCG_OPTS_ATTR(streaming_maxburst, streaming_maxburst, identity_conv,
+  kstrtou8, u8, identity_conv, 15);
 
 #undef identity_conv
 
-- 
2.11.0

--
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: gadget: uvc: Missing files for configfs interface

[Petr Cvek]

Dne 7.3.2017 v 06:58 Laurent Pinchart napsal(a):
> Hi Petr,
> 
> Thank you for the patch.
> 
> On Tuesday 07 Mar 2017 00:57:20 Petr Cvek wrote:
>> Commit 76e0da34c7ce ("usb-gadget/uvc: use per-attribute show and store
>> methods") caused a stringification of an undefined macro argument "aname",
>> so three UVC parameters (streaming_interval, streaming_maxpacket and
>> streaming_maxburst) were named "aname".
>>
>> Add the definition of "aname" to the main macro and name the filenames as
>> originaly intended.
> 
> Why don't you just 
> 
> - UVC_ATTR(f_uvc_opts_, cname, aname)
> + UVC_ATTR(f_uvc_opts_, cname, cname)
> 
> in the definition of the UVCG_OPTS_ATTR() macro ?

Hi,

In a fact I did it for my first testing version. But then I realized two 
things. First one is that someone may want to rename these three files (now or 
in the future). The second one is that this bug was caused by original author, 
who probably assumed the UVCG_OPTS_ATTR macro had "aname" argument as others 
UVCG_* macros and didn't check. I assumed that too and only after I saw three 
"aname" files with the same path I realized where is the problem.

So it's more like a human error prone type of a code. But if you think "cname" 
is enough I can send PATCH v2.

Petr
--
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: usb: gadget: webcam broken?

[Petr Cvek]

Dne 2.3.2017 v 09:19 Roger Quadros napsal(a):
> Petr,
> 
> On 02/03/17 06:57, Petr Cvek wrote:
>> Dne 2.3.2017 v 00:22 Laurent Pinchart napsal(a):
>>> Hi Roger,
>>>
>>> On Wednesday 01 Mar 2017 17:09:51 Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> I'm no longer able to use g_webcam with uvc-gadget [1] since v4.9. Logs at
>>>> the end. It looks like we're goofing up on the control endpoint.
>>>>
>>>> If I revert the following commit everything works fine.
>>>> commit 4fbac5206afd01b717d4bdc58793d471f3391b4b
>>>> Author: Petr Cvek <petr.c...@tul.cz>
>>>> Date:   Wed Aug 17 12:36:57 2016 +0200
>>>>
>>>> usb: gadget: uvc: Add missing call for additional setup data
>>>>
>>>> Am I missing something on uvc-gadget side or is the commit really bad?
>>>> From what I understand, uvc-gadget is responsible for sending response to
>>>> UVC class specific requests on control endpoint in uvc_send_response()
>>>> in uvc_v4l2.c.
>>>>
>>>> So the reported commit is sending a duplicate response with probably
>>>> improper data.
>>>
>>> Yes, this looks very dubious to me. I think it should be reverted. My 
>>> apologies for not having caught the patch during review.
>>
>> Hi,
>>
>> Now I've watched all codepaths again and yeah it is probably wrong patch, 
>> sorry.
>>
>> But if the code path is really:
>>
>> uvc_function_setup() -> userspace setup -> ioctl UVCIOC_SEND_RESPONSE -> 
>> uvc_send_response() -> usb_ep_queue() -> uvc_function_ep0_complete() -> 
>> userspace data
>>
>> it seems the USB timeouts with my hardware (PXA27x UDC) but with my patch it 
>> gets response immediately.
>>
> 
> I hope you were running uvc-gadget application on the PXA27x.
> 
> Just sending a response is not sufficient. It must send a response with 
> proper data.
> f_uvc itself doesn't know how to handle UVC class specific requests and has to
> depend on the user space application to populate the data in the response.
> 

Hi,

Yeah it was on the PXA27x, but that callback is not working there, so I was 
mistaken the bug is in UVC and not in PXA27x UDC itself.

Anyway I was using a newer (unofficial [1]) version of uvc-gadget, which 
already implements the most of the UVC requests.

BTW Did you try to use a configfs method for your usb webcam application? There 
was a discussion [2] about removing legacy UDC stuff (g_webcam and others g_*) 
in the future. If you used it, does it create these files for you?
streaming_interval
streaming_maxpacket
streaming_maxburst

P.S. Are you planning to use some generic linux friendly USB VID/PID? ;-)

[1] 
https://github.com/madscientist42/uvc-gadget/commit/e127ec3a3022e1090b3b741b48f661670f5dade2
[2] http://www.spinics.net/lists/linux-usb/msg152678.html

--
cheers,
Petr

--
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: [RFC] usb: gadget: uvc: webcam gadget USB PID is using value from a different gadget

[Petr Cvek]

Dne 24.1.2017 v 19:20 Felipe Balbi napsal(a):
> 
> Hi,
> 
> Petr Cvek <petr.c...@tul.cz> writes:
>>> Greg KH <g...@kroah.com> writes:
>>>>>>> fine by me. Just lsusb will look funky ;-)
>>>>>>
>>>>>> Heh, true, but I thought lsusb would use a string if the device provided
>>>>>> it.  Haven't looked at that portion of the code in a very long time...
>>>>>>
>>>>>
>>>>> My lsusb shows separate strings (using usbutils from slackware64-current):
>>>>>
>>>>> Bus 004 Device 003: ID 1d6b:0102 Linux Foundation EEM Gadget
>>>>> ...
>>>>>   idVendor   0x1d6b Linux Foundation
>>>>>   idProduct  0x0102 EEM Gadget
>>>>>   bcdDevice4.07
>>>>>   iManufacturer   1 Linux Foundation
>>>>>   iProduct2 Webcam gadget
>>>>> ...
>>>>
>>>> Ah, I guess it doesn't, but who knows how old that version of usbutils
>>>> is, considering the last release I did was well over a year ago.  I
>>>> should do a new one one of these days...
>>>>
>>>> Anyway, I'd like to not assign a product id to a chunk of code that is
>>>> going to be eventually deleted.  Felipe, what's the plan for the
>>>> "legacy" gadget code.  Is it ever going away?
>>>
>>> Well, I wasn't really planning on deleting them just stopped accepting
>>> any new one. I wanted to avoid angry mobs complaining about not having a
>>> g_mass_storage.ko anymore.
>>>
>>> Personally, I don't feel strongly about the legacy gadget
>>> drivers. They're not really needed anymore as everything they do can be
>>> done with configfs already. Perhaps we could schedule their removal for
>>> v5.0?
>>>
>>
>> If you want to remove legacy g_webcam then there should be a way to
>> set its module parameters from somewhere (it seems I can't find it
>> anywhere). For PXA27x especially "opts->streaming_maxpacket" from
>> f_uvc.c is critical. Default max packet size in PXA27x UDC (but
>> lowering the limit to 256 by g_webcam parameter works).
> 
> it should be part of the function's configfs interface. If it's not,
> please send a patch. Have a look at
> Documentation/usb/gadget_configfs.txt for more info.
> 

Hi 

sorry for a late answer (it took so much time to get my machine to the stage 
with working UVC :-/ ). It is really not working and I think I found why. Will 
send a patch soon.

Petr
--
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: usb: gadget: webcam broken?

[Petr Cvek]

Dne 2.3.2017 v 00:22 Laurent Pinchart napsal(a):
> Hi Roger,
> 
> On Wednesday 01 Mar 2017 17:09:51 Roger Quadros wrote:
>> Hi,
>>
>> I'm no longer able to use g_webcam with uvc-gadget [1] since v4.9. Logs at
>> the end. It looks like we're goofing up on the control endpoint.
>>
>> If I revert the following commit everything works fine.
>> commit 4fbac5206afd01b717d4bdc58793d471f3391b4b
>> Author: Petr Cvek <petr.c...@tul.cz>
>> Date:   Wed Aug 17 12:36:57 2016 +0200
>>
>> usb: gadget: uvc: Add missing call for additional setup data
>>
>> Am I missing something on uvc-gadget side or is the commit really bad?
>> From what I understand, uvc-gadget is responsible for sending response to
>> UVC class specific requests on control endpoint in uvc_send_response()
>> in uvc_v4l2.c.
>>
>> So the reported commit is sending a duplicate response with probably
>> improper data.
> 
> Yes, this looks very dubious to me. I think it should be reverted. My 
> apologies for not having caught the patch during review.

Hi,

Now I've watched all codepaths again and yeah it is probably wrong patch, sorry.

But if the code path is really:

uvc_function_setup() -> userspace setup -> ioctl UVCIOC_SEND_RESPONSE -> 
uvc_send_response() -> usb_ep_queue() -> uvc_function_ep0_complete() -> 
userspace data

it seems the USB timeouts with my hardware (PXA27x UDC) but with my patch it 
gets response immediately.

Petr

--
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

[PATCH v2 2/2] usb: gadget: pxa27x: Test for a valid argument pointer

[Petr Cvek]

A call usb_put_phy(udc->transceiver) must be tested for a valid pointer.
Use an already existing test for usb_unregister_notifier call.

Reported-by: Robert Jarzmik <robert.jarz...@free.fr>
Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/udc/pxa27x_udc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c 
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 821bd8f4cae6..d48e239660c3 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2531,9 +2531,10 @@ static int pxa_udc_remove(struct platform_device *_dev)
usb_del_gadget_udc(>gadget);
pxa_cleanup_debugfs(udc);
 
-   if (!IS_ERR_OR_NULL(udc->transceiver))
+   if (!IS_ERR_OR_NULL(udc->transceiver)) {
usb_unregister_notifier(udc->transceiver, _udc_phy);
-   usb_put_phy(udc->transceiver);
+   usb_put_phy(udc->transceiver);
+   }
 
udc->transceiver = NULL;
the_controller = NULL;
-- 
2.11.0

--
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

[PATCH v2 1/2] usb: gadget: pxa27x: Remove duplicate function prototype

[Petr Cvek]

Functions udc_enable() and udc_disable() have a duplicated prototype.
Remove it.

Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/udc/pxa27x_udc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c 
b/drivers/usb/gadget/udc/pxa27x_udc.c
index e1335ad5bce9..821bd8f4cae6 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -1608,9 +1608,6 @@ static int pxa_udc_pullup(struct usb_gadget *_gadget, int 
is_active)
return 0;
 }
 
-static void udc_enable(struct pxa_udc *udc);
-static void udc_disable(struct pxa_udc *udc);
-
 /**
  * pxa_udc_vbus_session - Called by external transceiver to enable/disable udc
  * @_gadget: usb gadget
-- 
2.11.0

--
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

[PATCH v2 0/2] usb: gadget: pxa27x: Duplicated definition and valid pointer fixes

[Petr Cvek]

Two simple fixes for pxa27x gadget UDC. First one remove function declaration
duplication, second one add test for a valid pointer before function call as
it can be called with an error value in the pointer.

Changes from v1:
 * Shorter titles, corrected Robert Jarzmik's signature

Petr Cvek (2):
  usb: gadget: pxa27x: Remove duplicate function prototype
  usb: gadget: pxa27x: Test for a valid argument pointer

 drivers/usb/gadget/udc/pxa27x_udc.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

-- 
2.11.0

--
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

[PATCH 1/2] [TRIVIAL] usb: gadget: Remove duplicated function declaration in pxa27x_udc

[Petr Cvek]

Remove duplicated function declaration for udc_enable() and udc_disable().

Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/udc/pxa27x_udc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c 
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 7fa60f5b7ae4..6fa675bf2c6f 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -1608,9 +1608,6 @@ static int pxa_udc_pullup(struct usb_gadget *_gadget, int 
is_active)
return 0;
 }
 
-static void udc_enable(struct pxa_udc *udc);
-static void udc_disable(struct pxa_udc *udc);
-
 /**
  * pxa_udc_vbus_session - Called by external transceiver to enable/disable udc
  * @_gadget: usb gadget
-- 
2.11.0

--
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: [RFC] usb: gadget: scheduling while atomic in pxa27x_udc, IS_ERR_OR_NULL test, duplicated definition

[Petr Cvek]

Dne 22.2.2017 v 21:46 Robert Jarzmik napsal(a):
> Petr Cvek <petr.c...@tul.cz> writes:
> 
> Hi Petr,
> 
>> I found a few problems with the PXA27x UDC.
>>
>>  usb_function_activate() in drivers/usb/gadget/composite.c
>>
>> does spin_lock_irqsave() and then calls 
>>
>>  gadget->ops->pullup() in drivers/usb/gadget/udc/core.c
>>
>> which is set to pxa_udc_pullup(), which should be called not in interrupt
>>
>>  /**
>>   * pxa_udc_pullup - Offer manual D+ pullup control
>>   * @_gadget: usb gadget using the control
>>   * @is_active: 0 if disconnect, else connect D+ pullup resistor
>>   * Context: !in_interrupt()
>>   *
>>   * Returns 0 if OK, -EOPNOTSUPP if udc driver doesn't handle D+ pullup
>>   */
>>
>> This finally causes fail at
>>
>>  udc_enable() in drivers/usb/gadget/udc/pxa27x_udc.c
>>  
>> at code
>>
>>  /*
>>   * Caller must be able to sleep in order to cope with startup transients
>>   */
>>  msleep(100);
>>
>> with a following error (with CONFIG_DEBUG_PREEMPT on):
>>
>>  BUG: scheduling while atomic: v4l_id/360/0x0002
>>
>> With the msleep changed to mdelay, the code (specified as !in_interrupt()) 
>> seems to work fine
>> (after torture reloads). Can the caller (udc core) be changed to be able to
>> sleep?
> 
> This question is for Felipe. From the several years back when I wrote this 
> code
> I think it was granted that the pullup() callback was not in interrupt 
> context,
> but Felipe knows better.
> 

Well now it is definitely inside spin_lock_irqsave()

http://lxr.free-electrons.com/source/drivers/usb/gadget/composite.c#L374


>>  if (of_have_populated_dt()) {
>>  udc->transceiver =
>>  devm_usb_get_phy_by_phandle(udc->dev, "phys", 0);
>>
>>  if (IS_ERR(udc->transceiver))
>>  return PTR_ERR(udc->transceiver);
>>  } else {
>>  udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
>>  }
>>
>> One branch returns on error and second one is fine, udc->transceiver then 
>> can hold an error,
>> but this is fine for the rest of driver (tested). Question is does it have 
>> to return from a first
>> branch (e.g. my device does not have phy)?
> In the devicetree context (first branch), even if you don't have a phy, you'll
> instanciate a "nop" phy, ie. "usb-nop-xceiv". From a hardware perspective, you
> have a phy, it's just that you don't have to do anything from a software
> perspective to activate your phy.
> 
> In the platform_data context (second branch), an error is the common path, as
> there is no phy in many old platforms, and pxa27x are old ... hence no check 
> of
> error.
> 

OK so no problem there.

>> And finally it seems definitions from drivers/usb/gadget/udc/pxa27x_udc.c 
>> are duplicated:
>>
>>  static void udc_enable(struct pxa_udc *udc);
>>  static void udc_disable(struct pxa_udc *udc);
>>
>> I will send patch series as soon as we agree on solutions.
> Excellent.

OK mdelay/msleep problem will take longer, so I will send that two patches now.

Petr

--
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

[PATCH 0/2] usb: gadget: Fixes for pxa27x_udc

[Petr Cvek]

Two simple fixes for pxa27x gadget UDC. First one remove function declaration
duplication, second one add test for a valid pointer before function call as
it can be called with an error value in the pointer.

Petr Cvek (2):
  [TRIVIAL] usb: gadget: Remove duplicated function declaration in
pxa27x_udc
  usb: gadget: Add test if argument pointer has a valid value in
pxa27x_udc

 drivers/usb/gadget/udc/pxa27x_udc.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

-- 
2.11.0

--
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

[PATCH 2/2] usb: gadget: Add test if argument pointer has a valid value in pxa27x_udc

[Petr Cvek]

Move call usb_put_phy(udc->transceiver) inside a valid pointer test.

Reported-by: robert.jarz...@free.fr
Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/udc/pxa27x_udc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c 
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 6fa675bf2c6f..9d2e1a8aa69d 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2531,9 +2531,10 @@ static int pxa_udc_remove(struct platform_device *_dev)
usb_del_gadget_udc(>gadget);
pxa_cleanup_debugfs(udc);
 
-   if (!IS_ERR_OR_NULL(udc->transceiver))
+   if (!IS_ERR_OR_NULL(udc->transceiver)) {
usb_unregister_notifier(udc->transceiver, _udc_phy);
-   usb_put_phy(udc->transceiver);
+   usb_put_phy(udc->transceiver);
+   }
 
udc->transceiver = NULL;
the_controller = NULL;
-- 
2.11.0

--
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

[RFC] usb: gadget: scheduling while atomic in pxa27x_udc, IS_ERR_OR_NULL test, duplicated definition

[Petr Cvek]

Hi,

I found a few problems with the PXA27x UDC.

usb_function_activate() in drivers/usb/gadget/composite.c

does spin_lock_irqsave() and then calls 

gadget->ops->pullup() in drivers/usb/gadget/udc/core.c

which is set to pxa_udc_pullup(), which should be called not in interrupt

/**
 * pxa_udc_pullup - Offer manual D+ pullup control
 * @_gadget: usb gadget using the control
 * @is_active: 0 if disconnect, else connect D+ pullup resistor
 * Context: !in_interrupt()
 *
 * Returns 0 if OK, -EOPNOTSUPP if udc driver doesn't handle D+ pullup
 */

This finally causes fail at

udc_enable() in drivers/usb/gadget/udc/pxa27x_udc.c

at code

/*
 * Caller must be able to sleep in order to cope with startup transients
 */
msleep(100);

with a following error (with CONFIG_DEBUG_PREEMPT on):

BUG: scheduling while atomic: v4l_id/360/0x0002
...
Preemption disabled at:
[] usb_function_activate+0x20/0xa4 [libcomposite]
CPU: 0 PID: 360 Comm: v4l_id Not tainted 4.10.0-rc5-magician+ #2
Hardware name: HTC Magician
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (__schedule_bug+0x98/0xcc)
[] (__schedule_bug) from [] (__schedule+0x58/0x444)
[] (__schedule) from [] (schedule+0xa8/0xc8)
[] (schedule) from [] (schedule_timeout+0x1c8/0x1ec)
[] (schedule_timeout) from [] (msleep+0x1c/0x20)
[] (msleep) from [] (udc_enable+0x170/0x1d4 
[pxa27x_udc])
[] (udc_enable [pxa27x_udc]) from [] 
(pxa_udc_pullup+0x40/0x68 [pxa27x_udc])
[] (pxa_udc_pullup [pxa27x_udc]) from [] 
(usb_gadget_connect+0x3c/0x5c [udc_core])
[] (usb_gadget_connect [udc_core]) from [] 
(usb_function_activate+0x98/0xa4 [libcomposite])
[] (usb_function_activate [libcomposite]) from [] 
(uvc_function_connect+0x14/0x38 [usb_f_uvc])
[] (uvc_function_connect [usb_f_uvc]) from [] 
(uvc_v4l2_open+0x4c/0x60 [usb_f_uvc])
[] (uvc_v4l2_open [usb_f_uvc]) from [] 
(v4l2_open+0x7c/0xc0 [videodev])
[] (v4l2_open [videodev]) from [] 
(chrdev_open+0x198/0x1b0)
[] (chrdev_open) from [] 
(do_dentry_open.constprop.3+0x1b0/0x2ec)
[] (do_dentry_open.constprop.3) from [] 
(path_openat+0xc10/0xdd4)
[] (path_openat) from [] (do_filp_open+0x30/0x78)
[] (do_filp_open) from [] (do_sys_open+0xf0/0x1b0)
[] (do_sys_open) from [] (ret_fast_syscall+0x0/0x38)

With the msleep changed to mdelay, the code (specified as !in_interrupt()) 
seems to work fine
(after torture reloads). Can the caller (udc core) be changed to be able to 
sleep?

Second bug was discovered by Robert Jarzmik during discussion in

[BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

A call to usb_put_phy(udc->transceiver) in pxa_udc_remove() can be called with  
variable
containing error pointer or NULL. So it should be moved to a previous call like 
this (modified
original suggestion):

if (!IS_ERR_OR_NULL(udc->transceiver)) {
usb_unregister_notifier(udc->transceiver, _udc_phy);
usb_put_phy(udc->transceiver);
}

And as we talking about it, is this return correct?

if (of_have_populated_dt()) {
udc->transceiver =
devm_usb_get_phy_by_phandle(udc->dev, "phys", 0);

if (IS_ERR(udc->transceiver))
return PTR_ERR(udc->transceiver);
} else {
udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
}

One branch returns on error and second one is fine, udc->transceiver then can 
hold an error,
but this is fine for the rest of driver (tested). Question is does it have to 
return from a first
branch (e.g. my device does not have phy)?

And finally it seems definitions from drivers/usb/gadget/udc/pxa27x_udc.c are 
duplicated:

static void udc_enable(struct pxa_udc *udc);
static void udc_disable(struct pxa_udc *udc);

I will send patch series as soon as we agree on solutions.

best regards,
Petr
--
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: [BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

[Petr Cvek]

Dne 12.2.2017 v 13:02 Robert Jarzmik napsal(a):
> Petr Cvek <petr.c...@tul.cz> writes:
> 
>> Hi
>>
>> any news?
> 
> Yeah, I don't have the error on my mainstone, which kind of hinders my debug
> capability.
> 
> I have tried your script, and this is what I get :
>   - dmesg | tail
>   random: crng init done
>   gadgetfs: USB Gadget filesystem, version 24 Aug 2004
>   configfs-gadget gadget: uvc_function_bind
>   [DEBUG] opts->streaming_maxpacket=256
>   name: ep1in-int, mp=16
>   name: ep1in-int, mp=16
>   name: ep2in-int, mp=16
>   name: ep3out-int, mp=16
>   name: ep4in-iso, mp=256
>   - lsmod
>   Module  Size  Used byNot tainted
>   usb_f_uvc  40752  6 
>   libcomposite   38099 14 usb_f_uvc
>   pxa27x_udc 21131  0 
>   gadgetfs   16173  0 
>   videobuf2_vmalloc   4659  1 usb_f_uvc
>   videobuf2_memops1421  1 videobuf2_vmalloc
> 
> This was tried on a mainstone board, upon mainline kernel v4.8-rc3 with your
> patch. but without an actual camera nor the USB cable attached to a host USB.
> 

That's weird I even removed pxa_set_udc_info() from magician machine init and 
it still fails.
Host cable and/or actual camera is not required. It fails without them.

So you binded pxa27x-udc as UDC controller (= activated it) and then rmmoded it 
and nobody complained?


Can you try v4.10? Or send me exact commit on what you tested it so I can test 
it too.
But yeah if it doesn't fail on v4.8-rc3 then problem must be somewhere else. I 
appended actually
used .config, if some other configuration can cause it (perhaps some checking, 
dynamic debug, branch
optimalization or stack protecting?). I'm using GCC 6.3.0 (with one iwmmxt 
patch, but kernel
compiles for xscale).

#
# Automatically generated file; DO NOT EDIT.
# Linux/arm 4.10.0-rc5 Kernel Configuration
#
CONFIG_ARM=y
CONFIG_SYS_SUPPORTS_APM_EMULATION=y
CONFIG_HAVE_PROC_CPU=y
CONFIG_STACKTRACE_SUPPORT=y
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_TRACE_IRQFLAGS_SUPPORT=y
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
CONFIG_FIX_EARLYCON_MEM=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_NEED_DMA_MAP_STATE=y
CONFIG_ARCH_SUPPORTS_UPROBES=y
CONFIG_ARCH_MTD_XIP=y
CONFIG_VECTORS_BASE=0x
CONFIG_ARM_PATCH_PHYS_VIRT=y
CONFIG_GENERIC_BUG=y
CONFIG_PGTABLE_LEVELS=2
CONFIG_DEFCONFIG_LIST="/lib/modules/$UNAME_RELEASE/.config"
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_EXTABLE_SORT=y

#
# General setup
#
CONFIG_BROKEN_ON_SMP=y
CONFIG_INIT_ENV_ARG_LIMIT=32
CONFIG_CROSS_COMPILE=""
# CONFIG_COMPILE_TEST is not set
CONFIG_LOCALVERSION="-magician"
# CONFIG_LOCALVERSION_AUTO is not set
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
CONFIG_KERNEL_GZIP=y
# CONFIG_KERNEL_LZMA is not set
# CONFIG_KERNEL_XZ is not set
# CONFIG_KERNEL_LZO is not set
# CONFIG_KERNEL_LZ4 is not set
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_POSIX_MQUEUE is not set
CONFIG_CROSS_MEMORY_ATTACH=y
# CONFIG_FHANDLE is not set
# CONFIG_USELIB is not set
# CONFIG_AUDIT is not set
CONFIG_HAVE_ARCH_AUDITSYSCALL=y

#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_SHOW_LEVEL=y
CONFIG_HARDIRQS_SW_RESEND=y
CONFIG_IRQ_DOMAIN=y
CONFIG_HANDLE_DOMAIN_IRQ=y
# CONFIG_IRQ_DOMAIN_DEBUG is not set
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
CONFIG_ARCH_CLOCKSOURCE_DATA=y
CONFIG_GENERIC_CLOCKEVENTS=y

#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
CONFIG_NO_HZ_IDLE=y
# CONFIG_NO_HZ is not set
# CONFIG_HIGH_RES_TIMERS is not set

#
# CPU/Task time and stats accounting
#
CONFIG_TICK_CPU_ACCOUNTING=y
# CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set
# CONFIG_IRQ_TIME_ACCOUNTING is not set
# CONFIG_BSD_PROCESS_ACCT is not set
# CONFIG_TASKSTATS is not set

#
# RCU Subsystem
#
CONFIG_PREEMPT_RCU=y
# CONFIG_RCU_EXPERT is not set
CONFIG_SRCU=y
# CONFIG_TASKS_RCU is not set
CONFIG_RCU_STALL_COMMON=y
# CONFIG_TREE_RCU_TRACE is not set
# CONFIG_RCU_EXPEDITE_BOOT is not set
# CONFIG_BUILD_BIN2C is not set
# CONFIG_IKCONFIG is not set
CONFIG_LOG_BUF_SHIFT=18
CONFIG_NMI_LOG_BUF_SHIFT=12
CONFIG_GENERIC_SCHED_CLOCK=y
# CONFIG_CGROUPS is not set
# CONFIG_CHECKPOINT_RESTORE is not set
CONFIG_NAMESPACES=y
CONFIG_UTS_NS=y
CONFIG_IPC_NS=y
# CONFIG_USER_NS is not set
CONFIG_PID_NS=y
CONFIG_NET_NS=y
# CONFIG_SCHED_AUTOGROUP is not set
# CONFIG_SYSFS_DEPRECATED is not set
# CONFIG_RELAY is not set
CONFIG_BLK_DEV_INITRD=y
CONFIG_INITRAMFS_SOURCE=""
# CONFIG_RD_GZIP is not set
# CONFIG_RD_BZIP2 is not set
# CONFIG_RD_LZMA is not set
CONFIG_RD_XZ=y
# CONFIG_RD_LZO is not set
# CONFIG_RD_LZ4 is not set
CONFIG_INITRAMFS

Re: [BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

[Petr Cvek]

Hi

any news?

Petr

Dne 4.2.2017 v 23:42 Robert Jarzmik napsal(a):
> Petr Cvek <petr.c...@tul.cz> writes:
> 
>> Setting the UVC gadget with configfs and then reloading UDC controler driver
>> (pxa27x_udc) causes kernel to fail.
>>
>> UDC subsystem was patched only in decreasing maxpacket size for UVC, addition
>> of more predefined endpoints for pxa27x_udc and addition of some debugging 
>> pr_info.
>>
>> Practically same behaviour was observed on 4.7.0 too.
> Hi Petr,
> 
> If you could provide me your "debug patch" for the new endpoints, packet size
> and pr_infos, I could try to reproduce on my platform and have a look.
> 
> I must say that I'm not familiar with uvc, so I can only suppose you're trying
> to stream the magician camera to USB to "act" like a Webcam, is it what you're
> trying to achieve ?
> 
> Cheers.
> 
> --
> Robert
> 

--
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: [BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

[Petr Cvek]

Dne 4.2.2017 v 23:42 Robert Jarzmik napsal(a):
> Petr Cvek <petr.c...@tul.cz> writes:
> 
>> Setting the UVC gadget with configfs and then reloading UDC controler driver
>> (pxa27x_udc) causes kernel to fail.
>>
>> UDC subsystem was patched only in decreasing maxpacket size for UVC, addition
>> of more predefined endpoints for pxa27x_udc and addition of some debugging 
>> pr_info.
>>
>> Practically same behaviour was observed on 4.7.0 too.
> Hi Petr,
> 
> If you could provide me your "debug patch" for the new endpoints, packet size
> and pr_infos, I could try to reproduce on my platform and have a look.
> 

Sent it in the attachment, along with the belonging error log and a script to 
reproduce.

Not all patched parts (UDC+webcam) are probably necessary as the failure can be 
reproduced
without using webcam at all. Warning: patch is probably dirty formatted (I 
tried to
clean it a little bit, but still ...). Patch is from vanilla kernel 
(4.10.0-rc5) at commit:

a4685d2f58e2230d4e27fb2ee581d7ea35e5d046 

Failure starts after last:

modprobe pxa27x_udc

which is commented in the script. Modprobe command fails with:

Segmentation fault

> I must say that I'm not familiar with uvc, so I can only suppose you're trying
> to stream the magician camera to USB to "act" like a Webcam, is it what you're
> trying to achieve ?

Exactly! With USB 1.1 there is little bit problem with bandwidth for raw YUV 
:-), but
JPEG 320x240 is fine (although I had to patch UVC function and g_webcam). UVC 
gadget requires
userspace part of driver. I heavily changed one from there (to sends the images 
from filesystem),
but it is not required to reproduce the error:

http://git.ideasonboard.org/uvc-gadget.git

With the configfs method this userspace driver does not work, but it cannot be 
properly debugged
unless gadget driver/kernel oopses after reload.

Petr 

> 
> Cheers.
> 
> --
> Robert
> 

>From b444aeea9de2d8e507f2301f9fed1eb64dcc7e1e Mon Sep 17 00:00:00 2001
From: Petr Cvek <petr.c...@tul.cz>
Date: Wed, 3 Aug 2016 22:29:44 +0200
Subject: [PATCH] PXA27x UDC more endpoints for more gadgets + UVC gadget
 (configfs)

---
 drivers/usb/gadget/function/f_uvc.c |  6 ++-
 drivers/usb/gadget/function/uvc_v4l2.c  | 34 +-
 drivers/usb/gadget/function/uvc_video.c |  3 +-
 drivers/usb/gadget/legacy/webcam.c  | 35 +++
 drivers/usb/gadget/udc/core.c   |  3 ++
 drivers/usb/gadget/udc/pxa27x_udc.c | 79 +
 drivers/usb/gadget/udc/pxa27x_udc.h |  7 ++-
 7 files changed, 116 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 27ed51b5082f..ff20eacad4c4 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -597,6 +597,9 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
 	opts = fi_to_f_uvc_opts(f->fi);
 	/* Sanity check the streaming endpoint module parameters.
 	 */
+
+	pr_info("[DEBUG] opts->streaming_maxpacket=%i\n",opts->streaming_maxpacket);
+
 	opts->streaming_interval = clamp(opts->streaming_interval, 1U, 16U);
 	opts->streaming_maxpacket = clamp(opts->streaming_maxpacket, 1U, 3072U);
 	opts->streaming_maxburst = min(opts->streaming_maxburst, 15U);
@@ -845,7 +848,8 @@ static struct usb_function_instance *uvc_alloc_inst(void)
 		(const struct uvc_descriptor_header * const *)ctl_cls;
 
 	opts->streaming_interval = 1;
-	opts->streaming_maxpacket = 1024;
+
+	opts->streaming_maxpacket = 256;	//for PXA limit (PXA should support 1023)
 
 	uvcg_attach_configfs(opts);
 	return >func_inst;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 3e22b45687d3..4a2957b765ae 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -61,6 +61,7 @@ struct uvc_format {
 static struct uvc_format uvc_formats[] = {
 	{ 16, V4L2_PIX_FMT_YUYV  },
 	{ 0,  V4L2_PIX_FMT_MJPEG },
+	{ 0,  V4L2_PIX_FMT_JPEG },
 };
 
 static int
@@ -81,6 +82,34 @@ uvc_v4l2_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
 	return 0;
 }
 
+//around v3.10 for v4l2 compliance
+static int uvc_v4l2_enum_format(struct file *file, void  *priv,
+   struct v4l2_fmtdesc *fmt)
+{
+   struct uvc_format *format;
+   enum v4l2_buf_type type = fmt->type;
+
+   if (fmt->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+   pr_info("wcam: bad type %i\n",fmt->type);
+   return -EINVAL;
+   }
+
+   if (fmt->index >= ARRAY_SIZE(uvc_formats)) {
+   pr_info("wcam: index to high %i/%i\n",fmt->index,ARRAY_SIZE(uvc_formats));
+   return -EINVAL;
+   }
+
+   fmt->pixelformat = uvc_formats[fmt->index].fcc;
+   

[BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

[Petr Cvek]

Setting the UVC gadget with configfs and then reloading UDC controler driver
(pxa27x_udc) causes kernel to fail.

UDC subsystem was patched only in decreasing maxpacket size for UVC, addition
of more predefined endpoints for pxa27x_udc and addition of some debugging 
pr_info.

Practically same behaviour was observed on 4.7.0 too.

Script:

modprobe udc-core
modprobe pxa27x-udc
modprobe libcomposite
modprobe usb_f_uvc

mkdir -p /tmp/udc
cd /tmp/udc

mount none /tmp/udc -t configfs
mkdir -p usb_gadget/gcam

cd usb_gadget/gcam
mkdir -p configs/c.1

mkdir -p functions/uvc.usb0
mkdir -p strings/0x409
mkdir -p configs/c.1/strings/0x409

echo 0x1d6b > idVendor
echo 0x0102 > idProduct
echo 16 > bMaxPacketSize0
echo "Magician" > strings/0x409/serialnumber
echo "Linux Foundation" > strings/0x409/manufacturer
echo "Webcam/EEM" > strings/0x409/product
echo "Video" > configs/c.1/strings/0x409/configuration
echo 239 > bDeviceClass
echo 2 > bDeviceSubClass
echo 1 > bDeviceProtocol
echo 500 > configs/c.1/MaxPower
echo 0xc0 > configs/c.1/bmAttributes

mkdir -p functions/uvc.usb0/streaming/mjpeg/m/240p
cat < functions/uvc.usb0/streaming/mjpeg/m/240p/dwFrameInterval
66
100
500
EOF
echo "320" > functions/uvc.usb0/streaming/mjpeg/m/240p/wWidth 
echo "240" > functions/uvc.usb0/streaming/mjpeg/m/240p/wHeight
echo "200" >  
functions/uvc.usb0/streaming/mjpeg/m/240p/dwDefaultFrameInterval

mkdir -p functions/uvc.usb0/streaming/header/h
cd functions/uvc.usb0/streaming/header/h
ln -s ../../mjpeg/m
cd ../../class/fs
ln -s ../../header/h

cd ../../class/hs
ln -s ../../header/h

cd ../../../control
mkdir header/h
ln -s header/h class/fs

ln -s header/h class/ss

cd ../../../
ln -s functions/uvc.usb0 configs/c.1

echo pxa27x-udc > UDC

rmmod pxa27x_udc

modprobe pxa27x_udc
Segmentation fault

dmesg
configfs-gadget gadget: uvc_unbind
configfs-gadget gadget: uvc_function_bind
kobject (c2902870): tried to init an initialized object, something is 
seriously wrong.
CPU: 0 PID: 340 Comm: modprobe Not tainted 4.10.0-rc5+ #1
Hardware name: HTC Magician
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (kobject_init+0x74/0x94)
[] (kobject_init) from [] 
(device_initialize+0x20/0x9c)
[] (device_initialize) from [] 
(device_register+0xc/0x18)
[] (device_register) from [] 
(__video_register_device+0xdd0/0x1684 [videodev])
[] (__video_register_device [videodev]) from [] 
(uvc_function_bind+0x324/0x470 [usb_f_uvc])
[] (uvc_function_bind [usb_f_uvc]) from [] 
(usb_add_function+0x70/0x1bc [libcomposite])
[] (usb_add_function [libcomposite]) from [] 
(configfs_composite_bind+0x224/0x320 [libcomposite])
[] (configfs_composite_bind [libcomposite]) from [] 
(udc_bind_to_driver+0x34/0xf8 [udc_core])
[] (udc_bind_to_driver [udc_core]) from [] 
(usb_add_gadget_udc_release+0x190/0x23c [udc_core])
[] (usb_add_gadget_udc_release [udc_core]) from [] 
(pxa_udc_probe+0x238/0x320 [pxa27x_udc])
[] (pxa_udc_probe [pxa27x_udc]) from [] 
(platform_drv_probe+0x38/0x94)
[] (platform_drv_probe) from [] 
(driver_probe_device+0x230/0x420)
[] (driver_probe_device) from [] 
(__driver_attach+0xe4/0xf8)
[] (__driver_attach) from [] 
(bus_for_each_dev+0x58/0x88)
[] (bus_for_each_dev) from [] 
(bus_add_driver+0x148/0x234)
[] (bus_add_driver) from [] 
(driver_register+0x78/0xf4)
[] (driver_register) from [] 
(do_one_initcall+0x48/0x19c)
[] (do_one_initcall) from [] 
(do_init_module+0x54/0x37c)
[] (do_init_module) from [] 
(load_module+0x1c98/0x1f6c)
[] (load_module) from [] 
(SyS_finit_module+0x88/0xbc)
[] (SyS_finit_module) from [] 
(ret_fast_syscall+0x0/0x38)
Unable to handle kernel paging request at virtual address bf5a0718
pgd = c28dc000
[bf5a0718] *pgd=a28b8811, *pte=, *ppte=
Internal error: Oops: 7 [#1] ARM
Modules linked in: pxa27x_udc(+) usb_f_uvc videobuf2_vmalloc 
libcomposite udc_core ppp_deflate zlib_inflate zlib_deflate bsd_comp ppp_async 
ppp_generic slhc ircomm_tty ircomm configfs btusb btintel bluetooth 
firmware_class ads7846 max1586 pxaficp_ir soc_mediabus ohci_pxa27x irda 
ohci_hcd fixed snd_soc_pxa2xx spi_pxa2xx_platform snd_pxa2xx_lib 
snd_pcm_dmaengine snd_soc_pxa_ssp videobuf2_dma_sg usbcore snd_soc_core 
videobuf2_memops v4l2_common videobuf2_v4l2 i2c_pxa pwm_bl videodev backlight 
snd_pcm videobuf2_core usb_common snd_timer pwm_pxa i2c_core snd ssp rtc_pxa 
rtc_sa1100 soundcore leds_gpio htc_pasic3 led_class mfd_core pda_power [last 
unloaded: pxa27x_udc]
CPU: 0 PID: 340 Comm: modprobe Not tainted 4.10.0-rc5+ #1
Hardware name: HTC Magician
task: c3b40c00 task.stack: c33fc000
PC is at kobject_get+0x10/0xa4
LR is at get_device+0x14/0x1c
pc : []lr : []psr: a013
sp : c33fdbb8  

Re: [RFC] usb: gadget: uvc: webcam gadget USB PID is using value from a different gadget

[Petr Cvek]

Dne 23.1.2017 v 15:27 Felipe Balbi napsal(a):
> 
> Hi,
> 
> Greg KH  writes:
> fine by me. Just lsusb will look funky ;-)

 Heh, true, but I thought lsusb would use a string if the device provided
 it.  Haven't looked at that portion of the code in a very long time...

>>>
>>> My lsusb shows separate strings (using usbutils from slackware64-current):
>>>
>>> Bus 004 Device 003: ID 1d6b:0102 Linux Foundation EEM Gadget
>>> ...
>>>   idVendor   0x1d6b Linux Foundation
>>>   idProduct  0x0102 EEM Gadget
>>>   bcdDevice4.07
>>>   iManufacturer   1 Linux Foundation
>>>   iProduct2 Webcam gadget
>>> ...
>>
>> Ah, I guess it doesn't, but who knows how old that version of usbutils
>> is, considering the last release I did was well over a year ago.  I
>> should do a new one one of these days...
>>
>> Anyway, I'd like to not assign a product id to a chunk of code that is
>> going to be eventually deleted.  Felipe, what's the plan for the
>> "legacy" gadget code.  Is it ever going away?
> 
> Well, I wasn't really planning on deleting them just stopped accepting
> any new one. I wanted to avoid angry mobs complaining about not having a
> g_mass_storage.ko anymore.
> 
> Personally, I don't feel strongly about the legacy gadget
> drivers. They're not really needed anymore as everything they do can be
> done with configfs already. Perhaps we could schedule their removal for
> v5.0?
> 

If you want to remove legacy g_webcam then there should be a way to set its 
module parameters from somewhere (it  seems I can't find it anywhere). For 
PXA27x especially "opts->streaming_maxpacket" from f_uvc.c is critical. Default 
max packet size in PXA27x UDC (but lowering the limit to 256 by g_webcam 
parameter works).

Petr

--
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: [RFC] usb: gadget: uvc: webcam gadget USB PID is using value from a different gadget

[Petr Cvek]

Dne 23.1.2017 v 12:32 Greg KH napsal(a):
 I know it is only a cosmetic change on a legacy driver, but I assume
 it would be better to have some default value for configfs API than to
 borrow a PID from a whole different gadget.
>>>
>>> For class devices, they really don't need a new id, we should just use
>>> only one of them as it doesn't matter :)
>>

So using 0106 "Composite gadget: ACM + Mass Storage" or 0104 "Multifunction 
Composite Gadget" should be fine? (my actual setup is not multifunction though).

I'm actually trying to catch bug, when using configfs access webcam does not 
work or kernel oopses (and webcam does not work) :-D.

>> fine by me. Just lsusb will look funky ;-)
> 
> Heh, true, but I thought lsusb would use a string if the device provided
> it.  Haven't looked at that portion of the code in a very long time...
> 

My lsusb shows separate strings (using usbutils from slackware64-current):

Bus 004 Device 003: ID 1d6b:0102 Linux Foundation EEM Gadget
...
  idVendor   0x1d6b Linux Foundation
  idProduct  0x0102 EEM Gadget
  bcdDevice4.07
  iManufacturer   1 Linux Foundation
  iProduct2 Webcam gadget
...


Best regards,
Petr

--
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

[RFC] usb: gadget: uvc: webcam gadget USB PID is using value from a different gadget

[Petr Cvek]

Hello,

It seems the USB product ID for g_webcam usb device gadget is incorrectly used 
from EEM gadget "Ethernet Emulation Model". So "webcam" device has a confusing 
description in lsusb:

1d6b:0102 Linux Foundation EEM Gadget

I would change it to 0x0106, which is a next unassigned value (according to 
http://www.linux-usb.org/usb.ids). Does this step require some official 
communication with the holder of VID (Linux Foundation)? Or all it takes is 
just to send patch to the kernel and to the usb.ids database?

I know it is only a cosmetic change on a legacy driver, but I assume it would 
be better to have some default value for configfs API than to borrow a PID from 
a whole different gadget.

Change in question would be something like this (I will send a proper patch 
after RFC):

Fixing USB Product ID for UVC gadget (used from Ethernet Emulation Model) and 
changing it to new one.

Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/legacy/webcam.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/legacy/webcam.c 
b/drivers/usb/gadget/legacy/webcam.c
index f9661cd627c8..988814f2e1c7 100644
--- a/drivers/usb/gadget/legacy/webcam.c
+++ b/drivers/usb/gadget/legacy/webcam.c
@@ -42,7 +42,7 @@ MODULE_PARM_DESC(trace, "Trace level bitmask");
  */
 
 #define WEBCAM_VENDOR_ID   0x1d6b  /* Linux Foundation */
-#define WEBCAM_PRODUCT_ID  0x0102  /* Webcam A/V gadget */
+#define WEBCAM_PRODUCT_ID  0x0106  /* UVC video gadget */
 #define WEBCAM_DEVICE_BCD  0x0010  /* 0.10 */
 
 static char webcam_vendor_label[] = "Linux Foundation";
-- 
2.11.0
--
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

[PATCH] USB: Gadget: UVC: Add missing call for additional setup data

[Petr Cvek]

Some UVC commands require additional data (non zero uvc->event_length).
Add usb_ep_queue() call, so uvc_function_ep0_complete() can be called
and send received data to the userspace.

Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/function/f_uvc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/function/f_uvc.c 
b/drivers/usb/gadget/function/f_uvc.c
index 29b41b5..27ed51b 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -258,6 +258,13 @@ uvc_function_setup(struct usb_function *f, const struct 
usb_ctrlrequest *ctrl)
memcpy(_event->req, ctrl, sizeof(uvc_event->req));
v4l2_event_queue(>vdev, _event);
 
+   /* Pass additional setup data to userspace */
+   if (uvc->event_setup_out && uvc->event_length) {
+   uvc->control_req->length = uvc->event_length;
+   return usb_ep_queue(uvc->func.config->cdev->gadget->ep0,
+   uvc->control_req, GFP_ATOMIC);
+   }
+
return 0;
 }
 
-- 
2.9.2

--
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

[PATCH, RFC] USB: Gadget: UVC: Add missing call for additional setup data

[Petr Cvek]

Some UVC commands require additional data (non zero uvc->event_length).
Add usb_ep_queue() call, so uvc_function_ep0_complete() can be called
and send received data to the userspace.

Signed-off-by: Petr Cvek <petr.c...@tul.cz>
---
 drivers/usb/gadget/function/f_uvc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/function/f_uvc.c 
b/drivers/usb/gadget/function/f_uvc.c
index 29b41b5..27ed51b 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -258,6 +258,13 @@ uvc_function_setup(struct usb_function *f, const struct 
usb_ctrlrequest *ctrl)
memcpy(_event->req, ctrl, sizeof(uvc_event->req));
v4l2_event_queue(>vdev, _event);
 
+   /* Pass additional setup data to userspace */
+   if (uvc->event_setup_out && uvc->event_length) {
+   uvc->control_req->length = uvc->event_length;
+   return usb_ep_queue(uvc->func.config->cdev->gadget->ep0,
+   uvc->control_req, GFP_ATOMIC);
+   }
+
return 0;
 }
 
-- 
2.9.2

--
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

[PATCH] USB: Gadget: UVC: Fix unreachable control queue completion function in f_uvc.c

[Petr Cvek]

Some UVC commands require additional data. Add usb_ep_queue(), so
uvc_function_ep0_complete() can be called and send received data
to the userspace.

Fixing maxpacket variable inicialization value to be inside USB
full speed range.

Signed-off-by: Petr Cvek petr.c...@tul.cz
---
 drivers/usb/gadget/function/f_uvc.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c 
b/drivers/usb/gadget/function/f_uvc.c
index cf0df8f..9200d59 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -258,6 +258,11 @@ uvc_function_setup(struct usb_function *f, const struct 
usb_ctrlrequest *ctrl)
memcpy(uvc_event-req, ctrl, sizeof(uvc_event-req));
v4l2_event_queue(uvc-vdev, v4l2_event);
 
+   /* Pass additional setup data to userspace */
+   if (uvc-event_setup_out  uvc-event_length) {
+   req-length = uvc-event_length;
+   return usb_ep_queue(cdev-gadget-ep0, req, GFP_ATOMIC);
+   }
return 0;
 }
 
@@ -868,7 +873,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
(const struct uvc_descriptor_header * const *)ctl_cls;
 
opts-streaming_interval = 1;
-   opts-streaming_maxpacket = 1024;
+   opts-streaming_maxpacket = 1023;
 
uvcg_attach_configfs(opts);
return opts-func_inst;
-- 
1.7.12.1


--
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 v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

On 26.7.2015 17:14, Alan Stern wrote:
 On Sun, 26 Jul 2015, Petr Cvek wrote:
 
 What about higher speeds (not relevant on PXA, but ep_matches() is
 called from usb_ep_autoconfig_ss() )? According to

  http://wiki.osdev.org/Universal_Serial_Bus#Maximum_Data_Payload_Size_2

 High speed INT endpoint has a maximum data payload 1024 B and BULK
 only 512 B (are other attributes of the data phase similar?). What
 about superspeed?
 
 It's true that high speed interrupt endpoints can have higher maxpacket
 values than bulk endpoints.  But this is okay, since ep_matches()  
 checks that the hardware maxpacket value is at least as large as the
 value in the descriptor:
 
   if (ep-maxpacket_limit  max)
   return 0;

OK 

  
 * modprobe g_serial use_acm=1 n_ports=1
 * original version of ep_matches() (returns bulk and int)
 * compatible EP configuration/definition for UDC side 
 http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/pxa27x_udc.c#L2352
USB_EP_CTRL,
USB_EP_OUT_BULK(1),
USB_EP_IN_BULK(2),
USB_EP_IN_ISO(3),
USB_EP_OUT_ISO(4),
USB_EP_IN_INT(5),
 * modified EP configuration for UDC side (just changed EP3 ISO to BULK, so 
 there is one free BULK)
USB_EP_CTRL,
USB_EP_OUT_BULK(1),
USB_EP_IN_BULK(2),
USB_EP_IN_BULK(3),  //change
USB_EP_OUT_ISO(4),
USB_EP_IN_INT(5),

 ===results===
 * original configuration is OK, all endpoints are found (in order 
 ep2in-bulk, ep1out-bulk, ep3in-int), INT notification seems to work

 I don't understand.  Above you said that the EP definition in the UDC 
 is USB_EP_IN_ISO(3).  So how can you end up with ep3in-int?  int != ISO.
 You should have ended up with the third endpoint being ep5in-int, 
 because ep_matches() doesn't allow an isochronous to match a request 
 for an interrupt endpoint.

 I have changed definition of ISO to BULK only to accomplish minimal
 change of driver code (for my demonstration free BULK must be defined
 before INT - inserting new EP would require to reindex all next EPs
 and modifying links from PXA side endpoints). The USB_EP_IN_ISO(3) is
 just unused endpoint.
 
 This doesn't answer my question.  I was asking about the original 
 configuration, not your changed configuration.  You wrote:
 
 * original configuration is OK, all endpoints are found (in order
 ep2in-bulk, ep1out-bulk, ep3in-int), INT notification seems to work
 
 But that isn't possible, because in the original configuration ep3in is
 iso, not int.  Did you intend to write ep5in-int rather than
 ep3in-int?
 

Oh my bad, It seems I got confused by my debug printks (warning about bulk as 
int and end of search). It should be:

original:
ep2in-bulk
ep1out-bulk
ep5in-int

modified (free bulk before int):
ep2in-bulk
ep1out-bulk
ep3in-bulk
...
pxa27x-udc pxa27x-udc: ep15:pxa_ep_enable: type mismatch
...
g_serial gadget: acm ttyGS0 can't notify serial state, -22

 * modified configuration fails:

[ 4259.609088] pxa27x-udc pxa27x-udc: ep15:pxa_ep_enable: type mismatch

 by this condition: 


 http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/pxa27x_udc.c#L1416

 because ep_matches() returns BULK.

 Okay, that's a problem in pxa27x-udc.  Why does it insist on an exact 
 match between the hardware endpoint type and the type contained in the 
 descriptor?  It should accept an interrupt descriptor if the hardware 
 type is bulk.

 Hmm, making BULK EP equivalent with INT EP (when INT is requested)
 would made debugging (there is special bitfield in the config
 registers) and configuration preset (not anymore unordered set, but
 definition in the specific sequence) hell. But in other ways it can
 be OK (specification does not say that using EP marked INT as BULK
 will fail).
 
 This business about using bulk endpoints in place of interrupt
 endpoints goes back to the days when most UDCs had a very limited
 selection of endpoints.  The idea was that a gadget could work even if
 there weren't enough interrupt endpoints in the hardware, provided
 there were extra bulk endpoints.
 
I thought so, but on the PXA27x you can configure any endpoint for any type and 
at the same time you must configure them for the exact matching order.

Maybe matching with BULK as INT functionality could work if all predefined INT 
was first in the array (.udc_usb_ep). This would prioritize predefined INT 
endpoints and use BULK only after INT exhaustion.

(ideal way would be resetting UDC and generating EP configuration for every 
set_configuration/interface, but I don't know if possible).

 I think optimal idea is custom matcher function. It would eliminate
 codes for superspeed checking on SoC which known only fullspeed ;-).
 
 Wuldn't it also mean duplicating a lot of code?  Each custom matcher 
 function would essentially have to include most of ep_matches().
 
There can be global generic matching function (striped of quirks) which

Re: [PATCH v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

On 25.7.2015 20:08, Robert Baldyga wrote:

 Let me give you another example :
   - pxa27x_udc offers 3 endpoints : ep-in, ep-out, ep-iso-in
   - a gadget driver does :
 - request an ep-in
 - request an ep-out
 - request an ep-in
 - request an ep-iso-in
 In that case, the ep-iso-in request will fail, right ? Yet I would have 
 expected
 the second ep-in request to fail, as that's the one which cannot be serviced.
 
 Gadget driver cannot simply request ep-in. Endpoints are matched with ep 
 descriptors containing complete information about direction, type, 
 maxpacketsize etc. of requested endpoint. So described situation can never 
 take a place.
 
 However if gadget driver requests more endpoints than UDC driver supplies it 
 will do fail ;)

Yes and returning of BULK instead of INT can cause it (only defined BULK gets 
eaten by requested INT).

 
 Current matching mechanism is very simple and surely will not always return 
 optimal endpont set. Maybe we should try to develop something more 
 sophisticated.

I can test it (as I'm trying to get to work other gadgets like g_webcam, 
g_audio, g_hid and possibly function composites).

 

 Of course, this hypothetical case implies that pxa27x_udc is not compatible 
 with
 this gadget driver, so it's not really relevant, is it ...

 Because if they do, the ep_matches() function works poorly. It returns a 
 BULK
 for device (gadget) side, but host side (PC) thinks that this endpoint is 
 an INT
 and handles it in this way. But the PXA27x thinks the endpoint is a BULK 
 and
 handles it in its way (according to datasheet, settings for a BULK and an 
 INT
 transfers are not 100% compatible).

 How do they differ?
 One example I have in mind is chapter 12.4.2 of pxa27x developer manual
 Endpoint Memory Configuration, quote follows :
If the USB host controller transmits more OUT data than the 
 maximum
size packet for a bulk or interrupt endpoint, the UDC does not 
 send
any handshake to the host controller causing the host controller 
 to
time-out. If the USB host controller transmits more OUT data than 
 the
maximum size packet for an isochronous endpoint, the UDC sets the 
 data
packet error (DPE) bit in the Endpoint Control/Status register,
UDCCSRx[DPE].

 Perhaps you could submit a patch that adds a do not allocate a bulk
 endpoint when an interrupt endpoint is requested quirk flag to the
 usb_gadget structure, and modify the matching code to take the new flag
 into account.
 Well, if it was working that way already in the past, I don't see overloading
 the code with a quirk a necessity. My only need is that it continues to work.
 
 In this patchset I'm adding 'ep_match' callback to usb_gadget_ops, which can 
 be used to supply non-standard matching algorithm, so there is no need for 
 new quirk.

Yeah that would be better, every UDC to handle its way.

 
 Cheers,
 Robert
 

Petr
--
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 v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

On 25.7.2015 19:04, Robert Jarzmik wrote:
 Alan Stern st...@rowland.harvard.edu writes:
 
 Hi Alan,
 
 On Sat, 25 Jul 2015, Robert Jarzmik wrote:

 Petr Cvek petr.c...@tul.cz writes:

 On 23.7.2015 21:46, Alan Stern wrote:
 It seems that it allows using a BULK endpoint for requested INT
 endpoint. For my PXA27x machine the original code returns BULK EP
 even with valid INT endpoint definition (because BULK EPs are defined
 earlier than INT EPs).

 Yes, it does allow a bulk endpoint to be used when an interrupt 
 endpoint was requested.  However, it won't return a bulk endpoint if 
 all the bulk endpoints are already in use.
 This cannot work for pxa27x.

 Do you mean that on pxa27x, a bulk endpoint cannot be used as an
 interrupt endpoint?  Why not?  From the device controller's point of
 view, there is no difference between bulk and interrupt (except
 possibly for the maxpacket sizes and high-bandwidth usage when running
 at high speed).
 That's the point, maxpacket size and priority.
 
 As you said, it's not that it won't work, it won't work with the priority
 expected by the software stack, ie. higher priority for ISO endpoint.
 

Yes, maxpacket could be problem. Datasheet has listed range (1-64) for INT and 
specific values (8, 16, 32, 64) for BULK.


 The pxa27x IP has a hardware limitation which prevents an endpoint from 
 changing
 its type once the UDC is enabled (see the comment at the beginning of
 pxa27x_udc.c).

 If that patchset implies that for a requested INT endpoint a BULK endpoint 
 can
 be returned, that won't work. Felipe and Robert, is that what this patchset
 implies ?

 Sort of.  The matching code has always behaved that way and this
 patchset does not change the behavior.
 Then all is fine I suppose, if it was working before and nothing changes, it
 will continue to work, won't it ?

Yes functional behavior of this patch is same as in vanilla, I only began this 
thread, because I have found out that someone is sending patchset. 

But I found this behavior when I was trying to use g_webcam gadget. 

 
 A default PXA27x configuration returns BULK for requested INT. Which is
 unfortunate, because PXA27x supports INT endpoints and has one predefined, 
 but
 this function find BULK first (one BULK is allocated and INT is never 
 used).
 See above.

 See response above.

 Besides, let's say the pxa27x has one bulk and one interrupt endpoint.  
 Now suppose the gadget driver requests a bulk endpoint first.  The 
 matching code will allocate the single bulk endpoint.  Then the gadget 
 driver requests an interrupt endpoint.  The matching code cannot 
 allocate the bulk endpoint, because that endpoint is already allocated.  
 So it will allocate the interrupt endpoint.

 Thus, as you can see, under the right conditions everything will work 
 as desired.
 
 Let me give you another example :
  - pxa27x_udc offers 3 endpoints : ep-in, ep-out, ep-iso-in
  - a gadget driver does :
- request an ep-in
- request an ep-out
- request an ep-in
- request an ep-iso-in
 In that case, the ep-iso-in request will fail, right ? Yet I would have 
 expected
 the second ep-in request to fail, as that's the one which cannot be serviced.
 
 Of course, this hypothetical case implies that pxa27x_udc is not compatible 
 with
 this gadget driver, so it's not really relevant, is it ...

I have finally gathered enough information and luck (unstable machine) to try 
test g_serial so configuration:

* modprobe g_serial use_acm=1 n_ports=1
* original version of ep_matches() (returns bulk and int)
* compatible EP configuration/definition for UDC side 
http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/pxa27x_udc.c#L2352
USB_EP_CTRL,
USB_EP_OUT_BULK(1),
USB_EP_IN_BULK(2),
USB_EP_IN_ISO(3),
USB_EP_OUT_ISO(4),
USB_EP_IN_INT(5),
* modified EP configuration for UDC side (just changed EP3 ISO to BULK, so 
there is one free BULK)
USB_EP_CTRL,
USB_EP_OUT_BULK(1),
USB_EP_IN_BULK(2),
USB_EP_IN_BULK(3),  //change
USB_EP_OUT_ISO(4),
USB_EP_IN_INT(5),

===results===
* original configuration is OK, all endpoints are found (in order ep2in-bulk, 
ep1out-bulk, ep3in-int), INT notification seems to work
* modified configuration fails:

[ 4259.609088] pxa27x-udc pxa27x-udc: ep15:pxa_ep_enable: type mismatch

by this condition: 


http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/pxa27x_udc.c#L1416

because ep_matches() returns BULK. g_serial later disables INT notification

[ 4259.609871] g_serial gadget: acm ttyGS0 can't notify serial state, 
-22

So this function is waiting regression, all it takes is just one change into 
the PXA27x EP configuration or change of allocation order for endpoints in a 
gadget. And it limits other existing gadget from being supported too (PXA can 
have only 23 endpoints including different altsetting/interface/cfg 
combinations).

It could be easily

Re: [PATCH v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

On 25.7.2015 13:04, Robert Jarzmik wrote:
 Petr Cvek petr.c...@tul.cz writes:
 
 On 23.7.2015 21:46, Alan Stern wrote:
 It seems that it allows using a BULK endpoint for requested INT
 endpoint. For my PXA27x machine the original code returns BULK EP
 even with valid INT endpoint definition (because BULK EPs are defined
 earlier than INT EPs).

 Yes, it does allow a bulk endpoint to be used when an interrupt 
 endpoint was requested.  However, it won't return a bulk endpoint if 
 all the bulk endpoints are already in use.
 This cannot work for pxa27x.
 
 The pxa27x IP has a hardware limitation which prevents an endpoint from 
 changing
 its type once the UDC is enabled (see the comment at the beginning of
 pxa27x_udc.c).

Just crazy idea (based on how much I have to recompile kernel, when switching 
between testing gadgets), how much possible (EP allocation, no resource errors 
return, configuration/interface settings change) it would be to generate an EP 
configuration on the fly and only enabling the PXA27x when a set_configuration 
usb request is received?

Petr
--
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 v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

On 25.7.2015 23:36, Alan Stern wrote:
 On Sat, 25 Jul 2015, Petr Cvek wrote:
 
 On 25.7.2015 19:04, Robert Jarzmik wrote:
 Alan Stern st...@rowland.harvard.edu writes:

 Hi Alan,

 On Sat, 25 Jul 2015, Robert Jarzmik wrote:

 Petr Cvek petr.c...@tul.cz writes:

 On 23.7.2015 21:46, Alan Stern wrote:
 It seems that it allows using a BULK endpoint for requested INT
 endpoint. For my PXA27x machine the original code returns BULK EP
 even with valid INT endpoint definition (because BULK EPs are defined
 earlier than INT EPs).

 Yes, it does allow a bulk endpoint to be used when an interrupt 
 endpoint was requested.  However, it won't return a bulk endpoint if 
 all the bulk endpoints are already in use.
 This cannot work for pxa27x.

 Do you mean that on pxa27x, a bulk endpoint cannot be used as an
 interrupt endpoint?  Why not?  From the device controller's point of
 view, there is no difference between bulk and interrupt (except
 possibly for the maxpacket sizes and high-bandwidth usage when running
 at high speed).
 That's the point, maxpacket size and priority.

 As you said, it's not that it won't work, it won't work with the priority
 expected by the software stack, ie. higher priority for ISO endpoint.


 Yes, maxpacket could be problem. Datasheet has listed range (1-64)
 for INT and specific values (8, 16, 32, 64) for BULK.
 
 In practice I doubt this will matter.  Using a larger maxpacket size 
 than the gadget driver expects is rarely important for interrupt 
 transfers, since they almost never involve more than one packet's worth 
 of data.
 
 So for example, if the gadget driver wants an interrupt endpoint with 
 maxpacket 42, it almost certainly will work okay if it gets a bulk 
 endpoint with maxpacket 64.

What about higher speeds (not relevant on PXA, but ep_matches() is called from 
usb_ep_autoconfig_ss() )? According to 

http://wiki.osdev.org/Universal_Serial_Bus#Maximum_Data_Payload_Size_2

High speed INT endpoint has a maximum data payload 1024 B and BULK only 512 B 
(are other attributes of the data phase similar?). What about superspeed?

 
 If that patchset implies that for a requested INT endpoint a BULK 
 endpoint can
 be returned, that won't work. Felipe and Robert, is that what this 
 patchset
 implies ?

 Sort of.  The matching code has always behaved that way and this
 patchset does not change the behavior.
 Then all is fine I suppose, if it was working before and nothing changes, it
 will continue to work, won't it ?

 Yes functional behavior of this patch is same as in vanilla, I only
 began this thread, because I have found out that someone is sending
 patchset.

 But I found this behavior when I was trying to use g_webcam gadget. 
 
 I have finally gathered enough information and luck (unstable
 machine) to try test g_serial so configuration:

 * modprobe g_serial use_acm=1 n_ports=1
 * original version of ep_matches() (returns bulk and int)
 * compatible EP configuration/definition for UDC side 
 http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/pxa27x_udc.c#L2352
  USB_EP_CTRL,
  USB_EP_OUT_BULK(1),
  USB_EP_IN_BULK(2),
  USB_EP_IN_ISO(3),
  USB_EP_OUT_ISO(4),
  USB_EP_IN_INT(5),
 * modified EP configuration for UDC side (just changed EP3 ISO to BULK, so 
 there is one free BULK)
  USB_EP_CTRL,
  USB_EP_OUT_BULK(1),
  USB_EP_IN_BULK(2),
  USB_EP_IN_BULK(3),  //change
  USB_EP_OUT_ISO(4),
  USB_EP_IN_INT(5),

 ===results===
 * original configuration is OK, all endpoints are found (in order 
 ep2in-bulk, ep1out-bulk, ep3in-int), INT notification seems to work
 
 I don't understand.  Above you said that the EP definition in the UDC 
 is USB_EP_IN_ISO(3).  So how can you end up with ep3in-int?  int != ISO.
 You should have ended up with the third endpoint being ep5in-int, 
 because ep_matches() doesn't allow an isochronous to match a request 
 for an interrupt endpoint.

I have changed definition of ISO to BULK only to accomplish minimal change of 
driver code (for my demonstration free BULK must be defined before INT - 
inserting new EP would require to reindex all next EPs and modifying links from 
PXA side endpoints). The USB_EP_IN_ISO(3) is just unused endpoint.

 
 * modified configuration fails:

  [ 4259.609088] pxa27x-udc pxa27x-udc: ep15:pxa_ep_enable: type mismatch

 by this condition: 

  
 http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/pxa27x_udc.c#L1416

 because ep_matches() returns BULK.
 
 Okay, that's a problem in pxa27x-udc.  Why does it insist on an exact 
 match between the hardware endpoint type and the type contained in the 
 descriptor?  It should accept an interrupt descriptor if the hardware 
 type is bulk.

Hmm, making BULK EP equivalent with INT EP (when INT is requested) would made 
debugging (there is special bitfield in the config registers) and configuration 
preset (not anymore unordered set, but definition in the specific sequence) 
hell. But in other

Re: [PATCH v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

On 23.7.2015 16:36, Felipe Balbi wrote:
 Hi,
 
 On Thu, Jul 23, 2015 at 06:40:40AM +0200, Petr Cvek wrote:
 Hello,

 Is this:

  case USB_ENDPOINT_XFER_INT:
  /* Bulk endpoints handle interrupt transfers,
   * except the toggle-quirky iso-synch kind
   */
  if (!ep-caps.type_int  !ep-caps.type_bulk)
  return 0;

 ... or original:

  switch (type) {
  case USB_ENDPOINT_XFER_INT:
  /* bulk endpoints handle interrupt transfers,
   * except the toggle-quirky iso-synch kind
   */
  if ('s' == tmp[2])  {// == -iso
  return 0;

 code still valid? 

 It seems that it allows using a BULK endpoint for requested INT
 endpoint. For my PXA27x machine the original code returns BULK EP even
 with valid INT endpoint definition (because BULK EPs are defined
 earlier than INT EPs).

 This part of the code is from pre git era

  1da177e4c3f41524e886b7f1b8a0c1fc7321cac2

 before pxa27x driver was written and only few chips was supported.
 Does anyone know if the INT endpoints works now?
 
 it's very difficult to read your reply when you remove all context.
 

Ah sorry, I was hacking around PXA UDC and found possible bug in one 
ep_matches() function:

http://lxr.free-electrons.com/source/drivers/usb/gadget/epautoconf.c#L75

when searching for origin of this bug I have found about this new patch series 
(someone could know how that part of code was created).

Petr Cvek

--
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 v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

On 23.7.2015 21:46, Alan Stern wrote:
 On Thu, 23 Jul 2015, Petr Cvek wrote:
 
 Hello,

 Is this:

  case USB_ENDPOINT_XFER_INT:
  /* Bulk endpoints handle interrupt transfers,
   * except the toggle-quirky iso-synch kind
   */
  if (!ep-caps.type_int  !ep-caps.type_bulk)
  return 0;

 ... or original:

  switch (type) {
  case USB_ENDPOINT_XFER_INT:
  /* bulk endpoints handle interrupt transfers,
   * except the toggle-quirky iso-synch kind
   */
  if ('s' == tmp[2])  {// == -iso
  return 0;

 code still valid? 
 
 It's hard to understand your question.  Are you asking whether this 
 code is still present in the current version of the kernel?  You can 
 find out for yourself easily enough.

I'm asking if there are still UDCs which require this quirk. If not, we should 
change it to:

if ('n' != tmp[2])  {// != -int

so it will only return INT endpoints. Or if there are one or two which does not 
support interrupt endpoints, it would be (in my opinion) more practical to make 
a special case for them than force all other UDCs to use a BULK endpoint when 
they have an INT endpoint.

 
 It seems that it allows using a BULK endpoint for requested INT
 endpoint. For my PXA27x machine the original code returns BULK EP
 even with valid INT endpoint definition (because BULK EPs are defined
 earlier than INT EPs).
 
 Yes, it does allow a bulk endpoint to be used when an interrupt 
 endpoint was requested.  However, it won't return a bulk endpoint if 
 all the bulk endpoints are already in use.

A default PXA27x configuration returns BULK for requested INT. Which is 
unfortunate, because PXA27x supports INT endpoints and has one predefined, but 
this function find BULK first (one BULK is allocated and INT is never used).

 
 This part of the code is from pre git era

  1da177e4c3f41524e886b7f1b8a0c1fc7321cac2

 before pxa27x driver was written and only few chips was supported.
 Does anyone know if the INT endpoints works now?
 
 What makes you think they might not work?

Because if they do, the ep_matches() function works poorly. It returns a BULK 
for device (gadget) side, but host side (PC) thinks that this endpoint is an 
INT and handles it in this way. But the PXA27x thinks the endpoint is a BULK 
and handles it in its way (according to datasheet, settings for a BULK and an 
INT transfers are not 100% compatible).

I cannot test INT as BULK behavior for the gadget functions, because all 
gadgets which works on PXA27x does not use INT endpoints (some allocate the 
endpoint but never use it).

 
 Alan Stern
 

Petr Cvek
--
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 v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Petr Cvek]

Hello,

Is this:

case USB_ENDPOINT_XFER_INT:
/* Bulk endpoints handle interrupt transfers,
 * except the toggle-quirky iso-synch kind
 */
if (!ep-caps.type_int  !ep-caps.type_bulk)
return 0;

... or original:

switch (type) {
case USB_ENDPOINT_XFER_INT:
/* bulk endpoints handle interrupt transfers,
 * except the toggle-quirky iso-synch kind
 */
if ('s' == tmp[2])  {// == -iso
return 0;

code still valid? 

It seems that it allows using a BULK endpoint for requested INT endpoint. For 
my PXA27x machine the original code returns BULK EP even with valid INT 
endpoint definition (because BULK EPs are defined earlier than INT EPs).

This part of the code is from pre git era

1da177e4c3f41524e886b7f1b8a0c1fc7321cac2

before pxa27x driver was written and only few chips was supported. Does anyone 
know if the INT endpoints works now?

Petr Cvek
--
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