Re: [PATCH 2/3] usb: dwc3: gadget: Don't skip updating remaining data

[Thinh Nguyen]

On 8/1/2018 6:26 PM, Thinh Nguyen wrote:
> Hi Felipe,
>
>
> On 8/1/2018 1:33 AM, Felipe Balbi wrote:
>> Felipe Balbi  writes:
>>
>>> Hi,
>>>
>>> Felipe Balbi  writes:
>>>
>>> 
>>>
> This is an issue, but it's not the same one.
>
>  irq/16-dwc3-5032  [003] d...65.404194: dwc3_complete_trb:
> ep1out: trb 890522d5 buf b8d6d000 size 0 ctrl 3b56446c
> (hlCS:Sc:isoc-first)
>
 So this is chained to the next one, that's correct.


>  irq/16-dwc3-5032  [003] d...65.404209: dwc3_complete_trb:
> ep1out: trb c15f388f buf 37821000 size 1023 ctrl
> 3b564c78 (hlcS:SC:isoc)
>
 But here, HWO should've been left as is, because of the short packet.
 That's what the databook says on the two notes on section 8.2.3 after
 table 8-8 of Databook 2.60a:
> My proposal doesn't change how we handle the TRB's HWO currently.
>
 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a
 short packet, the chained TRBs that follow it are not written back
 (e.g. the BUFSIZ and HWO fields remain the same as the
 software-prepared value)

 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is
 also set), and a short packet is received, the core retires the TRB in
 progress and skip past the TRB where CHN=0, accumulating the ISP and
 IOC bits from each TRB. If ISP or IOC is set in any TRB, the core
 generates an XferInProgress event. Hardware does not set the HWO bit
 to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0
 TRB will also be retired and its buffer size field updated with the
 total number of bytes remaining in the BD.

 Note that at most we have confirmation that SIZE will be updated in
 case of Isoc endpoints, but there's nothing there about HWO, so I
 expected it to remain set according to the rest of the text.

 There's one possibility that "TRB will also be *retired*" means that
 it's not skipped, which would mean that HWO is, indeed, set to 0. To
> Right. And the programming guide also says that for isoc OUT transfer,
> - Any non-first TRBs with CHN=1 that had not already been retired will
> not be written back. HWO will still be 1, and software can reclaim them
> for another transfer.
> - The last TRB of the Buffer Descriptor will be retired with:
> * HWO = 0.
> * BUFSIZ = The total remaining buffer size of the Buffer Descriptor.
>  
> So we know that the last isoc TRB will have CHN=0 and HWO=0.
>
>
 patch that, however, we don't need all the extra checking you have
 implemented. I'll try to propose a slightly simpler fix if you're
 willing to test it out.
>>> Something like below:
>> and here's another possibility. This makes it clearer that we want to
>> skip all TRBs with CHN bit set:
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 9ba614032d5d..56e2a2ebae66 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
>> dwc3_ep *dep,
>> struct dwc3_request *req, struct dwc3_trb *trb,
>> const struct dwc3_event_depevt *event, int status, int chain)
>>  {
>> +   const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
>> unsigned intcount;
>>  
>> dwc3_ep_inc_deq(dep);
>> @@ -2256,6 +2257,16 @@ static int 
>> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>> return 1;
>> }
>>  
>> +   /*
>> +* In case of ISOC endpoints and Short or Zero packets, the
>> +* last TRB will be retired and its size field will be updated
>> +* to contain the full remaining size; meaning req->remaining
>> +* will be count from the last TRB in the chain.
>> +*/
>> +   if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc)
>> +   && chain)
>> +   return 0;
>> +
>> count = trb->size & DWC3_TRB_SIZE_MASK;
>> req->remaining += count;
>>
> These patches will not fix the issue.
>

What do you think of this fix?

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f452ab708ffc..338f7ab8a8b4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2277,8 +2277,10 @@ static int
dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 * with one TRB pending in the ring. We need to manually clear
HWO bit
 * from that TRB.
 */
-   if ((req->zero || req->unaligned) && (trb->ctrl &
DWC3_TRB_CTRL_HWO)) {
-   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+   if ((req->zero || req->unaligned) && !chain) {
+   if (trb->ctrl & DWC3_TRB_CTRL_HWO)
+   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+
return 1;
}


Thinh
--
To unsubscribe from this list: send the line 

[PATCH] usb: ehci-sh: convert to SPDX identifiers

[Kuninori Morimoto]

From: Kuninori Morimoto 

Signed-off-by: Kuninori Morimoto 
---
 include/linux/platform_data/ehci-sh.h | 14 +-
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/include/linux/platform_data/ehci-sh.h 
b/include/linux/platform_data/ehci-sh.h
index 5c15a73..d8397df 100644
--- a/include/linux/platform_data/ehci-sh.h
+++ b/include/linux/platform_data/ehci-sh.h
@@ -1,21 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
 /*
  * EHCI SuperH driver platform data
  *
  * Copyright (C) 2012  Nobuhiro Iwamatsu 
  * Copyright (C) 2012  Renesas Solutions Corp.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; version 2 of the License.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
 #ifndef __USB_EHCI_SH_H
-- 
2.7.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

Re: [PATCH 2/3] usb: dwc3: gadget: Don't skip updating remaining data

[Thinh Nguyen]

Hi Felipe,


On 8/1/2018 1:33 AM, Felipe Balbi wrote:
> Felipe Balbi  writes:
>
>> Hi,
>>
>> Felipe Balbi  writes:
>>
>> 
>>
 This is an issue, but it's not the same one.

  irq/16-dwc3-5032  [003] d...65.404194: dwc3_complete_trb:
 ep1out: trb 890522d5 buf b8d6d000 size 0 ctrl 3b56446c
 (hlCS:Sc:isoc-first)

>>> So this is chained to the next one, that's correct.
>>>
>>>
  irq/16-dwc3-5032  [003] d...65.404209: dwc3_complete_trb:
 ep1out: trb c15f388f buf 37821000 size 1023 ctrl
 3b564c78 (hlcS:SC:isoc)

>>> But here, HWO should've been left as is, because of the short packet.
>>> That's what the databook says on the two notes on section 8.2.3 after
>>> table 8-8 of Databook 2.60a:

My proposal doesn't change how we handle the TRB's HWO currently.

>>>
>>> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a
>>> short packet, the chained TRBs that follow it are not written back
>>> (e.g. the BUFSIZ and HWO fields remain the same as the
>>> software-prepared value)
>>>
>>> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is
>>> also set), and a short packet is received, the core retires the TRB in
>>> progress and skip past the TRB where CHN=0, accumulating the ISP and
>>> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core
>>> generates an XferInProgress event. Hardware does not set the HWO bit
>>> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0
>>> TRB will also be retired and its buffer size field updated with the
>>> total number of bytes remaining in the BD.
>>>
>>> Note that at most we have confirmation that SIZE will be updated in
>>> case of Isoc endpoints, but there's nothing there about HWO, so I
>>> expected it to remain set according to the rest of the text.
>>>
>>> There's one possibility that "TRB will also be *retired*" means that
>>> it's not skipped, which would mean that HWO is, indeed, set to 0. To

Right. And the programming guide also says that for isoc OUT transfer,
- Any non-first TRBs with CHN=1 that had not already been retired will
not be written back. HWO will still be 1, and software can reclaim them
for another transfer.
- The last TRB of the Buffer Descriptor will be retired with:
* HWO = 0.
* BUFSIZ = The total remaining buffer size of the Buffer Descriptor.
 
So we know that the last isoc TRB will have CHN=0 and HWO=0.


>>> patch that, however, we don't need all the extra checking you have
>>> implemented. I'll try to propose a slightly simpler fix if you're
>>> willing to test it out.
>> Something like below:
> and here's another possibility. This makes it clearer that we want to
> skip all TRBs with CHN bit set:
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9ba614032d5d..56e2a2ebae66 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
> dwc3_ep *dep,
> struct dwc3_request *req, struct dwc3_trb *trb,
> const struct dwc3_event_depevt *event, int status, int chain)
>  {
> +   const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
> unsigned intcount;
>  
> dwc3_ep_inc_deq(dep);
> @@ -2256,6 +2257,16 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
> dwc3_ep *dep,
> return 1;
> }
>  
> +   /*
> +* In case of ISOC endpoints and Short or Zero packets, the
> +* last TRB will be retired and its size field will be updated
> +* to contain the full remaining size; meaning req->remaining
> +* will be count from the last TRB in the chain.
> +*/
> +   if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc)
> +   && chain)
> +   return 0;
> +
> count = trb->size & DWC3_TRB_SIZE_MASK;
> req->remaining += count;
>

These patches will not fix the issue.

Let's take a look at the problem again.

static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
struct dwc3_request *req, struct dwc3_trb *trb,
const struct dwc3_event_depevt *event, int status, int chain)
{
unsigned intcount;

dwc3_ep_inc_deq(dep);

trace_dwc3_complete_trb(dep, trb);

/*
 * If we're in the middle of series of chained TRBs and we
 * receive a short transfer along the way, DWC3 will skip
 * through all TRBs including the last TRB in the chain (the
 * where CHN bit is zero. DWC3 will also avoid clearing HWO
 * bit and SW has to do it manually.
 *
 * We're going to do that here to avoid problems of HW trying
 * to use bogus TRBs for transfers.
 */
if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;

/*
 * If we're dealing with unaligned size OUT transfer, we will be left
 * with 

RE: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers

[He, Bo]

Your patch fix the issue BUG: scheduling while atomic:
BUG: scheduling while atomic: SettingsProvide/3433/0x0104
Preemption disabled at:
[] __do_softirq+0x53/0x31b
CPU: 1 PID: 3433 Comm: SettingsProvide Tainted: P U
ilt-2e5dc0ac-g3f662bf-dirty #8
Call Trace:
 
 dump_stack+0x70/0x9e
 __schedule_bug+0x7f/0xd0
 __schedule+0x61d/0x860
 schedule+0x36/0x90
 dwc3_gadget_ep_dequeue+0x27a/0x2e0 [dwc3]
 usb_ep_dequeue+0x23/0x90
 ffs_aio_cancel+0x4c/0x70
 kiocb_cancel+0x40/0x50
 free_ioctx_users+0x6e/0x100
 percpu_ref_switch_to_atomic_rcu+0x159/0x160
 rcu_process_callbacks+0x1a7/0x520
 __do_softirq+0x11a/0x31b
 irq_exit+0xb5/0xc0
 smp_apic_timer_interrupt+0x7c/0x160

the BUG is introduced form the patch:
commit: cf3113d89
usb: dwc3: gadget: properly increment dequeue pointer on ep_dequeue

   the commit: cf3113d89 call the below function maybe in softirq context:
wait_event_lock_irq(dep->wait_end_transfer,
   !(dep->flags & 
DWC3_EP_END_TRANSFER_PENDING),
   dwc->lock);

-Original Message-
From: Vincent Pelletier  
Sent: Wednesday, August 1, 2018 11:04 PM
To: Felipe Balbi 
Cc: Alan Stern ; Roger Quadros ; 
Lars-Peter Clausen ; Sam Protsenko 
; linux-usb@vger.kernel.org; Praneeth Bajjuri 
; He, Bo ; Bai, Jie A 
Subject: Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO 
transfers

Hello,

Bo He, CC'ed, found a regression introduced in my patch, discussed in this 
thread, and submitted a patch:
  Subject: [PATCH] fix panic at pwq_activate_delayed_work.
  Date: Wed, 1 Aug 2018 10:14:38 +
  Message-ID: 


On Fri, 29 Jun 2018 09:32:43 +0300, Felipe Balbi  
wrote:
> Hmm, that's what I remember, but we don't have that documented and 
> dwc3 has a sleep in its dequeue, which I need to remove for other 
> reasons anyway.

Given the above comment from Felipe, I expected my patch would be dropped in 
favour of making dwc3 not sleep in dequeue, as it seems to be the actual root 
cause.

Should my patch be reverted ? It adds complexity which, I believe, becomes 
superfluous if dequeue does not sleep anywhere.

Or maybe non-sleeping dequeue is not there yet, and a solution right now (later 
revertable) is better, in which case my change would be worth fixing ?
--
Vincent Pelletier
--
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/8] usb: gadget: uvc: configfs: Add section header comments

[Laurent Pinchart]

The UVC configfs implementation is large and difficult to navigate. Add
a bit more air to the code to make it easier to read.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/usb/gadget/function/uvc_configfs.c | 120 ++---
 1 file changed, 91 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index 1df94b25abe1..dbc95c9558de 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -12,6 +12,10 @@
 #include "u_uvc.h"
 #include "uvc_configfs.h"
 
+/* 
-
+ * Global Utility Structures and Macros
+ */
+
 #define UVCG_STREAMING_CONTROL_SIZE1
 
 #define UVC_ATTR(prefix, cname, aname) \
@@ -37,7 +41,11 @@ static inline struct f_uvc_opts *to_f_uvc_opts(struct 
config_item *item)
func_inst.group);
 }
 
-/* control/header/ */
+/* 
-
+ * control/header/
+ * control/header
+ */
+
 DECLARE_UVC_HEADER_DESCRIPTOR(1);
 
 struct uvcg_control_header {
@@ -161,7 +169,6 @@ static void uvcg_control_header_drop(struct config_group 
*group,
kfree(h);
 }
 
-/* control/header */
 static struct config_group uvcg_control_header_grp;
 
 static struct configfs_group_operations uvcg_control_header_grp_ops = {
@@ -174,7 +181,10 @@ static const struct config_item_type 
uvcg_control_header_grp_type = {
.ct_owner   = THIS_MODULE,
 };
 
-/* control/processing/default */
+/* 
-
+ * control/processing/default
+ */
+
 static struct config_group uvcg_default_processing_grp;
 
 #define UVCG_DEFAULT_PROCESSING_ATTR(cname, aname, conv)   \
@@ -260,16 +270,20 @@ static const struct config_item_type 
uvcg_default_processing_type = {
.ct_owner   = THIS_MODULE,
 };
 
-/* struct uvcg_processing {}; */
+/* 
-
+ * control/processing
+ */
 
-/* control/processing */
 static struct config_group uvcg_processing_grp;
 
 static const struct config_item_type uvcg_processing_grp_type = {
.ct_owner = THIS_MODULE,
 };
 
-/* control/terminal/camera/default */
+/* 
-
+ * control/terminal/camera/default
+ */
+
 static struct config_group uvcg_default_camera_grp;
 
 #define UVCG_DEFAULT_CAMERA_ATTR(cname, aname, conv)   \
@@ -366,16 +380,20 @@ static const struct config_item_type 
uvcg_default_camera_type = {
.ct_owner   = THIS_MODULE,
 };
 
-/* struct uvcg_camera {}; */
+/* 
-
+ * control/terminal/camera
+ */
 
-/* control/terminal/camera */
 static struct config_group uvcg_camera_grp;
 
 static const struct config_item_type uvcg_camera_grp_type = {
.ct_owner = THIS_MODULE,
 };
 
-/* control/terminal/output/default */
+/* 
-
+ * control/terminal/output/default
+ */
+
 static struct config_group uvcg_default_output_grp;
 
 #define UVCG_DEFAULT_OUTPUT_ATTR(cname, aname, conv)   \
@@ -433,23 +451,30 @@ static const struct config_item_type 
uvcg_default_output_type = {
.ct_owner   = THIS_MODULE,
 };
 
-/* struct uvcg_output {}; */
+/* 
-
+ * control/terminal/output
+ */
 
-/* control/terminal/output */
 static struct config_group uvcg_output_grp;
 
 static const struct config_item_type uvcg_output_grp_type = {
.ct_owner = THIS_MODULE,
 };
 
-/* control/terminal */
+/* 
-
+ * control/terminal
+ */
+
 static struct config_group uvcg_terminal_grp;
 
 static const struct config_item_type uvcg_terminal_grp_type = {
.ct_owner = THIS_MODULE,
 };
 
-/* control/class/{fs} */
+/* 
-
+ * control/class/{fs|ss}
+ */
+
 static struct config_group uvcg_control_class_fs_grp;
 static struct config_group uvcg_control_class_ss_grp;
 
@@ -552,24 +577,32 @@ static const struct config_item_type 
uvcg_control_class_type = {
.ct_owner   = THIS_MODULE,
 };
 
-/* control/class */
+/* 
-
+ * control/class
+ */
+
 static struct config_group uvcg_control_class_grp;
 
 static const struct config_item_type uvcg_control_class_grp_type = {
.ct_owner = THIS_MODULE,
 };
 
-/* control */
+/* 
-
+ * control
+ */
+
 static struct config_group 

[PATCH v2 8/8] usb: gadget: uvc: configfs: Prevent format changes after linking header

[Laurent Pinchart]

From: Joel Pepper 

While checks are in place to avoid attributes and children of a format
being manipulated after the format is linked into the streaming header,
the linked flag was never actually set, invalidating the protections.

Signed-off-by: Joel Pepper 
---
 drivers/usb/gadget/function/uvc_configfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index b8763343dcae..799dc32c5bc7 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -928,6 +928,7 @@ static int uvcg_streaming_header_allow_link(struct 
config_item *src,
format_ptr->fmt = target_fmt;
list_add_tail(_ptr->entry, _hdr->formats);
++src_hdr->num_fmt;
+   ++target_fmt->linked;
 
 out:
mutex_unlock(>lock);
@@ -965,6 +966,8 @@ static void uvcg_streaming_header_drop_link(struct 
config_item *src,
break;
}
 
+   --target_fmt->linked;
+
 out:
mutex_unlock(>lock);
mutex_unlock(su_mutex);
-- 
Regards,

Laurent Pinchart

--
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/8] usb: gadget: uvc: Improve configfs support

[Laurent Pinchart]

Hello,

This patch series reworks the UVC gadget function configfs implementation to
support multiple UVC functions in a single device and 

The first two patches simplify the code (01/08) and improve readability
(02/08).

The series then moves to dynamically allocating all configfs groups to support
multiple instances. It first fixes a config item reference leak (03/08) that
would cause a memory leak with dynamic allocation, and then allocate configfs
groups dynamically (04/08).

As a consequence we have to expose the interface numbers through new configfs
attributes (05/08) in order to let the userspace helper application discover
them. The next two patches add and document new bFormatIndex (06/08) and
bFrameIndex (07/08) configfs attributes to expose indices of UVC format and
frame descriptors to userspace, allowing their dynamic discovery.

The last patch finally fixes a bug that allowed modification of descriptors
after linking them (08/08).

Felipe, all this is based on top of your testing/next branch, and is a
candidate for v4.20. Please let me know if I should base the patches on a
different branch.

Changes since v1:

- Fix config items reference leak
- Free dynamically allocated configfs groups
- Squash attribute documentation and attribute creation patches

Joel Pepper (2):
  usb: gadget: uvc: configfs: Add bFrameIndex attributes
  usb: gadget: uvc: configfs: Prevent format changes after linking
header

Laurent Pinchart (6):
  usb: gadget: uvc: configfs: Don't wrap groups unnecessarily
  usb: gadget: uvc: configfs: Add section header comments
  usb: gadget: uvc: configfs: Drop leaked references to config items
  usb: gadget: uvc: configfs: Allocate groups dynamically
  usb: gadget: uvc: configfs: Add interface number attributes
  usb: gadget: uvc: configfs: Add bFormatIndex attributes

 Documentation/ABI/testing/configfs-usb-gadget-uvc |  24 +
 drivers/usb/gadget/function/f_uvc.c   |  10 +-
 drivers/usb/gadget/function/u_uvc.h   |   3 +
 drivers/usb/gadget/function/uvc_configfs.c| 926 +-
 4 files changed, 600 insertions(+), 363 deletions(-)

-- 
Regards,

Laurent Pinchart

--
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 5/8] usb: gadget: uvc: configfs: Add interface number attributes

[Laurent Pinchart]

The video control and video streaming interface numbers are needed in
the UVC gadget userspace stack to reply to UVC requests. They are
hardcoded to fixed values at the moment, preventing configurations with
multiple functions.

To fix this, make them dynamically discoverable by userspace through
read-only configfs attributes in /control/bInterfaceNumber and
/streaming/bInterfaceNumber respectively.

Signed-off-by: Laurent Pinchart 
---
Changes since v1:

- Document the new attribute
---
 Documentation/ABI/testing/configfs-usb-gadget-uvc |  8 +++
 drivers/usb/gadget/function/f_uvc.c   |  2 +
 drivers/usb/gadget/function/u_uvc.h   |  3 ++
 drivers/usb/gadget/function/uvc_configfs.c| 62 +++
 4 files changed, 75 insertions(+)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc 
b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 9281e2aa38df..490a0136fb02 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -12,6 +12,10 @@ Date:Dec 2014
 KernelVersion: 4.0
 Description:   Control descriptors
 
+   All attributes read only:
+   bInterfaceNumber- USB interface number for this
+ streaming interface
+
 What:  /config/usb-gadget/gadget/functions/uvc.name/control/class
 Date:  Dec 2014
 KernelVersion: 4.0
@@ -109,6 +113,10 @@ Date:  Dec 2014
 KernelVersion: 4.0
 Description:   Streaming descriptors
 
+   All attributes read only:
+   bInterfaceNumber- USB interface number for this
+ streaming interface
+
 What:  /config/usb-gadget/gadget/functions/uvc.name/streaming/class
 Date:  Dec 2014
 KernelVersion: 4.0
diff --git a/drivers/usb/gadget/function/f_uvc.c 
b/drivers/usb/gadget/function/f_uvc.c
index 95cb1b5f5ffe..4ea987741e6e 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -699,12 +699,14 @@ uvc_function_bind(struct usb_configuration *c, struct 
usb_function *f)
uvc_iad.bFirstInterface = ret;
uvc_control_intf.bInterfaceNumber = ret;
uvc->control_intf = ret;
+   opts->control_interface = ret;
 
if ((ret = usb_interface_id(c, f)) < 0)
goto error;
uvc_streaming_intf_alt0.bInterfaceNumber = ret;
uvc_streaming_intf_alt1.bInterfaceNumber = ret;
uvc->streaming_intf = ret;
+   opts->streaming_interface = ret;
 
/* Copy descriptors */
f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
diff --git a/drivers/usb/gadget/function/u_uvc.h 
b/drivers/usb/gadget/function/u_uvc.h
index 2ed292e94fbc..5242d489e20a 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -25,6 +25,9 @@ struct f_uvc_opts {
unsigned intstreaming_maxpacket;
unsigned intstreaming_maxburst;
 
+   unsigned intcontrol_interface;
+   unsigned intstreaming_interface;
+
/*
 * Control descriptors array pointers for full-/high-speed and
 * super-speed. They point by default to the uvc_fs_control_cls and
diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index ae722549eabc..fa8d2e1f54ba 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -713,9 +713,40 @@ static const struct uvcg_config_group_type 
uvcg_control_class_grp_type = {
  * control
  */
 
+static ssize_t uvcg_default_control_b_interface_number_show(
+   struct config_item *item, char *page)
+{
+   struct config_group *group = to_config_group(item);
+   struct mutex *su_mutex = >cg_subsys->su_mutex;
+   struct config_item *opts_item;
+   struct f_uvc_opts *opts;
+   int result = 0;
+
+   mutex_lock(su_mutex); /* for navigating configfs hierarchy */
+
+   opts_item = item->ci_parent;
+   opts = to_f_uvc_opts(opts_item);
+
+   mutex_lock(>lock);
+   result += sprintf(page, "%u\n", opts->control_interface);
+   mutex_unlock(>lock);
+
+   mutex_unlock(su_mutex);
+
+   return result;
+}
+
+UVC_ATTR_RO(uvcg_default_control_, b_interface_number, bInterfaceNumber);
+
+static struct configfs_attribute *uvcg_default_control_attrs[] = {
+   _default_control_attr_b_interface_number,
+   NULL,
+};
+
 static const struct uvcg_config_group_type uvcg_control_grp_type = {
.type = {
.ct_item_ops= _config_item_ops,
+   .ct_attrs   = uvcg_default_control_attrs,
.ct_owner   = THIS_MODULE,
},
.name = "control",
@@ -2259,9 +2290,40 @@ static const struct 

[PATCH v2 4/8] usb: gadget: uvc: configfs: Allocate groups dynamically

[Laurent Pinchart]

The UVC configfs implementation creates all groups as global static
variables. This prevents creation of multiple UVC function instances,
as they would all require their own configfs group instances.

Fix this by allocating all groups dynamically. To avoid duplicating code
around, extend the config_item_type structure with group name and
children, and implement helper functions to create children
automatically for most groups.

Signed-off-by: Laurent Pinchart 
---
Changes since v1:

- Free groups by implementing .release() handler and removing children
  explicitly.
---
 drivers/usb/gadget/function/f_uvc.c|   8 +-
 drivers/usb/gadget/function/uvc_configfs.c | 581 -
 2 files changed, 338 insertions(+), 251 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c 
b/drivers/usb/gadget/function/f_uvc.c
index d8ce7868fe22..95cb1b5f5ffe 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -792,6 +792,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
struct uvc_output_terminal_descriptor *od;
struct uvc_color_matching_descriptor *md;
struct uvc_descriptor_header **ctl_cls;
+   int ret;
 
opts = kzalloc(sizeof(*opts), GFP_KERNEL);
if (!opts)
@@ -868,7 +869,12 @@ static struct usb_function_instance *uvc_alloc_inst(void)
opts->streaming_interval = 1;
opts->streaming_maxpacket = 1024;
 
-   uvcg_attach_configfs(opts);
+   ret = uvcg_attach_configfs(opts);
+   if (ret < 0) {
+   kfree(opts);
+   return ERR_PTR(ret);
+   }
+
return >func_inst;
 }
 
diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index 8d513cc6fb8c..ae722549eabc 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -41,6 +41,71 @@ static inline struct f_uvc_opts *to_f_uvc_opts(struct 
config_item *item)
func_inst.group);
 }
 
+struct uvcg_config_group_type {
+   struct config_item_type type;
+   const char *name;
+   const struct uvcg_config_group_type **children;
+   int (*create_children)(struct config_group *group);
+};
+
+static void uvcg_config_item_release(struct config_item *item)
+{
+   struct config_group *group = to_config_group(item);
+
+   kfree(group);
+}
+
+static struct configfs_item_operations uvcg_config_item_ops = {
+   .release= uvcg_config_item_release,
+};
+
+static int uvcg_config_create_group(struct config_group *parent,
+   const struct uvcg_config_group_type *type);
+
+static int uvcg_config_create_children(struct config_group *group,
+   const struct uvcg_config_group_type *type)
+{
+   const struct uvcg_config_group_type **child;
+   int ret;
+
+   if (type->create_children)
+   return type->create_children(group);
+
+   for (child = type->children; child && *child; ++child) {
+   ret = uvcg_config_create_group(group, *child);
+   if (ret < 0)
+   return ret;
+   }
+
+   return 0;
+}
+
+static int uvcg_config_create_group(struct config_group *parent,
+   const struct uvcg_config_group_type *type)
+{
+   struct config_group *group;
+
+   group = kzalloc(sizeof(*group), GFP_KERNEL);
+   if (!group)
+   return -ENOMEM;
+
+   config_group_init_type_name(group, type->name, >type);
+   configfs_add_default_group(group, parent);
+
+   return uvcg_config_create_children(group, type);
+}
+
+static void uvcg_config_remove_children(struct config_group *group)
+{
+   struct config_group *child, *n;
+
+   list_for_each_entry_safe(child, n, >default_groups, group_entry) 
{
+   list_del(>group_entry);
+   uvcg_config_remove_children(child);
+   config_item_put(>cg_item);
+   }
+}
+
 /* 
-
  * control/header/
  * control/header
@@ -137,6 +202,7 @@ static struct configfs_attribute 
*uvcg_control_header_attrs[] = {
 };
 
 static const struct config_item_type uvcg_control_header_type = {
+   .ct_item_ops= _config_item_ops,
.ct_attrs   = uvcg_control_header_attrs,
.ct_owner   = THIS_MODULE,
 };
@@ -161,32 +227,23 @@ static struct config_item 
*uvcg_control_header_make(struct config_group *group,
return >item;
 }
 
-static void uvcg_control_header_drop(struct config_group *group,
- struct config_item *item)
-{
-   struct uvcg_control_header *h = to_uvcg_control_header(item);
-
-   kfree(h);
-}
-
-static struct config_group uvcg_control_header_grp;
-
 static struct configfs_group_operations uvcg_control_header_grp_ops = {
.make_item  = 

[PATCH v2 6/8] usb: gadget: uvc: configfs: Add bFormatIndex attributes

[Laurent Pinchart]

The UVC format description are numbered using the descriptor's
bFormatIndex field. The index is used in UVC requests, and is thus
needed to handle requests in userspace. Make it dynamically discoverable
by exposing it in a bFormatIndex configfs attribute of the uncompressed
and mjpeg format config items.

The bFormatIndex value exposed through the attribute is stored in the
config item private data. However, that value is never set: the driver
instead computes the bFormatIndex value when linking the stream class
header in the configfs hierarchy and stores it directly in the class
descriptors in a separate structure. In order to expose the value
through the configfs attribute, store it in the config item private data
as well. This results in a small code simplification.

Signed-off-by: Laurent Pinchart 
---

Changes since v1:

- Squash patch "usb: gadget: uvc: configfs: Document the bFormatIndex
  attribute".
---
 Documentation/ABI/testing/configfs-usb-gadget-uvc |  8 
 drivers/usb/gadget/function/uvc_configfs.c| 14 --
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc 
b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 490a0136fb02..a6cc8d6d398e 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -168,6 +168,10 @@ Description:   Specific MJPEG format descriptors
 
All attributes read only,
except bmaControls and bDefaultFrameIndex:
+   bFormatIndex- unique id for this format descriptor;
+   only defined after parent header is
+   linked into the streaming class;
+   read-only
bmaControls - this format's data for bmaControls in
the streaming header
bmInterfaceFlags- specifies interlace information,
@@ -212,6 +216,10 @@ Date:  Dec 2014
 KernelVersion: 4.0
 Description:   Specific uncompressed format descriptors
 
+   bFormatIndex- unique id for this format descriptor;
+   only defined after parent header is
+   linked into the streaming class;
+   read-only
bmaControls - this format's data for bmaControls in
the streaming header
bmInterfaceFlags- specifies interlace information,
diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index fa8d2e1f54ba..5cee8aca3734 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -1538,6 +1538,7 @@ UVC_ATTR(uvcg_uncompressed_, cname, aname);
 
 #define identity_conv(x) (x)
 
+UVCG_UNCOMPRESSED_ATTR_RO(b_format_index, bFormatIndex, identity_conv);
 UVCG_UNCOMPRESSED_ATTR(b_bits_per_pixel, bBitsPerPixel, identity_conv);
 UVCG_UNCOMPRESSED_ATTR(b_default_frame_index, bDefaultFrameIndex,
   identity_conv);
@@ -1568,6 +1569,7 @@ uvcg_uncompressed_bma_controls_store(struct config_item 
*item,
 UVC_ATTR(uvcg_uncompressed_, bma_controls, bmaControls);
 
 static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
+   _uncompressed_attr_b_format_index,
_uncompressed_attr_guid_format,
_uncompressed_attr_b_bits_per_pixel,
_uncompressed_attr_b_default_frame_index,
@@ -1738,6 +1740,7 @@ UVC_ATTR(uvcg_mjpeg_, cname, aname)
 
 #define identity_conv(x) (x)
 
+UVCG_MJPEG_ATTR_RO(b_format_index, bFormatIndex, identity_conv);
 UVCG_MJPEG_ATTR(b_default_frame_index, bDefaultFrameIndex,
   identity_conv);
 UVCG_MJPEG_ATTR_RO(bm_flags, bmFlags, identity_conv);
@@ -1768,6 +1771,7 @@ uvcg_mjpeg_bma_controls_store(struct config_item *item,
 UVC_ATTR(uvcg_mjpeg_, bma_controls, bmaControls);
 
 static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
+   _mjpeg_attr_b_format_index,
_mjpeg_attr_b_default_frame_index,
_mjpeg_attr_bm_flags,
_mjpeg_attr_b_aspect_ratio_x,
@@ -2079,24 +2083,22 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, 
void *priv3, int n,
struct uvcg_format *fmt = priv1;
 
if (fmt->type == UVCG_UNCOMPRESSED) {
-   struct uvc_format_uncompressed *unc = *dest;
struct uvcg_uncompressed *u =
container_of(fmt, struct uvcg_uncompressed,
 fmt);
 
+   u->desc.bFormatIndex = n + 1;
+   u->desc.bNumFrameDescriptors = fmt->num_frames;
memcpy(*dest, >desc, sizeof(u->desc));
*dest += 

[PATCH v2 1/8] usb: gadget: uvc: configfs: Don't wrap groups unnecessarily

[Laurent Pinchart]

Various configfs groups (represented by config_group) are wrapped in
structures that they're the only member of. This allows adding other
data fields to groups, but it unnecessarily makes the code more complex.
Remove the outer structures and use config_group directly to simplify
the code. Groups can still be wrapped individually in the future if
other data fields need to be added.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/usb/gadget/function/uvc_configfs.c | 302 +++--
 1 file changed, 117 insertions(+), 185 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index b51f0d278826..1df94b25abe1 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -162,9 +162,7 @@ static void uvcg_control_header_drop(struct config_group 
*group,
 }
 
 /* control/header */
-static struct uvcg_control_header_grp {
-   struct config_group group;
-} uvcg_control_header_grp;
+static struct config_group uvcg_control_header_grp;
 
 static struct configfs_group_operations uvcg_control_header_grp_ops = {
.make_item  = uvcg_control_header_make,
@@ -177,31 +175,22 @@ static const struct config_item_type 
uvcg_control_header_grp_type = {
 };
 
 /* control/processing/default */
-static struct uvcg_default_processing {
-   struct config_group group;
-} uvcg_default_processing;
-
-static inline struct uvcg_default_processing
-*to_uvcg_default_processing(struct config_item *item)
-{
-   return container_of(to_config_group(item),
-   struct uvcg_default_processing, group);
-}
+static struct config_group uvcg_default_processing_grp;
 
 #define UVCG_DEFAULT_PROCESSING_ATTR(cname, aname, conv)   \
 static ssize_t uvcg_default_processing_##cname##_show( \
struct config_item *item, char *page)   \
 {  \
-   struct uvcg_default_processing *dp = to_uvcg_default_processing(item); \
+   struct config_group *group = to_config_group(item); \
struct f_uvc_opts *opts;\
struct config_item *opts_item;  \
-   struct mutex *su_mutex = >group.cg_subsys->su_mutex;\
+   struct mutex *su_mutex = >cg_subsys->su_mutex;   \
struct uvc_processing_unit_descriptor *pd;  \
int result; \
\
mutex_lock(su_mutex); /* for navigating configfs hierarchy */   \
\
-   opts_item = dp->group.cg_item.ci_parent->ci_parent->ci_parent;  \
+   opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
opts = to_f_uvc_opts(opts_item);\
pd = >uvc_processing; \
\
@@ -229,17 +218,17 @@ UVCG_DEFAULT_PROCESSING_ATTR(i_processing, iProcessing, 
identity_conv);
 static ssize_t uvcg_default_processing_bm_controls_show(
struct config_item *item, char *page)
 {
-   struct uvcg_default_processing *dp = to_uvcg_default_processing(item);
+   struct config_group *group = to_config_group(item);
struct f_uvc_opts *opts;
struct config_item *opts_item;
-   struct mutex *su_mutex = >group.cg_subsys->su_mutex;
+   struct mutex *su_mutex = >cg_subsys->su_mutex;
struct uvc_processing_unit_descriptor *pd;
int result, i;
char *pg = page;
 
mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
-   opts_item = dp->group.cg_item.ci_parent->ci_parent->ci_parent;
+   opts_item = group->cg_item.ci_parent->ci_parent->ci_parent;
opts = to_f_uvc_opts(opts_item);
pd = >uvc_processing;
 
@@ -274,40 +263,29 @@ static const struct config_item_type 
uvcg_default_processing_type = {
 /* struct uvcg_processing {}; */
 
 /* control/processing */
-static struct uvcg_processing_grp {
-   struct config_group group;
-} uvcg_processing_grp;
+static struct config_group uvcg_processing_grp;
 
 static const struct config_item_type uvcg_processing_grp_type = {
.ct_owner = THIS_MODULE,
 };
 
 /* control/terminal/camera/default */
-static struct uvcg_default_camera {
-   struct config_group group;
-} uvcg_default_camera;
-
-static inline struct uvcg_default_camera
-*to_uvcg_default_camera(struct config_item *item)
-{
-   return container_of(to_config_group(item),
-   struct uvcg_default_camera, group);
-}
+static struct config_group uvcg_default_camera_grp;
 
 #define 

[PATCH v2 3/8] usb: gadget: uvc: configfs: Drop leaked references to config items

[Laurent Pinchart]

Some of the .allow_link() and .drop_link() operations implementations
call config_group_find_item() and then leak the reference to the
returned item. Fix this by dropping those references where needed.

Signed-off-by: Laurent Pinchart 
---
 drivers/usb/gadget/function/uvc_configfs.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index dbc95c9558de..8d513cc6fb8c 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -529,6 +529,7 @@ static int uvcg_control_class_allow_link(struct config_item 
*src,
 unlock:
mutex_unlock(>lock);
 out:
+   config_item_put(header);
mutex_unlock(su_mutex);
return ret;
 }
@@ -564,6 +565,7 @@ static void uvcg_control_class_drop_link(struct config_item 
*src,
 unlock:
mutex_unlock(>lock);
 out:
+   config_item_put(header);
mutex_unlock(su_mutex);
 }
 
@@ -2026,6 +2028,7 @@ static int uvcg_streaming_class_allow_link(struct 
config_item *src,
 unlock:
mutex_unlock(>lock);
 out:
+   config_item_put(header);
mutex_unlock(su_mutex);
return ret;
 }
@@ -2066,6 +2069,7 @@ static void uvcg_streaming_class_drop_link(struct 
config_item *src,
 unlock:
mutex_unlock(>lock);
 out:
+   config_item_put(header);
mutex_unlock(su_mutex);
 }
 
-- 
Regards,

Laurent Pinchart

--
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 7/8] usb: gadget: uvc: configfs: Add bFrameIndex attributes

[Laurent Pinchart]

From: Joel Pepper 

- Add bFrameIndex as a UVCG_FRAME_ATTR_RO for each frame size.
- Automatically assign ascending bFrameIndex to each frame in a format.

Before all "bFrameindex" attributes were set to "1" with no way to
configure the gadget otherwise. This resulted in the host always
negotiating for bFrameIndex 1 (i.e. the first framesize of the gadget).
After the negotiation the host driver will set the user or application
selected framesize, while the gadget is actually set to the first
framesize.

Now, when the containing format is linked into the streaming header,
iterate over all child frame descriptors and assign ascending indices.
The automatically assigned indices can be read from the new read only
bFrameIndex configsfs attribute in each frame descriptor item.

Signed-off-by: Joel Pepper 
[Simplified documentation, renamed function, blank space update]
Signed-off-by: Laurent Pinchart 
---
 Documentation/ABI/testing/configfs-usb-gadget-uvc |  8 
 drivers/usb/gadget/function/uvc_configfs.c| 56 +++
 2 files changed, 64 insertions(+)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc 
b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index a6cc8d6d398e..809765bd9573 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -189,6 +189,10 @@ Date:  Dec 2014
 KernelVersion: 4.0
 Description:   Specific MJPEG frame descriptors
 
+   bFrameIndex - unique id for this framedescriptor;
+   only defined after parent format is
+   linked into the streaming header;
+   read-only
dwFrameInterval - indicates how frame interval can be
programmed; a number of values
separated by newline can be specified
@@ -240,6 +244,10 @@ Date:  Dec 2014
 KernelVersion: 4.0
 Description:   Specific uncompressed frame descriptors
 
+   bFrameIndex - unique id for this framedescriptor;
+   only defined after parent format is
+   linked into the streaming header;
+   read-only
dwFrameInterval - indicates how frame interval can be
programmed; a number of values
separated by newline can be specified
diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index 5cee8aca3734..b8763343dcae 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -868,6 +868,8 @@ static struct uvcg_streaming_header 
*to_uvcg_streaming_header(struct config_item
return container_of(item, struct uvcg_streaming_header, item);
 }
 
+static void uvcg_format_set_indices(struct config_group *fmt);
+
 static int uvcg_streaming_header_allow_link(struct config_item *src,
struct config_item *target)
 {
@@ -915,6 +917,8 @@ static int uvcg_streaming_header_allow_link(struct 
config_item *src,
if (!target_fmt)
goto out;
 
+   uvcg_format_set_indices(to_config_group(target));
+
format_ptr = kzalloc(sizeof(*format_ptr), GFP_KERNEL);
if (!format_ptr) {
ret = -ENOMEM;
@@ -1146,6 +1150,41 @@ end: 
\
\
 UVC_ATTR(uvcg_frame_, cname, aname);
 
+static ssize_t uvcg_frame_b_frame_index_show(struct config_item *item,
+char *page)
+{
+   struct uvcg_frame *f = to_uvcg_frame(item);
+   struct uvcg_format *fmt;
+   struct f_uvc_opts *opts;
+   struct config_item *opts_item;
+   struct config_item *fmt_item;
+   struct mutex *su_mutex = >item.ci_group->cg_subsys->su_mutex;
+   int result;
+
+   mutex_lock(su_mutex); /* for navigating configfs hierarchy */
+
+   fmt_item = f->item.ci_parent;
+   fmt = to_uvcg_format(fmt_item);
+
+   if (!fmt->linked) {
+   result = -EBUSY;
+   goto out;
+   }
+
+   opts_item = fmt_item->ci_parent->ci_parent->ci_parent;
+   opts = to_f_uvc_opts(opts_item);
+
+   mutex_lock(>lock);
+   result = sprintf(page, "%d\n", f->frame.b_frame_index);
+   mutex_unlock(>lock);
+
+out:
+   mutex_unlock(su_mutex);
+   return result;
+}
+
+UVC_ATTR_RO(uvcg_frame_, b_frame_index, bFrameIndex);
+
 #define noop_conversion(x) (x)
 
 UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
@@ -1294,6 +1333,7 @@ static ssize_t uvcg_frame_dw_frame_interval_store(struct 
config_item 

Re: [PATCH 4/8] usb: gadget: uvc: configfs: Add interface number attributes

[Laurent Pinchart]

Hi Kieran,

On Wednesday, 1 August 2018 13:07:20 EEST Kieran Bingham wrote:
> On 01/08/18 01:29, Laurent Pinchart wrote:
> > The video control and video streaming interface numbers are needed in
> > the UVC gadget userspace stack to reply to UVC requests. They are
> > hardcoded to fixed values at the moment, preventing configurations with
> > multiple functions.
> > 
> > To fix this, make them dynamically discoverable by userspace through
> > read-only configfs attributes in /control/bInterfaceNumber and
> > /streaming/bInterfaceNumber respectively.
> > 
> > Signed-off-by: Laurent Pinchart 
> > ---
> > 
> >  drivers/usb/gadget/function/f_uvc.c|  2 +
> >  drivers/usb/gadget/function/u_uvc.h|  3 ++
> >  drivers/usb/gadget/function/uvc_configfs.c | 62 +
> It sounds like this is extending the userspace ABI.
> 
> Do we need to document this anywhere?
>  Documentation/ABI/testing/configfs-usb-gadget-uvc ?

You're right, I'll document it.

> >  3 files changed, 67 insertions(+)

[snip]

> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> > b/drivers/usb/gadget/function/uvc_configfs.c index
> > e019ea317c7a..801117686108 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
> > @@ -687,8 +687,39 @@ static const struct uvcg_config_group_type
> > uvcg_control_class_grp_type = {> 
> >   * control
> >   */
> > 
> > +static ssize_t uvcg_default_control_b_interface_number_show(
> > +   struct config_item *item, char *page)
> > +{
> > +   struct config_group *group = to_config_group(item);
> > +   struct mutex *su_mutex = >cg_subsys->su_mutex;
> > +   struct config_item *opts_item;
> > +   struct f_uvc_opts *opts;
> > +   int result = 0;
> > +
> > +   mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> > +
> > +   opts_item = item->ci_parent;
> > +   opts = to_f_uvc_opts(opts_item);
> > +
> > +   mutex_lock(>lock);
> > +   result += sprintf(page, "%u\n", opts->control_interface);
> > +   mutex_unlock(>lock);
> > +
> > +   mutex_unlock(su_mutex);
> > +
> > +   return result;
> > +}
> > +
> > +UVC_ATTR_RO(uvcg_default_control_, b_interface_number, bInterfaceNumber);
> > +
> > +static struct configfs_attribute *uvcg_default_control_attrs[] = {
> > +   _default_control_attr_b_interface_number,
> > +   NULL,
> > +};

[snip]

> > +static ssize_t uvcg_default_streaming_b_interface_number_show(
> > +   struct config_item *item, char *page)
> > +{
> > +   struct config_group *group = to_config_group(item);
> > +   struct mutex *su_mutex = >cg_subsys->su_mutex;
> > +   struct config_item *opts_item;
> > +   struct f_uvc_opts *opts;
> > +   int result = 0;
> > +
> > +   mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> > +
> > +   opts_item = item->ci_parent;
> > +   opts = to_f_uvc_opts(opts_item);
> > +
> > +   mutex_lock(>lock);
> > +   result += sprintf(page, "%u\n", opts->streaming_interface);
> > +   mutex_unlock(>lock);
> > +
> > +   mutex_unlock(su_mutex);
> > +
> > +   return result;
> > +}
> 
> That looks like a lot of common boilerplate code copied for each file
> which prints a single %u.
> 
> Perhaps we should convert those to a macro - but for now they look fine.

Feel free to submit patches :-) Helper functions would be event better than 
macros to decrease the object size.

> > +
> > +UVC_ATTR_RO(uvcg_default_streaming_, b_interface_number,
> > bInterfaceNumber);
> > +static struct configfs_attribute
> > *uvcg_default_streaming_attrs[] = {
> > +   _default_streaming_attr_b_interface_number,
> > +   NULL,
> > +};

[snip]

-- 
Regards,

Laurent Pinchart



--
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 3/8] usb: gadget: uvc: configfs: Allocate groups dynamically

[Laurent Pinchart]

Hi Kieran,

On Wednesday, 1 August 2018 12:58:40 EEST Kieran Bingham wrote:
> On 01/08/18 01:29, Laurent Pinchart wrote:
> > The UVC configfs implementation creates all groups as global static
> > variables. This prevents creationg of multiple UVC function instances,
> 
> /creationg/creation/
> 
> > as they would all require their own configfs group instances.
> > 
> > Fix this by allocating all groups dynamically. To avoid duplicating code
> > around, extend the config_item_type structure with group name and
> > children, and implement helper functions to create children
> > automatically for most groups.
> > 
> > Signed-off-by: Laurent Pinchart 
> 
> I'm struggling to see what paths free all of the dynamically created
> children in this patch.
> 
> Is this already supported by the config_group framework?
> 
> I see a reference to config_group_put(>func_inst.group); in one
> error path - but that's about it.
> 
> Am I missing something nice and obvious? (or is it already handled by
> framework code not in this patch)
> 
> 
> In fact, I can't see how it could be handled by core - as the children
> are added to a new structure you have created.
> 
> I'll let you look into this :)

I did, for a whole day :-S It wasn't easy as the configfs documentation is 
quite terse, and doesn't clearly explain when and how references should be 
acquired and released. I'll post a v2 shortly.

> > ---
> > 
> >  drivers/usb/gadget/function/f_uvc.c|   8 +-
> >  drivers/usb/gadget/function/uvc_configfs.c | 480 ++--
> >  2 files changed, 282 insertions(+), 206 deletions(-)

[snip]


> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> > b/drivers/usb/gadget/function/uvc_configfs.c index
> > dbc95c9558de..e019ea317c7a 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c

[snip]

> > -static struct config_group uvcg_terminal_grp;
> > -
> > -static const struct config_item_type uvcg_terminal_grp_type = {
> > -   .ct_owner = THIS_MODULE,
> > +static const struct uvcg_config_group_type uvcg_terminal_grp_type = {
> > +   .type = {
> > +   .ct_owner = THIS_MODULE,
> > +   },
> > +   .name = "terminal",
> > +   .children = (const struct uvcg_config_group_type*[]) {
> 
> Is this cast really needed? Or is it just to constify the array ?

It's needed to make the expression within the curly braces an array that is 
then turned into a pointer to initialize the children field, which is defined 
as

const struct uvcg_config_group_type **children;

Without that you would get the following compilation errors.

drivers/usb/gadget/function/uvc_configfs.c:557:2: error: braces around scalar 
initializer [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:557:2: error: (near initialization 
for ‘uvcg_terminal_grp_type.children’) [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:558:3: error: initialization from 
incompatible pointer type [-Werror] 
drivers/usb/gadget/function/uvc_configfs.c:558:3: error: (near initialization 
for ‘uvcg_terminal_grp_type.children’) [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:559:3: error: excess elements in 
scalar initializer [-Werror] 
drivers/usb/gadget/function/uvc_configfs.c:559:3: error: (near initialization 
for ‘uvcg_terminal_grp_type.children’) [-Werror]
drivers/usb/gadget/function/uvc_configfs.c:560:3: error: excess elements in 
scalar initializer [-Werror] 
drivers/usb/gadget/function/uvc_configfs.c:560:3: error: (near initialization 
for ‘uvcg_terminal_grp_type.children’) [-Werror]

Such a syntax removes the need to declare the array externally to 
uvcg_terminal_grp_type as

static const struct uvcg_config_group_type *uvcg_terminal_grp_children[] = {
_camera_grp_type,
_output_grp_type,
NULL,
};

static const struct uvcg_config_group_type uvcg_terminal_grp_type = {
.type = {
.ct_owner = THIS_MODULE,
},
.name = "terminal",
.children = uvcg_terminal_grp_children,
};

> > +   _camera_grp_type,
> > +   _output_grp_type,
> > +   NULL,
> > +   },
> >  };

[snip]

-- 
Regards,

Laurent Pinchart



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

[Erika@EuropeMillions]

--
Congratulations, You Have won SIX HUNDRED AND FIFTY THOUSAND EURO in the
monthly Euro/ Google draws on July 1, 2018. reply to fill your claim 
form.


Erika Hermann
Online Coordinator
Desk038984EU
--
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: serial: ftdi_sio: Add support for CBUS GPIO

[Loic Poulain]

Thanks Andy,

On 1 August 2018 at 18:08, Andy Shevchenko  wrote:
> On Wed, Aug 1, 2018 at 6:46 PM, Loic Poulain  wrote:
>> Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
>> pins, allowing host to control them via simple USB control transfers.
>> To make use of a CBUS pin in Bit Bang mode, the pin must be configured
>> to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
>> USB to Serial function.
>>
>> In this implementation, a GPIO controller is registered on FTDI probe
>> if at least one CBUS pin is configured for I/O mode. For now, only
>> FTX devices are supported.
>>
>
>> This patch is based on previous Stefan Agner implementation tentative
>> on LKML ([PATCH 0/2] FTDI CBUS GPIO support).
>
> The best approach to refer to a mail is to put a message-id, something like
>
> (... [1].)
>
> [1]: Message-Id: <...message-id...>
>
>> +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
>> +   void *val, size_t bytes)
>> +{
>
>> +   if (bytes % 2) /* 16-bit eeprom */
>> +   return -EINVAL;
>
> Why is it fatal?
> You may read one byte less (and provide an error code like -EIO, or
> whatever fits better) or more (and provide exact amount asked).

Yes, indeed.

>
>> +   return 0;
>> +}
>
>> +   rv = ftdi_read_pins(fgc->port, );
>> +   if (rv)
>> +   dev_err(>dev, "Unable to read CBUS GPIO pins, %d\n", 
>> rv);
>
> Why not to return an error?
>
> (Message itself sounds like a noise. Perhaps, dev_dbg() or throw away.

Will do.

>
>> +   rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask, 
>> FTDI_SIO_BITMODE_CBUS);
>> +   if (rv)
>> +   dev_err(>dev, "Unable set CBUS Bit-Bang mode, %d\n", 
>> rv);
>
> Ditto WRT message.

yes

>
>> +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
>> +{
>> +   struct ftdi_private *priv = usb_get_serial_port_data(port);
>> +   struct usb_device *udev = port->serial->dev;
>> +   struct ftdi_gpiochip *fgc;
>> +   int rv;
>> +
>
>> +   fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
>> +   if (!fgc)
>> +   return -ENOMEM;
>
> devm_ ?
>
>> +   cbus_mux = kmalloc(4, GFP_KERNEL);
>> +   if (!cbus_mux)
>> +   return -ENOMEM;
>
> Is it mandatory to alloc so small amount on heap in this case?

Yes, because it is used as USB transfer buffer (DMA...) which can not
be on the stack.

>
>> +   /* CBUS pins are individually configurable via 8-bit MUX 
>> control
>> +* value, living at 0x1a for CBUS0. cf application note 
>> AN_201.
>> +*/
>
> Is it a comment style used in this file?
>
>> +   rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);
>
> 0x1a is a magic.
>
>> +   rv = gpiochip_add_data(>gc, fgc);
>> +   if (rv) {
>> +   dev_err(>dev, "Unable to add gpiochip\n");
>> +   kfree(fgc);
>> +   return rv;
>> +   }
>
> devm_ ?
>
>> +   return 0;
>> +}
>> +
>> +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
>> +{
>> +   struct ftdi_private *priv = usb_get_serial_port_data(port);
>> +   struct ftdi_gpiochip *fgc = priv->fgc;
>> +
>
>> +   if (fgc) {
>
> How you can be here when fgc == NULL?!
>
>> +   gpiochip_remove(>gc);
>> +   kfree(fgc);
>> +   }
>> +}
>
> I guess complete function will go away when you switch to devm.
>
>
> --
> With Best Regards,
> Andy Shevchenko

Regards,
Loic
--
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: fusb302 type-c chip driver supply cutting out

[Guenter Roeck]

On Wed, Aug 01, 2018 at 11:37:56AM -0700, Tim Harvey wrote:
> On Tue, Jul 31, 2018 at 2:22 AM Adam Thomson
>  wrote:
> >
> > On 27 July 2018 17:41, Tim Harvey wrote:
> >
> > Adding Guenter to the thread.
> >
> > > Greetings,
> > >
> > > I have a custom design with a Fairchild FUSB302 Type-C chip driver
> > > that I'm testing with Linux 4.17 and a BTI AC-60TC 60W charger. For
> > > this design we are using Type-C as a power/charger input only - no
> > > USB2/3 is connected. The board consumes approximately 12W typical
> > > which doesn't leave enough to charge the battery in a timely fashion
> > > unless switching up to a higher voltage via USB-PD.
> > >
> > > I'm finding that when there is no fusb302 driver enabled the charger
> > > puts out 5V@3A as expected but when I enable the fusb302 driver the
> > > charger cuts out momentarily when the fusb302 driver comes up on its
> > > way to switching up to 20V which causes the board to reboot.
> > >
> > > Here's my dts config for a 20V@3A 60W capable sink:
> > >
> > > fusb302@0x24 {
> > > compatible = "fcs,fusb302";
> > > pinctrl-names = "default";
> > > pinctrl-0 = <_usbpd>;
> > > reg = <0x24>;
> > > interrupt-parent = <>;
> > > interrupts = <10 IRQ_TYPE_EDGE_FALLING>;
> > > fcs,max-sink-microvolt = <2000>;
> > > fcs,max-sink-microamp = <300>;
> > > fcs,max-sink-microwatt = <6000>;
> > > };
> > >
> > >
> > > Here's what I see from the kernel while booting without a battery:
> > > [6.122858] typec_fusb302 1-0024: 1-0024 supply vbus not found,
> > > using dummy regulator
> > > ^ supply cuts out momentarily at this point causing board reboot
> > >
> > > Here's what I see from /sys/kernel/debug/tcpm when I add a battery on
> > > the system with enough charge to keep it powered during the VUSB cut:
> > > [6.134543] Setting voltage/current limit 0 mV 0 mA
> > > [6.134960] polarity 0
> > > [6.135304] Requesting mux mode 0, usb-role 0, orientation 0
> > > [6.136271] state change INVALID_STATE -> SNK_UNATTACHED
> > > [6.136334] CC1: 0 -> 0, CC2: 0 -> 0 [state SNK_UNATTACHED,
> > > polarity 0, disconnected]
> > > [6.136358] 1-0024: registered
> > > [6.144183] Setting voltage/current limit 0 mV 0 mA
> > > [6.144452] polarity 0
> > > [6.144586] Requesting mux mode 0, usb-role 0, orientation 0
> > > [6.145592] cc:=0
> > > [6.164039] pending state change PORT_RESET -> PORT_RESET_WAIT_OFF @ 
> > > 100
> > > ms
> > > [6.176918] VBUS off
> > > ^ this appears to be where the charger first cuts out
> >
> > The port is being reset here to return the port to a known state as part of
> > initialisation. It then seems that the Source/Charger is resetting it's VBUS
> > down to 0V for ~1s which is most likely it performing a hard reset.
> 
> Is this normal? I'm not clear why a Source/Charger would reset its VBUS.
> 
> >
> > >
> > > [6.273374] state change PORT_RESET -> PORT_RESET_WAIT_OFF [delayed 100
> > > ms]
> > > [6.273394] state change PORT_RESET_WAIT_OFF -> SNK_UNATTACHED
> > > [6.273403] Start DRP toggling
> > > [6.297315] CC1: 0 -> 0, CC2: 0 -> 5 [state DRP_TOGGLING, polarity
> > > 0, connected]
> > > [6.297327] state change DRP_TOGGLING -> SNK_ATTACH_WAIT
> > > [6.297529] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200
> > > ms
> > > [6.409960] VBUS on
> > > [6.513218] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200
> > > ms]
> > > [6.513236] state change SNK_DEBOUNCED -> SNK_ATTACHED
> > > [6.513244] polarity 1
> > > [6.513251] Requesting mux mode 1, usb-role 2, orientation 2
> > > [6.515121] state change SNK_ATTACHED -> SNK_STARTUP
> > > [6.515418] state change SNK_STARTUP -> SNK_DISCOVERY
> > > [6.515428] Setting voltage/current limit 5000 mV 3000 mA
> > > [6.515462] vbus=0 charge:=1
> > > [6.515536] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
> > > [6.530767] pending state change SNK_WAIT_CAPABILITIES ->
> > > HARD_RESET_SEND @ 240 ms
> > > [6.567379] PD RX, header: 0x6161 [1]
> > > [6.567407]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> > > [6.567417]  PDO 1: type 0, 9000 mV, 3000 mA []
> > > [6.567427]  PDO 2: type 0, 12000 mV, 3000 mA []
> > > [6.567436]  PDO 3: type 0, 15000 mV, 3000 mA []
> > > [6.567444]  PDO 4: type 0, 18000 mV, 3000 mA []
> > > [6.567452]  PDO 5: type 0, 2 mV, 3000 mA []
> > > [6.567459] state change SNK_WAIT_CAPABILITIES ->
> > > SNK_NEGOTIATE_CAPABILITIES
> > > [6.567643] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> > > [6.567657] Requesting PDO 5: 2 mV, 3000 mA
> > > [6.567665] PD TX, header: 0x1042
> > > [6.580208] PD TX complete, status: 0
> > > [6.580539] pending state change SNK_NEGOTIATE_CAPABILITIES ->
> > > HARD_RESET_SEND @ 60 ms
> > > [6.653644] 

Re: fusb302 type-c chip driver supply cutting out

[Tim Harvey]

On Tue, Jul 31, 2018 at 2:22 AM Adam Thomson
 wrote:
>
> On 27 July 2018 17:41, Tim Harvey wrote:
>
> Adding Guenter to the thread.
>
> > Greetings,
> >
> > I have a custom design with a Fairchild FUSB302 Type-C chip driver
> > that I'm testing with Linux 4.17 and a BTI AC-60TC 60W charger. For
> > this design we are using Type-C as a power/charger input only - no
> > USB2/3 is connected. The board consumes approximately 12W typical
> > which doesn't leave enough to charge the battery in a timely fashion
> > unless switching up to a higher voltage via USB-PD.
> >
> > I'm finding that when there is no fusb302 driver enabled the charger
> > puts out 5V@3A as expected but when I enable the fusb302 driver the
> > charger cuts out momentarily when the fusb302 driver comes up on its
> > way to switching up to 20V which causes the board to reboot.
> >
> > Here's my dts config for a 20V@3A 60W capable sink:
> >
> > fusb302@0x24 {
> > compatible = "fcs,fusb302";
> > pinctrl-names = "default";
> > pinctrl-0 = <_usbpd>;
> > reg = <0x24>;
> > interrupt-parent = <>;
> > interrupts = <10 IRQ_TYPE_EDGE_FALLING>;
> > fcs,max-sink-microvolt = <2000>;
> > fcs,max-sink-microamp = <300>;
> > fcs,max-sink-microwatt = <6000>;
> > };
> >
> >
> > Here's what I see from the kernel while booting without a battery:
> > [6.122858] typec_fusb302 1-0024: 1-0024 supply vbus not found,
> > using dummy regulator
> > ^ supply cuts out momentarily at this point causing board reboot
> >
> > Here's what I see from /sys/kernel/debug/tcpm when I add a battery on
> > the system with enough charge to keep it powered during the VUSB cut:
> > [6.134543] Setting voltage/current limit 0 mV 0 mA
> > [6.134960] polarity 0
> > [6.135304] Requesting mux mode 0, usb-role 0, orientation 0
> > [6.136271] state change INVALID_STATE -> SNK_UNATTACHED
> > [6.136334] CC1: 0 -> 0, CC2: 0 -> 0 [state SNK_UNATTACHED,
> > polarity 0, disconnected]
> > [6.136358] 1-0024: registered
> > [6.144183] Setting voltage/current limit 0 mV 0 mA
> > [6.144452] polarity 0
> > [6.144586] Requesting mux mode 0, usb-role 0, orientation 0
> > [6.145592] cc:=0
> > [6.164039] pending state change PORT_RESET -> PORT_RESET_WAIT_OFF @ 100
> > ms
> > [6.176918] VBUS off
> > ^ this appears to be where the charger first cuts out
>
> The port is being reset here to return the port to a known state as part of
> initialisation. It then seems that the Source/Charger is resetting it's VBUS
> down to 0V for ~1s which is most likely it performing a hard reset.

Is this normal? I'm not clear why a Source/Charger would reset its VBUS.

>
> >
> > [6.273374] state change PORT_RESET -> PORT_RESET_WAIT_OFF [delayed 100
> > ms]
> > [6.273394] state change PORT_RESET_WAIT_OFF -> SNK_UNATTACHED
> > [6.273403] Start DRP toggling
> > [6.297315] CC1: 0 -> 0, CC2: 0 -> 5 [state DRP_TOGGLING, polarity
> > 0, connected]
> > [6.297327] state change DRP_TOGGLING -> SNK_ATTACH_WAIT
> > [6.297529] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200
> > ms
> > [6.409960] VBUS on
> > [6.513218] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200
> > ms]
> > [6.513236] state change SNK_DEBOUNCED -> SNK_ATTACHED
> > [6.513244] polarity 1
> > [6.513251] Requesting mux mode 1, usb-role 2, orientation 2
> > [6.515121] state change SNK_ATTACHED -> SNK_STARTUP
> > [6.515418] state change SNK_STARTUP -> SNK_DISCOVERY
> > [6.515428] Setting voltage/current limit 5000 mV 3000 mA
> > [6.515462] vbus=0 charge:=1
> > [6.515536] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
> > [6.530767] pending state change SNK_WAIT_CAPABILITIES ->
> > HARD_RESET_SEND @ 240 ms
> > [6.567379] PD RX, header: 0x6161 [1]
> > [6.567407]  PDO 0: type 0, 5000 mV, 3000 mA [E]
> > [6.567417]  PDO 1: type 0, 9000 mV, 3000 mA []
> > [6.567427]  PDO 2: type 0, 12000 mV, 3000 mA []
> > [6.567436]  PDO 3: type 0, 15000 mV, 3000 mA []
> > [6.567444]  PDO 4: type 0, 18000 mV, 3000 mA []
> > [6.567452]  PDO 5: type 0, 2 mV, 3000 mA []
> > [6.567459] state change SNK_WAIT_CAPABILITIES ->
> > SNK_NEGOTIATE_CAPABILITIES
> > [6.567643] cc=0 cc1=0 cc2=5 vbus=0 vconn=sink polarity=1
> > [6.567657] Requesting PDO 5: 2 mV, 3000 mA
> > [6.567665] PD TX, header: 0x1042
> > [6.580208] PD TX complete, status: 0
> > [6.580539] pending state change SNK_NEGOTIATE_CAPABILITIES ->
> > HARD_RESET_SEND @ 60 ms
> > [6.653644] state change SNK_NEGOTIATE_CAPABILITIES ->
> > HARD_RESET_SEND [delayed 60 ms]
> > [6.653659] PD TX, type: 0x5
> > [6.684725] PD TX complete, status: 0
> > [6.684783] state change HARD_RESET_SEND -> HARD_RESET_START
> > [6.723530] state change HARD_RESET_START -> 

Re: [PATCH] USB: serial: ftdi_sio: Add support for CBUS GPIO

[Andy Shevchenko]

On Wed, Aug 1, 2018 at 6:46 PM, Loic Poulain  wrote:
> Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
> pins, allowing host to control them via simple USB control transfers.
> To make use of a CBUS pin in Bit Bang mode, the pin must be configured
> to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
> USB to Serial function.
>
> In this implementation, a GPIO controller is registered on FTDI probe
> if at least one CBUS pin is configured for I/O mode. For now, only
> FTX devices are supported.
>

> This patch is based on previous Stefan Agner implementation tentative
> on LKML ([PATCH 0/2] FTDI CBUS GPIO support).

The best approach to refer to a mail is to put a message-id, something like

(... [1].)

[1]: Message-Id: <...message-id...>

> +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
> +   void *val, size_t bytes)
> +{

> +   if (bytes % 2) /* 16-bit eeprom */
> +   return -EINVAL;

Why is it fatal?
You may read one byte less (and provide an error code like -EIO, or
whatever fits better) or more (and provide exact amount asked).

> +   return 0;
> +}

> +   rv = ftdi_read_pins(fgc->port, );
> +   if (rv)
> +   dev_err(>dev, "Unable to read CBUS GPIO pins, %d\n", 
> rv);

Why not to return an error?

(Message itself sounds like a noise. Perhaps, dev_dbg() or throw away.

> +   rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask, 
> FTDI_SIO_BITMODE_CBUS);
> +   if (rv)
> +   dev_err(>dev, "Unable set CBUS Bit-Bang mode, %d\n", 
> rv);

Ditto WRT message.

> +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
> +{
> +   struct ftdi_private *priv = usb_get_serial_port_data(port);
> +   struct usb_device *udev = port->serial->dev;
> +   struct ftdi_gpiochip *fgc;
> +   int rv;
> +

> +   fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
> +   if (!fgc)
> +   return -ENOMEM;

devm_ ?

> +   cbus_mux = kmalloc(4, GFP_KERNEL);
> +   if (!cbus_mux)
> +   return -ENOMEM;

Is it mandatory to alloc so small amount on heap in this case?

> +   /* CBUS pins are individually configurable via 8-bit MUX 
> control
> +* value, living at 0x1a for CBUS0. cf application note 
> AN_201.
> +*/

Is it a comment style used in this file?

> +   rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);

0x1a is a magic.

> +   rv = gpiochip_add_data(>gc, fgc);
> +   if (rv) {
> +   dev_err(>dev, "Unable to add gpiochip\n");
> +   kfree(fgc);
> +   return rv;
> +   }

devm_ ?

> +   return 0;
> +}
> +
> +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
> +{
> +   struct ftdi_private *priv = usb_get_serial_port_data(port);
> +   struct ftdi_gpiochip *fgc = priv->fgc;
> +

> +   if (fgc) {

How you can be here when fgc == NULL?!

> +   gpiochip_remove(>gc);
> +   kfree(fgc);
> +   }
> +}

I guess complete function will go away when you switch to devm.


-- 
With Best Regards,
Andy Shevchenko
--
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: serial: ftdi_sio: Add support for CBUS GPIO

[Loic Poulain]

Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
pins, allowing host to control them via simple USB control transfers.
To make use of a CBUS pin in Bit Bang mode, the pin must be configured
to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
USB to Serial function.

In this implementation, a GPIO controller is registered on FTDI probe
if at least one CBUS pin is configured for I/O mode. For now, only
FTX devices are supported.

This patch is based on previous Stefan Agner implementation tentative
on LKML ([PATCH 0/2] FTDI CBUS GPIO support).

Signed-off-by: Loic Poulain 
---
 drivers/usb/serial/Kconfig|   9 ++
 drivers/usb/serial/ftdi_sio.c | 222 ++
 drivers/usb/serial/ftdi_sio.h |  83 
 3 files changed, 314 insertions(+)

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 533f127..64c9f2e 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -181,6 +181,15 @@ config USB_SERIAL_FTDI_SIO
  To compile this driver as a module, choose M here: the
  module will be called ftdi_sio.
 
+config USB_SERIAL_FTDI_SIO_CBUS_GPIO
+   bool "USB FDTI CBUS GPIO support"
+   depends on USB_SERIAL_FTDI_SIO
+   depends on GPIOLIB
+   help
+ Say yes here to add support for the CBUS bit-bang mode, allowing CBUS
+ pins to act as GPIOs. Note that pins must first be configured for GPIO
+ in the device's EEPROM. The FT232R and FT-X series support this mode.
+
 config USB_SERIAL_VISOR
tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver"
help
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index b5cef32..3cfb5fd 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -40,12 +41,19 @@
 #include 
 #include 
 #include 
+
 #include "ftdi_sio.h"
 #include "ftdi_sio_ids.h"
 
 #define DRIVER_AUTHOR "Greg Kroah-Hartman , Bill Ryder 
, Kuba Ober , Andreas Mohr, Johan Hovold 
"
 #define DRIVER_DESC "USB FTDI Serial Converters Driver"
 
+struct ftdi_gpiochip {
+   struct gpio_chip gc;
+   unsigned int cbus_map[4];
+   unsigned long cbus_mask;
+   struct usb_serial_port *port;
+};
 
 struct ftdi_private {
enum ftdi_chip_type chip_type;
@@ -72,6 +80,8 @@ struct ftdi_private {
unsigned int latency;   /* latency setting in use */
unsigned short max_packet_size;
struct mutex cfg_lock; /* Avoid mess by parallel calls of config 
ioctl() and change_speed() */
+
+   struct ftdi_gpiochip *fgc;
 };
 
 /* struct ftdi_sio_quirk is used by devices requiring special attention. */
@@ -1528,6 +1538,211 @@ static int get_lsr_info(struct usb_serial_port *port,
return 0;
 }
 
+#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO
+
+static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
+   void *val, size_t bytes)
+{
+   if (bytes % 2) /* 16-bit eeprom */
+   return -EINVAL;
+
+   while (bytes) {
+   int rv;
+
+   rv = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+FTDI_SIO_READ_EEPROM_REQUEST,
+FTDI_SIO_READ_EEPROM_REQUEST_TYPE,
+0, off / 2, val, 2, WDR_TIMEOUT);
+   if (rv < 0)
+   return rv;
+
+   off += 2;
+   val += 2;
+   bytes -= 2;
+   }
+
+   return 0;
+}
+
+static int ftdi_set_bitmode(struct usb_serial_port *port, u8 bitmask,
+   u8 bitmode)
+{
+   struct ftdi_private *priv = usb_get_serial_port_data(port);
+   __u16 urb_value = 0;
+
+   urb_value = bitmode << 8 | bitmask;
+
+   if (usb_control_msg(port->serial->dev,
+usb_sndctrlpipe(port->serial->dev, 0),
+FTDI_SIO_SET_BITMODE_REQUEST,
+FTDI_SIO_SET_BITMODE_REQUEST_TYPE,
+urb_value, priv->interface,
+NULL, 0, WDR_SHORT_TIMEOUT) < 0) {
+   return -EIO;
+   }
+
+   return 0;
+}
+
+static int ftdi_read_pins(struct usb_serial_port *port, u8 *val)
+{
+   struct ftdi_private *priv = usb_get_serial_port_data(port);
+   unsigned char *buf;
+
+   buf = kmalloc(1, GFP_KERNEL);
+   if (!buf)
+   return -ENOMEM;
+
+   if (usb_control_msg(port->serial->dev,
+usb_rcvctrlpipe(port->serial->dev, 0),
+FTDI_SIO_READ_PINS_REQUEST,
+FTDI_SIO_READ_PINS_REQUEST_TYPE,
+0, priv->interface, buf, 1, WDR_TIMEOUT) < 0) {
+   kfree(buf);
+   return -EIO;
+   }
+
+   

Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers

[Vincent Pelletier]

Hello,

Bo He, CC'ed, found a regression introduced in my patch, discussed in
this thread, and submitted a patch:
  Subject: [PATCH] fix panic at pwq_activate_delayed_work.
  Date: Wed, 1 Aug 2018 10:14:38 +
  Message-ID: 


On Fri, 29 Jun 2018 09:32:43 +0300, Felipe Balbi
 wrote:
> Hmm, that's what I remember, but we don't have that documented and dwc3
> has a sleep in its dequeue, which I need to remove for other reasons
> anyway.

Given the above comment from Felipe, I expected my patch would be
dropped in favour of making dwc3 not sleep in dequeue, as it seems to
be the actual root cause.

Should my patch be reverted ? It adds complexity which, I believe,
becomes superfluous if dequeue does not sleep anywhere.

Or maybe non-sleeping dequeue is not there yet, and a solution right now
(later revertable) is better, in which case my change would be worth
fixing ?
-- 
Vincent Pelletier
--
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 6/8] usb: gadget: uvc: configfs: Document the bFormatIndex attribute

[Kieran Bingham]

Hi Laurent,

On 01/08/18 01:29, Laurent Pinchart wrote:
> Document for the bFormatIndex attribute of the format descriptors is
> missing. Add it.

It might be missing, but only just since the previous patch.

Perhaps this should squash into [5/8] ?


> Signed-off-by: Laurent Pinchart 
> ---
>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc 
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 9281e2aa38df..9896c8aa0e14 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -160,6 +160,10 @@ Description: Specific MJPEG format descriptors
>  
>   All attributes read only,
>   except bmaControls and bDefaultFrameIndex:
> + bFormatIndex- unique id for this format descriptor;
> + only defined after parent header is
> + linked into the streaming class;
> + read-only
>   bmaControls - this format's data for bmaControls in
>   the streaming header
>   bmInterfaceFlags- specifies interlace information,
> @@ -204,6 +208,10 @@ Date:Dec 2014
>  KernelVersion:   4.0
>  Description: Specific uncompressed format descriptors
>  
> + bFormatIndex- unique id for this format descriptor;
> + only defined after parent header is
> + linked into the streaming class;
> + read-only
>   bmaControls - this format's data for bmaControls in
>   the streaming header
>   bmInterfaceFlags- specifies interlace information,
> 

-- 
Regards
--
Kieran
--
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 4/8] usb: gadget: uvc: configfs: Add interface number attributes

[Kieran Bingham]

Hi Laurent,

Thank you for the patch,

On 01/08/18 01:29, Laurent Pinchart wrote:
> The video control and video streaming interface numbers are needed in
> the UVC gadget userspace stack to reply to UVC requests. They are
> hardcoded to fixed values at the moment, preventing configurations with
> multiple functions.
> 
> To fix this, make them dynamically discoverable by userspace through
> read-only configfs attributes in /control/bInterfaceNumber and
> /streaming/bInterfaceNumber respectively.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/usb/gadget/function/f_uvc.c|  2 +
>  drivers/usb/gadget/function/u_uvc.h|  3 ++
>  drivers/usb/gadget/function/uvc_configfs.c | 62 
> ++



It sounds like this is extending the userspace ABI.

Do we need to document this anywhere?
 Documentation/ABI/testing/configfs-usb-gadget-uvc ?


>  3 files changed, 67 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c 
> b/drivers/usb/gadget/function/f_uvc.c
> index 95cb1b5f5ffe..4ea987741e6e 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -699,12 +699,14 @@ uvc_function_bind(struct usb_configuration *c, struct 
> usb_function *f)
>   uvc_iad.bFirstInterface = ret;
>   uvc_control_intf.bInterfaceNumber = ret;
>   uvc->control_intf = ret;
> + opts->control_interface = ret;
>  
>   if ((ret = usb_interface_id(c, f)) < 0)
>   goto error;
>   uvc_streaming_intf_alt0.bInterfaceNumber = ret;
>   uvc_streaming_intf_alt1.bInterfaceNumber = ret;
>   uvc->streaming_intf = ret;
> + opts->streaming_interface = ret;
>  
>   /* Copy descriptors */
>   f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> diff --git a/drivers/usb/gadget/function/u_uvc.h 
> b/drivers/usb/gadget/function/u_uvc.h
> index 2ed292e94fbc..5242d489e20a 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -25,6 +25,9 @@ struct f_uvc_opts {
>   unsigned intstreaming_maxpacket;
>   unsigned intstreaming_maxburst;
>  
> + unsigned intcontrol_interface;
> + unsigned intstreaming_interface;
> +
>   /*
>* Control descriptors array pointers for full-/high-speed and
>* super-speed. They point by default to the uvc_fs_control_cls and
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
> b/drivers/usb/gadget/function/uvc_configfs.c
> index e019ea317c7a..801117686108 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -687,8 +687,39 @@ static const struct uvcg_config_group_type 
> uvcg_control_class_grp_type = {
>   * control
>   */
>  
> +static ssize_t uvcg_default_control_b_interface_number_show(
> + struct config_item *item, char *page)
> +{
> + struct config_group *group = to_config_group(item);
> + struct mutex *su_mutex = >cg_subsys->su_mutex;
> + struct config_item *opts_item;
> + struct f_uvc_opts *opts;
> + int result = 0;
> +
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> + opts_item = item->ci_parent;
> + opts = to_f_uvc_opts(opts_item);
> +
> + mutex_lock(>lock);
> + result += sprintf(page, "%u\n", opts->control_interface);
> + mutex_unlock(>lock);
> +
> + mutex_unlock(su_mutex);
> +
> + return result;
> +}
> +
> +UVC_ATTR_RO(uvcg_default_control_, b_interface_number, bInterfaceNumber);
> +
> +static struct configfs_attribute *uvcg_default_control_attrs[] = {
> + _default_control_attr_b_interface_number,
> + NULL,
> +};
> +
>  static const struct uvcg_config_group_type uvcg_control_grp_type = {
>   .type = {
> + .ct_attrs = uvcg_default_control_attrs,
>   .ct_owner = THIS_MODULE,
>   },
>   .name = "control",
> @@ -2246,8 +2277,39 @@ static const struct uvcg_config_group_type 
> uvcg_streaming_class_grp_type = {
>   * streaming
>   */
>  
> +static ssize_t uvcg_default_streaming_b_interface_number_show(
> + struct config_item *item, char *page)
> +{
> + struct config_group *group = to_config_group(item);
> + struct mutex *su_mutex = >cg_subsys->su_mutex;
> + struct config_item *opts_item;
> + struct f_uvc_opts *opts;
> + int result = 0;
> +
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> + opts_item = item->ci_parent;
> + opts = to_f_uvc_opts(opts_item);
> +
> + mutex_lock(>lock);
> + result += sprintf(page, "%u\n", opts->streaming_interface);
> + mutex_unlock(>lock);
> +
> + mutex_unlock(su_mutex);
> +
> + return result;
> +}

That looks like a lot of common boilerplate code copied for each file
which prints a single %u.

Perhaps we should convert those to a macro - but for now they look 

Re: [PATCH 3/8] usb: gadget: uvc: configfs: Allocate groups dynamically

[Kieran Bingham]

Hi Laurent,

Thank you for the patch,

On 01/08/18 01:29, Laurent Pinchart wrote:
> The UVC configfs implementation creates all groups as global static
> variables. This prevents creationg of multiple UVC function instances,

/creationg/creation/

> as they would all require their own configfs group instances.
> 
> Fix this by allocating all groups dynamically. To avoid duplicating code
> around, extend the config_item_type structure with group name and
> children, and implement helper functions to create children
> automatically for most groups.
> 
> Signed-off-by: Laurent Pinchart 

I'm struggling to see what paths free all of the dynamically created
children in this patch.

Is this already supported by the config_group framework?

I see a reference to config_group_put(>func_inst.group); in one
error path - but that's about it.

Am I missing something nice and obvious? (or is it already handled by
framework code not in this patch)


In fact, I can't see how it could be handled by core - as the children
are added to a new structure you have created.

I'll let you look into this :)



> ---
>  drivers/usb/gadget/function/f_uvc.c|   8 +-
>  drivers/usb/gadget/function/uvc_configfs.c | 480 
> +
>  2 files changed, 282 insertions(+), 206 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c 
> b/drivers/usb/gadget/function/f_uvc.c
> index d8ce7868fe22..95cb1b5f5ffe 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -792,6 +792,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
>   struct uvc_output_terminal_descriptor *od;
>   struct uvc_color_matching_descriptor *md;
>   struct uvc_descriptor_header **ctl_cls;
> + int ret;
>  
>   opts = kzalloc(sizeof(*opts), GFP_KERNEL);
>   if (!opts)
> @@ -868,7 +869,12 @@ static struct usb_function_instance *uvc_alloc_inst(void)
>   opts->streaming_interval = 1;
>   opts->streaming_maxpacket = 1024;
>  
> - uvcg_attach_configfs(opts);
> + ret = uvcg_attach_configfs(opts);
> + if (ret < 0) {
> + kfree(opts);
> + return ERR_PTR(ret);
> + }
> +
>   return >func_inst;
>  }
>  
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
> b/drivers/usb/gadget/function/uvc_configfs.c
> index dbc95c9558de..e019ea317c7a 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -41,6 +41,49 @@ static inline struct f_uvc_opts *to_f_uvc_opts(struct 
> config_item *item)
>   func_inst.group);
>  }
>  
> +struct uvcg_config_group_type {
> + struct config_item_type type;
> + const char *name;
> + const struct uvcg_config_group_type **children;
> + int (*create_children)(struct config_group *group);
> +};
> +
> +static int uvcg_config_create_group(struct config_group *parent,
> + const struct uvcg_config_group_type *type);
> +
> +static int uvcg_config_create_children(struct config_group *group,
> + const struct uvcg_config_group_type *type)
> +{
> + const struct uvcg_config_group_type **child;
> + int ret;
> +
> + if (type->create_children)
> + return type->create_children(group);
> +
> + for (child = type->children; child && *child; ++child) {
> + ret = uvcg_config_create_group(group, *child);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int uvcg_config_create_group(struct config_group *parent,
> + const struct uvcg_config_group_type *type)
> +{
> + struct config_group *group;
> +
> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> + if (!group)
> + return -ENOMEM;
> +
> + config_group_init_type_name(group, type->name, >type);
> + configfs_add_default_group(group, parent);
> +
> + return uvcg_config_create_children(group, type);
> +}
> +
>  /* 
> -
>   * control/header/
>   * control/header
> @@ -169,24 +212,23 @@ static void uvcg_control_header_drop(struct 
> config_group *group,
>   kfree(h);
>  }
>  
> -static struct config_group uvcg_control_header_grp;
> -
>  static struct configfs_group_operations uvcg_control_header_grp_ops = {
>   .make_item  = uvcg_control_header_make,
>   .drop_item  = uvcg_control_header_drop,
>  };
>  
> -static const struct config_item_type uvcg_control_header_grp_type = {
> - .ct_group_ops   = _control_header_grp_ops,
> - .ct_owner   = THIS_MODULE,
> +static const struct uvcg_config_group_type uvcg_control_header_grp_type = {
> + .type = {
> + .ct_group_ops   = _control_header_grp_ops,
> + .ct_owner   = THIS_MODULE,
> + },
> + .name = "header",
>  };
>  
>  /* 
> 

Re: [PATCH 2/8] usb: gadget: uvc: configfs: Add section header comments

[Kieran Bingham]

Hi Laurent,

A trivial but nice improvement.

On 01/08/18 01:29, Laurent Pinchart wrote:
> The UVC configfs implementation is large and difficult to navigate. Add
> a bit more air to the code to make it easier to read.
> 
> Signed-off-by: Laurent Pinchart 

Acked-by: Kieran Bingham 


> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 120 
> ++---
>  1 file changed, 91 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
> b/drivers/usb/gadget/function/uvc_configfs.c
> index 1df94b25abe1..dbc95c9558de 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -12,6 +12,10 @@
>  #include "u_uvc.h"
>  #include "uvc_configfs.h"
>  
> +/* 
> -
> + * Global Utility Structures and Macros
> + */
> +
>  #define UVCG_STREAMING_CONTROL_SIZE  1
>  
>  #define UVC_ATTR(prefix, cname, aname) \
> @@ -37,7 +41,11 @@ static inline struct f_uvc_opts *to_f_uvc_opts(struct 
> config_item *item)
>   func_inst.group);
>  }
>  
> -/* control/header/ */
> +/* 
> -
> + * control/header/
> + * control/header
> + */
> +
>  DECLARE_UVC_HEADER_DESCRIPTOR(1);
>  
>  struct uvcg_control_header {
> @@ -161,7 +169,6 @@ static void uvcg_control_header_drop(struct config_group 
> *group,
>   kfree(h);
>  }
>  
> -/* control/header */
>  static struct config_group uvcg_control_header_grp;
>  
>  static struct configfs_group_operations uvcg_control_header_grp_ops = {
> @@ -174,7 +181,10 @@ static const struct config_item_type 
> uvcg_control_header_grp_type = {
>   .ct_owner   = THIS_MODULE,
>  };
>  
> -/* control/processing/default */
> +/* 
> -
> + * control/processing/default
> + */
> +
>  static struct config_group uvcg_default_processing_grp;
>  
>  #define UVCG_DEFAULT_PROCESSING_ATTR(cname, aname, conv) \
> @@ -260,16 +270,20 @@ static const struct config_item_type 
> uvcg_default_processing_type = {
>   .ct_owner   = THIS_MODULE,
>  };
>  
> -/* struct uvcg_processing {}; */
> +/* 
> -
> + * control/processing
> + */
>  
> -/* control/processing */
>  static struct config_group uvcg_processing_grp;
>  
>  static const struct config_item_type uvcg_processing_grp_type = {
>   .ct_owner = THIS_MODULE,
>  };
>  
> -/* control/terminal/camera/default */
> +/* 
> -
> + * control/terminal/camera/default
> + */
> +
>  static struct config_group uvcg_default_camera_grp;
>  
>  #define UVCG_DEFAULT_CAMERA_ATTR(cname, aname, conv) \
> @@ -366,16 +380,20 @@ static const struct config_item_type 
> uvcg_default_camera_type = {
>   .ct_owner   = THIS_MODULE,
>  };
>  
> -/* struct uvcg_camera {}; */
> +/* 
> -
> + * control/terminal/camera
> + */
>  
> -/* control/terminal/camera */
>  static struct config_group uvcg_camera_grp;
>  
>  static const struct config_item_type uvcg_camera_grp_type = {
>   .ct_owner = THIS_MODULE,
>  };
>  
> -/* control/terminal/output/default */
> +/* 
> -
> + * control/terminal/output/default
> + */
> +
>  static struct config_group uvcg_default_output_grp;
>  
>  #define UVCG_DEFAULT_OUTPUT_ATTR(cname, aname, conv) \
> @@ -433,23 +451,30 @@ static const struct config_item_type 
> uvcg_default_output_type = {
>   .ct_owner   = THIS_MODULE,
>  };
>  
> -/* struct uvcg_output {}; */
> +/* 
> -
> + * control/terminal/output
> + */
>  
> -/* control/terminal/output */
>  static struct config_group uvcg_output_grp;
>  
>  static const struct config_item_type uvcg_output_grp_type = {
>   .ct_owner = THIS_MODULE,
>  };
>  
> -/* control/terminal */
> +/* 
> -
> + * control/terminal
> + */
> +
>  static struct config_group uvcg_terminal_grp;
>  
>  static const struct config_item_type uvcg_terminal_grp_type = {
>   .ct_owner = THIS_MODULE,
>  };
>  
> -/* control/class/{fs} */
> +/* 
> -
> + * control/class/{fs|ss}
> + */
> +
>  static struct config_group uvcg_control_class_fs_grp;
>  static struct config_group uvcg_control_class_ss_grp;
>  
> @@ -552,24 +577,32 @@ static const struct config_item_type 
> uvcg_control_class_type = {
>   .ct_owner   = THIS_MODULE,
>  };
>  
> -/* control/class */
> +/* 
> 

Re: [PATCH 2/3] usb: dwc3: gadget: Don't skip updating remaining data

[Felipe Balbi]

Felipe Balbi  writes:

> Hi,
>
> Felipe Balbi  writes:
>
> 
>
>>> This is an issue, but it's not the same one.
>>>
>>>  irq/16-dwc3-5032  [003] d...65.404194: dwc3_complete_trb:
>>> ep1out: trb 890522d5 buf b8d6d000 size 0 ctrl 3b56446c
>>> (hlCS:Sc:isoc-first)
>>>
>>
>> So this is chained to the next one, that's correct.
>>
>>
>>>  irq/16-dwc3-5032  [003] d...65.404209: dwc3_complete_trb:
>>> ep1out: trb c15f388f buf 37821000 size 1023 ctrl
>>> 3b564c78 (hlcS:SC:isoc)
>>>
>>
>> But here, HWO should've been left as is, because of the short packet.
>> That's what the databook says on the two notes on section 8.2.3 after
>> table 8-8 of Databook 2.60a:
>>
>> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a
>> short packet, the chained TRBs that follow it are not written back
>> (e.g. the BUFSIZ and HWO fields remain the same as the
>> software-prepared value)
>>
>> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is
>> also set), and a short packet is received, the core retires the TRB in
>> progress and skip past the TRB where CHN=0, accumulating the ISP and
>> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core
>> generates an XferInProgress event. Hardware does not set the HWO bit
>> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0
>> TRB will also be retired and its buffer size field updated with the
>> total number of bytes remaining in the BD.
>>
>> Note that at most we have confirmation that SIZE will be updated in
>> case of Isoc endpoints, but there's nothing there about HWO, so I
>> expected it to remain set according to the rest of the text.
>>
>> There's one possibility that "TRB will also be *retired*" means that
>> it's not skipped, which would mean that HWO is, indeed, set to 0. To
>> patch that, however, we don't need all the extra checking you have
>> implemented. I'll try to propose a slightly simpler fix if you're
>> willing to test it out.
>
> Something like below:

and here's another possibility. This makes it clearer that we want to
skip all TRBs with CHN bit set:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9ba614032d5d..56e2a2ebae66 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
dwc3_ep *dep,
struct dwc3_request *req, struct dwc3_trb *trb,
const struct dwc3_event_depevt *event, int status, int chain)
 {
+   const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
unsigned intcount;
 
dwc3_ep_inc_deq(dep);
@@ -2256,6 +2257,16 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
dwc3_ep *dep,
return 1;
}
 
+   /*
+* In case of ISOC endpoints and Short or Zero packets, the
+* last TRB will be retired and its size field will be updated
+* to contain the full remaining size; meaning req->remaining
+* will be count from the last TRB in the chain.
+*/
+   if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc)
+   && chain)
+   return 0;
+
count = trb->size & DWC3_TRB_SIZE_MASK;
req->remaining += count;

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/8] usb: gadget: uvc: configfs: Don't wrap groups unnecessarily

[Kieran Bingham]

Hi Laurent,

Thank you for the patch,

On 01/08/18 01:29, Laurent Pinchart wrote:
> Various configfs groups (represented by config_group) as wrapped in

/as wrapped/are wrapped/

> structures that they're the only member of. This allows adding other
> data fields to groups, but is unnecessarily makes the code more complex.

s/is/it/

> Remove the outter structures and use config_group directly to simplify

/outter/outer/

> the code. Groups can still be wrapped individually in the future if
> other data fields need to be added.
> 
> Signed-off-by: Laurent Pinchart 

phew. Well that is some terse reading of macro modifications.

But except for the trivial spellings above, I can't see anything wrong.
And this definitely moves the right way :D

Reviewed-by: Kieran Bingham 


> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 302 
> +++--
>  1 file changed, 117 insertions(+), 185 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
> b/drivers/usb/gadget/function/uvc_configfs.c
> index b51f0d278826..1df94b25abe1 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -162,9 +162,7 @@ static void uvcg_control_header_drop(struct config_group 
> *group,
>  }
>  
>  /* control/header */
> -static struct uvcg_control_header_grp {
> - struct config_group group;
> -} uvcg_control_header_grp;
> +static struct config_group uvcg_control_header_grp;
>  
>  static struct configfs_group_operations uvcg_control_header_grp_ops = {
>   .make_item  = uvcg_control_header_make,
> @@ -177,31 +175,22 @@ static const struct config_item_type 
> uvcg_control_header_grp_type = {
>  };
>  
>  /* control/processing/default */
> -static struct uvcg_default_processing {
> - struct config_group group;
> -} uvcg_default_processing;
> -
> -static inline struct uvcg_default_processing
> -*to_uvcg_default_processing(struct config_item *item)
> -{
> - return container_of(to_config_group(item),
> - struct uvcg_default_processing, group);
> -}
> +static struct config_group uvcg_default_processing_grp;
>  
>  #define UVCG_DEFAULT_PROCESSING_ATTR(cname, aname, conv) \
>  static ssize_t uvcg_default_processing_##cname##_show(   
> \
>   struct config_item *item, char *page)   \
>  {\
> - struct uvcg_default_processing *dp = to_uvcg_default_processing(item); \
> + struct config_group *group = to_config_group(item); \
>   struct f_uvc_opts *opts;\
>   struct config_item *opts_item;  \
> - struct mutex *su_mutex = >group.cg_subsys->su_mutex;\
> + struct mutex *su_mutex = >cg_subsys->su_mutex;   \
>   struct uvc_processing_unit_descriptor *pd;  \
>   int result; \
>   \
>   mutex_lock(su_mutex); /* for navigating configfs hierarchy */   \
>   \
> - opts_item = dp->group.cg_item.ci_parent->ci_parent->ci_parent;  \
> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
>   opts = to_f_uvc_opts(opts_item);\
>   pd = >uvc_processing; \
>   \
> @@ -229,17 +218,17 @@ UVCG_DEFAULT_PROCESSING_ATTR(i_processing, iProcessing, 
> identity_conv);
>  static ssize_t uvcg_default_processing_bm_controls_show(
>   struct config_item *item, char *page)
>  {
> - struct uvcg_default_processing *dp = to_uvcg_default_processing(item);
> + struct config_group *group = to_config_group(item);
>   struct f_uvc_opts *opts;
>   struct config_item *opts_item;
> - struct mutex *su_mutex = >group.cg_subsys->su_mutex;
> + struct mutex *su_mutex = >cg_subsys->su_mutex;
>   struct uvc_processing_unit_descriptor *pd;
>   int result, i;
>   char *pg = page;
>  
>   mutex_lock(su_mutex); /* for navigating configfs hierarchy */
>  
> - opts_item = dp->group.cg_item.ci_parent->ci_parent->ci_parent;
> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent;
>   opts = to_f_uvc_opts(opts_item);
>   pd = >uvc_processing;
>  
> @@ -274,40 +263,29 @@ static const struct config_item_type 
> uvcg_default_processing_type = {
>  /* struct uvcg_processing {}; */
>  
>  /* control/processing */
> -static struct uvcg_processing_grp {
> - struct config_group group;
> -} uvcg_processing_grp;
> +static struct config_group uvcg_processing_grp;
>  
>  static const struct config_item_type uvcg_processing_grp_type = {
>   .ct_owner = 

Re: [PATCH 2/3] usb: dwc3: gadget: Don't skip updating remaining data

[Felipe Balbi]

Hi,

Felipe Balbi  writes:



>> This is an issue, but it's not the same one.
>>
>>  irq/16-dwc3-5032  [003] d...65.404194: dwc3_complete_trb:
>> ep1out: trb 890522d5 buf b8d6d000 size 0 ctrl 3b56446c
>> (hlCS:Sc:isoc-first)
>>
>
> So this is chained to the next one, that's correct.
>
>
>>  irq/16-dwc3-5032  [003] d...65.404209: dwc3_complete_trb:
>> ep1out: trb c15f388f buf 37821000 size 1023 ctrl
>> 3b564c78 (hlcS:SC:isoc)
>>
>
> But here, HWO should've been left as is, because of the short packet.
> That's what the databook says on the two notes on section 8.2.3 after
> table 8-8 of Databook 2.60a:
>
> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a
> short packet, the chained TRBs that follow it are not written back
> (e.g. the BUFSIZ and HWO fields remain the same as the
> software-prepared value)
>
> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is
> also set), and a short packet is received, the core retires the TRB in
> progress and skip past the TRB where CHN=0, accumulating the ISP and
> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core
> generates an XferInProgress event. Hardware does not set the HWO bit
> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0
> TRB will also be retired and its buffer size field updated with the
> total number of bytes remaining in the BD.
>
> Note that at most we have confirmation that SIZE will be updated in
> case of Isoc endpoints, but there's nothing there about HWO, so I
> expected it to remain set according to the rest of the text.
>
> There's one possibility that "TRB will also be *retired*" means that
> it's not skipped, which would mean that HWO is, indeed, set to 0. To
> patch that, however, we don't need all the extra checking you have
> implemented. I'll try to propose a slightly simpler fix if you're
> willing to test it out.

Something like below:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9ba614032d5d..a8c7271a83b5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
dwc3_ep *dep,
struct dwc3_request *req, struct dwc3_trb *trb,
const struct dwc3_event_depevt *event, int status, int chain)
 {
+   const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
unsigned intcount;
 
dwc3_ep_inc_deq(dep);
@@ -2257,7 +2258,17 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
dwc3_ep *dep,
}
 
count = trb->size & DWC3_TRB_SIZE_MASK;
-   req->remaining += count;
+
+   /*
+* In case of ISOC endpoints and Short or Zero packets, the
+* last TRB will be retired and its size field will be updated
+* to contain the full remaining size; meaning req->remaining
+* will be count from the last TRB in the chain.
+*/
+   if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc))
+   req->remaining = count;
+   else
+   req->remaining += count;
 
if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
return 1;

If this minimal fix works, then as a follow-up I'll make a cleanup patch
to make this part of the code slightly better. We're starting to get way
too many conditionals here and they can be cleaned up a little.

I just wanna know if the minimal fix works for you and if it does, we
can mark it for stable and make sure the backport goes as far back as
possible.

cheers

-- 
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 2/3] usb: dwc3: gadget: Don't skip updating remaining data

[Felipe Balbi]

Hi,

First of all, not receiving your mails through the mailing list. Can you
check that you're not
being blocked for some reason? You may wanna send trace data compressed
with gzip or xz.

On Wed, Aug 1, 2018 at 2:27 AM Thinh Nguyen 
wrote:

> On 7/30/2018 11:58 PM, Felipe Balbi wrote:
> > Thinh Nguyen  writes:
> >> Hi Felipe,
> >>
> >> On 7/29/2018 11:08 PM, Felipe Balbi wrote:
> >>> Thinh Nguyen  writes:
>  DWC3 must check for the BUFSIZ and update the req->remaining
>  regardless of transfer alignment. Returning early from transfer OUT
>  unalignment will skip updating the req->remaining.
> 
>  Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to
> wMaxPacketSize")
>  Signed-off-by: Thinh Nguyen 
>  ---
>   drivers/usb/dwc3/gadget.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
>  diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>  index 032ea7d709ba..a5b8387a37ba 100644
>  --- a/drivers/usb/dwc3/gadget.c
>  +++ b/drivers/usb/dwc3/gadget.c
>  @@ -2246,6 +2246,9 @@ static int
> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
> trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> 
>  +  count = trb->size & DWC3_TRB_SIZE_MASK;
>  +  req->remaining += count;
>  +
> /*
>  * If we're dealing with unaligned size OUT transfer, we will be
> left
>  * with one TRB pending in the ring. We need to manually clear HWO
> bit
>  @@ -2256,9 +2259,6 @@ static int
> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> return 1;
> }
> 
>  -  count = trb->size & DWC3_TRB_SIZE_MASK;
>  -  req->remaining += count;
>  -
> >>> this shouldn't be necessary, actually. The remaining TRB which was
> added
> >>> to fix alignment issues, is simply because DWC3 doesn't like OUT
> >>> transfers whose size are not aligned to wMaxPacketSize. We *know* we
> >>> have e.g. 511 bytes to receive from host, but dwc3 wants me to write
> >>> 1024 bytes on the TRB (for superspeed), so I need to add this chained
> >>> TRB for the remaining 513 bytes.
> >>>
> >>> I don't expect host to send us more than 511 bytes, if it does, the
> host
> >>> is faulty and I don't want to operate on those bytes anyway.
> >>>
> >> Right. But that's not the issue here. I realized that I did not describe
> >> the issue fully...
> >>
> >> The issue here is the reporting of the actual number of bytes
> >> transferred. For isoc OUT transfers, the last TRB of the Buffer
> >> Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31
> >> programming guide 4.3.7). So, the check for unaligned && HWO will be
> >> false and will not return early. As a result, req->remaining will still
> >> be updated with the BUFSIZ count, and the remaining will be 513+ in your
> >> example. The 'actual' bytes written calculation will be wrong (e.g 511 -
> >> 513 = -2).
> >>
> >> We can't rely on the current check to determine whether it's the last
> >> TRB. My current solution is to update req->remaining for all TRB
> >> completions. However, we have to also update the 'actual' bytes
> >> calculation for OUT transfers.
> > I think the bug here is another one. Look at the TRB types:
> >
> >  irq/16-dwc3-2463  [003] d...  3589.933478: dwc3_complete_trb:
> ep1out: trb 7d3fbc0d buf b8ed3800 size 0 ctrl 2e46446c
> (hlCS:Sc:isoc-first)

>  irq/16-dwc3-2463  [003] d...  3589.933492: dwc3_complete_trb:
> ep1out: trb d1fcb0c4 buf 3787e000 size 1023 ctrl 2e464c68
> (hlcS:SC:isoc-first)

>
> > Shouldn't the one be of type ISOC instead of ISOC_FIRST? Can you test
> > patch below and see if things behave better? Please also capture
> > tracepoints :)
> >
> > modified   drivers/usb/dwc3/gadget.c
> > @@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep
> *dep,
> >   /* Now prepare one extra TRB to align transfer
> size */
> >   trb = >trb_pool[dep->trb_enqueue];
> >   __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
> > - maxp - rem, false, 0,
> > + maxp - rem, false, 1,
> >   req->request.stream_id,
> >   req->request.short_not_ok,
> >   req->request.no_interrupt);
> > @@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct
> dwc3_ep *dep,
> >   /* Now prepare one extra TRB to align transfer size */
> >   trb = >trb_pool[dep->trb_enqueue];
> >   __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp -
> rem,
> > - false, 0, req->request.stream_id,
> > + false, 1, req->request.stream_id,
> >