These two patches improve the isoc handling and make the dwc3 gadget driver
somewhat usable with the UVC gadget for isochronous endpoints.
The first patch makes starting isochronous transfers more reliable. I think
it's more less, what Thinh suggested some time ago[1]. It's still not
perfect because the first request must still be queued within 2 seconds but
it's a lot better than the current situation.
The second patch makes it possible to have gaps in the data stream. The UVC
gadget relies on such behaviour. Without this, using the UVC gadget with a
live stream stops after the first frame, because all later frames are
dropped.
I'm not sure if this is the correct solution, but all other drivers
currently work this way, from what I can tell.
Regards,
Michael
[1] https://marc.info/?l=linux-usb&m=156088170723824&w=4
Michael Olbrich (2):
usb: dwc3: gadget: make starting isoc transfers more robust
usb: dwc3: gadget: restart the transfer if a isoc request is queued
too late
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/gadget.c | 39 +++++++++++++++++++++++++++++++--------
2 files changed, 32 insertions(+), 8 deletions(-)
--
2.20.1
Currently __dwc3_gadget_start_isoc must be called very shortly after
XferNotReady. Otherwise the frame number is outdated and start transfer
will fail, even with several retries.
DSTS provides the lower 14 bit of the frame number. Use it in combination
with the frame number provided by XferNotReady to guess the current frame
number. This will succeed unless more than one 14 rollover has happened
since XferNotReady.
Start transfer might still fail if the frame number is increased
immediately after DSTS is read. So retries are still needed.
Don't drop the current request if this happens. This way it is not lost and
can be used immediately to try again with the next frame number.
With this change, __dwc3_gadget_start_isoc is still not successfully in all
cases bit it increases the acceptable delay after XferNotReady
significantly.
Signed-off-by: Michael Olbrich <[email protected]>
---
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++--------
2 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 3dd783b889cb..c5b223656e08 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -709,6 +709,7 @@ struct dwc3_ep {
u8 type;
u8 resource_index;
u32 frame_number;
+ u32 last_frame_number;
u32 interval;
char name[20];
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 173f5329d3d9..ac4673d91939 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
}
}
-static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
+static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)
{
struct dwc3_gadget_ep_cmd_params params;
struct dwc3_request *req;
@@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
}
ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
- if (ret < 0) {
+ if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
/*
* FIXME we need to iterate over the list of requests
* here and stop, unmap, free and del each of the linked
@@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
return ret;
}
- return 0;
+ return ret;
}
static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
@@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
dep->start_cmd_status = 0;
dep->combo_num = 0;
- return __dwc3_gadget_kick_transfer(dep);
+ return __dwc3_gadget_kick_transfer(dep, false);
}
static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
@@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
}
for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
- dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
+ /*
+ * last_frame_number is set from XferNotReady and may be
+ * already out of date. DSTS only provides the lower 14 bit
+ * of the current frame number. So add the upper two bits of
+ * last_frame_number and handle a possible rollover.
+ * This will provide the correct frame_number unless more than
+ * rollover has happened since XferNotReady.
+ */
+ u32 frame = __dwc3_gadget_get_frame(dwc);
+
+ dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame;
+ if (frame < (dep->last_frame_number & 0x3fff))
+ dep->frame_number += 0x4000;
+ dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
- ret = __dwc3_gadget_kick_transfer(dep);
+ ret = __dwc3_gadget_kick_transfer(dep,
+ i + 1 < DWC3_ISOC_MAX_RETRIES);
if (ret != -EAGAIN)
break;
}
@@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
}
}
- return __dwc3_gadget_kick_transfer(dep);
+ return __dwc3_gadget_kick_transfer(dep, false);
}
static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
@@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
if (!dwc3_gadget_ep_request_completed(req) &&
req->num_pending_sgs) {
- __dwc3_gadget_kick_transfer(dep);
+ __dwc3_gadget_kick_transfer(dep, false);
goto out;
}
@@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
const struct dwc3_event_depevt *event)
{
dep->frame_number = event->parameters;
+ dep->last_frame_number = event->parameters;
}
static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
--
2.20.1
Hi Michael,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on balbi-usb/next]
[also build test WARNING on v5.4-rc7 next-20191111]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Michael-Olbrich/usb-dwc3-gadget-improve-isoc-handling/20191113-032659
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
reproduce: make htmldocs
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>):
Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme.
WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org
WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints'
include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops'
Error: Cannot open file drivers/dma-buf/reservation.c
Error: Cannot open file drivers/dma-buf/reservation.c
Error: Cannot open file drivers/dma-buf/reservation.c
Error: Cannot open file include/linux/reservation.h
Error: Cannot open file include/linux/reservation.h
include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'locked_down' not described in 'security_list_options'
lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found
lib/genalloc.c:1: warning: 'gen_pool_alloc' not found
lib/genalloc.c:1: warning: 'gen_pool_free' not found
lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found
drivers/gpio/gpiolib-of.c:92: warning: Excess function parameter 'dev' description in 'of_gpio_need_valid_mask'
include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client'
>> drivers/usb/dwc3/core.h:724: warning: Function parameter or member 'last_frame_number' not described in 'dwc3_ep'
drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found
drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found
drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found
drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found
include/linux/w1.h:277: warning: Function parameter or member 'of_match_table' not described in 'w1_family'
fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode'
fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode'
fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode'
kernel/dma/coherent.c:1: warning: no structured comments found
include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry'
include/linux/skbuff.h:888: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'list' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
include/net/sock.h:2450: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE'
include/net/sock.h:2450: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE'
include/net/sock.h:2450: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE'
include/linux/netdevice.h:2053: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
include/linux/netdevice.h:2053: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
include/linux/rculist.h:374: warning: Excess function parameter 'cond' description in 'list_for_each_entry_rcu'
include/linux/rculist.h:651: warning: Excess function parameter 'cond' description in 'hlist_for_each_entry_rcu'
mm/util.c:1: warning: 'get_user_pages_fast' not found
drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c:335: warning: Excess function parameter 'dev' description in 'amdgpu_gem_prime_export'
drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c:336: warning: Excess function parameter 'dev' description in 'amdgpu_gem_prime_export'
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:142: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:821: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2821: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:378: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:1: warning: no structured comments found
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1: warning: no structured comments found
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: 'pp_dpm_sclk pp_dpm_mclk pp_dpm_pcie' not found
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:132: warning: Incorrect use of kernel-doc format: * @atomic_obj
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:238: warning: Incorrect use of kernel-doc format: * gpu_info FW provided soc bounding box struct or 0 if not
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager'
vim +724 drivers/usb/dwc3/core.h
72246da40f3719 Felipe Balbi 2011-08-19 @724
:::::: The code at line 724 was first introduced by commit
:::::: 72246da40f3719af3bfd104a2365b32537c27d83 usb: Introduce DesignWare USB3 DRD Driver
:::::: TO: Felipe Balbi <[email protected]>
:::::: CC: Greg Kroah-Hartman <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Hi,
Michael Olbrich wrote:
> Currently __dwc3_gadget_start_isoc must be called very shortly after
> XferNotReady. Otherwise the frame number is outdated and start transfer
> will fail, even with several retries.
>
> DSTS provides the lower 14 bit of the frame number. Use it in combination
> with the frame number provided by XferNotReady to guess the current frame
> number. This will succeed unless more than one 14 rollover has happened
> since XferNotReady.
>
> Start transfer might still fail if the frame number is increased
> immediately after DSTS is read. So retries are still needed.
> Don't drop the current request if this happens. This way it is not lost and
> can be used immediately to try again with the next frame number.
>
> With this change, __dwc3_gadget_start_isoc is still not successfully in all
> cases bit it increases the acceptable delay after XferNotReady
> significantly.
>
> Signed-off-by: Michael Olbrich <[email protected]>
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++--------
> 2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 3dd783b889cb..c5b223656e08 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -709,6 +709,7 @@ struct dwc3_ep {
> u8 type;
> u8 resource_index;
> u32 frame_number;
> + u32 last_frame_number;
There's no need to add a new field for last_frame_number. Just store the
value in a local variable in __dwc3_gadget_start_isoc().
> u32 interval;
>
> char name[20];
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 173f5329d3d9..ac4673d91939 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
> }
> }
>
> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)
> {
> struct dwc3_gadget_ep_cmd_params params;
> struct dwc3_request *req;
> @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> }
>
> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
> - if (ret < 0) {
> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
> /*
> * FIXME we need to iterate over the list of requests
> * here and stop, unmap, free and del each of the linked
> @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> return ret;
> }
>
> - return 0;
> + return ret;
> }
>
> static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
> dep->start_cmd_status = 0;
> dep->combo_num = 0;
>
> - return __dwc3_gadget_kick_transfer(dep);
> + return __dwc3_gadget_kick_transfer(dep, false);
> }
>
> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> }
>
> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
> + /*
> + * last_frame_number is set from XferNotReady and may be
> + * already out of date. DSTS only provides the lower 14 bit
> + * of the current frame number. So add the upper two bits of
> + * last_frame_number and handle a possible rollover.
> + * This will provide the correct frame_number unless more than
> + * rollover has happened since XferNotReady.
> + */
> + u32 frame = __dwc3_gadget_get_frame(dwc);
> +
> + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame;
> + if (frame < (dep->last_frame_number & 0x3fff))
> + dep->frame_number += 0x4000;
Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We
started using 0x3fff in multiple places now, can we create a macro for that?
Also, add an empty line here.
> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
>
> - ret = __dwc3_gadget_kick_transfer(dep);
> + ret = __dwc3_gadget_kick_transfer(dep,
> + i + 1 < DWC3_ISOC_MAX_RETRIES);
> if (ret != -EAGAIN)
> break;
> }
> @@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
> }
> }
>
> - return __dwc3_gadget_kick_transfer(dep);
> + return __dwc3_gadget_kick_transfer(dep, false);
> }
>
> static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
> @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>
> if (!dwc3_gadget_ep_request_completed(req) &&
> req->num_pending_sgs) {
> - __dwc3_gadget_kick_transfer(dep);
> + __dwc3_gadget_kick_transfer(dep, false);
> goto out;
> }
>
> @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
> const struct dwc3_event_depevt *event)
> {
> dep->frame_number = event->parameters;
> + dep->last_frame_number = event->parameters;
> }
>
> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
Other than the comments I provided, this patch looks fine to me.
BR,
Thinh
Hi,
On Wed, Nov 13, 2019 at 03:55:30AM +0000, Thinh Nguyen wrote:
> Michael Olbrich wrote:
> > Currently __dwc3_gadget_start_isoc must be called very shortly after
> > XferNotReady. Otherwise the frame number is outdated and start transfer
> > will fail, even with several retries.
> >
> > DSTS provides the lower 14 bit of the frame number. Use it in combination
> > with the frame number provided by XferNotReady to guess the current frame
> > number. This will succeed unless more than one 14 rollover has happened
> > since XferNotReady.
> >
> > Start transfer might still fail if the frame number is increased
> > immediately after DSTS is read. So retries are still needed.
> > Don't drop the current request if this happens. This way it is not lost and
> > can be used immediately to try again with the next frame number.
> >
> > With this change, __dwc3_gadget_start_isoc is still not successfully in all
> > cases bit it increases the acceptable delay after XferNotReady
> > significantly.
> >
> > Signed-off-by: Michael Olbrich <[email protected]>
> > ---
> > drivers/usb/dwc3/core.h | 1 +
> > drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++--------
> > 2 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 3dd783b889cb..c5b223656e08 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -709,6 +709,7 @@ struct dwc3_ep {
> > u8 type;
> > u8 resource_index;
> > u32 frame_number;
> > + u32 last_frame_number;
>
> There's no need to add a new field for last_frame_number. Just store the
> value in a local variable in __dwc3_gadget_start_isoc().
I'm using it to check for rollover, so __dwc3_gadget_start_isoc does not
help. I introduced it because I caused a second (incorrect) rollover when
the first try failed. But now that I think about it, it should be possible
without the extra variable.
> > u32 interval;
> >
> > char name[20];
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 173f5329d3d9..ac4673d91939 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
> > }
> > }
> >
> > -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> > +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)
> > {
> > struct dwc3_gadget_ep_cmd_params params;
> > struct dwc3_request *req;
> > @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> > }
> >
> > ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
> > - if (ret < 0) {
> > + if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
> > /*
> > * FIXME we need to iterate over the list of requests
> > * here and stop, unmap, free and del each of the linked
> > @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> > return ret;
> > }
> >
> > - return 0;
> > + return ret;
> > }
> >
> > static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> > @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
> > dep->start_cmd_status = 0;
> > dep->combo_num = 0;
> >
> > - return __dwc3_gadget_kick_transfer(dep);
> > + return __dwc3_gadget_kick_transfer(dep, false);
> > }
> >
> > static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> > @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> > }
> >
> > for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
> > - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
> > + /*
> > + * last_frame_number is set from XferNotReady and may be
> > + * already out of date. DSTS only provides the lower 14 bit
> > + * of the current frame number. So add the upper two bits of
> > + * last_frame_number and handle a possible rollover.
> > + * This will provide the correct frame_number unless more than
> > + * rollover has happened since XferNotReady.
> > + */
> > + u32 frame = __dwc3_gadget_get_frame(dwc);
> > +
> > + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame;
> > + if (frame < (dep->last_frame_number & 0x3fff))
> > + dep->frame_number += 0x4000;
>
> Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We
Ok.
> started using 0x3fff in multiple places now, can we create a macro for that?
Makes sense. Any preferences for the name? <something>_MASK I guess, but I
don't know what the correct name for the 14 bit frame number should be.
> Also, add an empty line here.
ok.
> > + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
> >
> > - ret = __dwc3_gadget_kick_transfer(dep);
> > + ret = __dwc3_gadget_kick_transfer(dep,
> > + i + 1 < DWC3_ISOC_MAX_RETRIES);
> > if (ret != -EAGAIN)
> > break;
> > }
> > @@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
> > }
> > }
> >
> > - return __dwc3_gadget_kick_transfer(dep);
> > + return __dwc3_gadget_kick_transfer(dep, false);
> > }
> >
> > static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
> > @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
> >
> > if (!dwc3_gadget_ep_request_completed(req) &&
> > req->num_pending_sgs) {
> > - __dwc3_gadget_kick_transfer(dep);
> > + __dwc3_gadget_kick_transfer(dep, false);
> > goto out;
> > }
> >
> > @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
> > const struct dwc3_event_depevt *event)
> > {
> > dep->frame_number = event->parameters;
> > + dep->last_frame_number = event->parameters;
> > }
> >
> > static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>
> Other than the comments I provided, this patch looks fine to me.
Great. Thanks for the review.
Regards,
Michael
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi,
Michael Olbrich wrote:
> Hi,
>
> On Wed, Nov 13, 2019 at 03:55:30AM +0000, Thinh Nguyen wrote:
>> Michael Olbrich wrote:
>>> Currently __dwc3_gadget_start_isoc must be called very shortly after
>>> XferNotReady. Otherwise the frame number is outdated and start transfer
>>> will fail, even with several retries.
>>>
>>> DSTS provides the lower 14 bit of the frame number. Use it in combination
>>> with the frame number provided by XferNotReady to guess the current frame
>>> number. This will succeed unless more than one 14 rollover has happened
>>> since XferNotReady.
>>>
>>> Start transfer might still fail if the frame number is increased
>>> immediately after DSTS is read. So retries are still needed.
>>> Don't drop the current request if this happens. This way it is not lost and
>>> can be used immediately to try again with the next frame number.
>>>
>>> With this change, __dwc3_gadget_start_isoc is still not successfully in all
>>> cases bit it increases the acceptable delay after XferNotReady
>>> significantly.
>>>
>>> Signed-off-by: Michael Olbrich <[email protected]>
>>> ---
>>> drivers/usb/dwc3/core.h | 1 +
>>> drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++--------
>>> 2 files changed, 24 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 3dd783b889cb..c5b223656e08 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -709,6 +709,7 @@ struct dwc3_ep {
>>> u8 type;
>>> u8 resource_index;
>>> u32 frame_number;
>>> + u32 last_frame_number;
>> There's no need to add a new field for last_frame_number. Just store the
>> value in a local variable in __dwc3_gadget_start_isoc().
> I'm using it to check for rollover, so __dwc3_gadget_start_isoc does not
> help. I introduced it because I caused a second (incorrect) rollover when
> the first try failed. But now that I think about it, it should be possible
> without the extra variable.
We don't need this extra field for dwc3_ep. If you need to add a new
one, then you need to also describe that field in the struct dwc3_ep.
>
>>> u32 interval;
>>>
>>> char name[20];
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 173f5329d3d9..ac4673d91939 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>>> }
>>> }
>>>
>>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)
>>> {
>>> struct dwc3_gadget_ep_cmd_params params;
>>> struct dwc3_request *req;
>>> @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>> }
>>>
>>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
>>> - if (ret < 0) {
>>> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
>>> /*
>>> * FIXME we need to iterate over the list of requests
>>> * here and stop, unmap, free and del each of the linked
>>> @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>> return ret;
>>> }
>>>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>> @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
>>> dep->start_cmd_status = 0;
>>> dep->combo_num = 0;
>>>
>>> - return __dwc3_gadget_kick_transfer(dep);
>>> + return __dwc3_gadget_kick_transfer(dep, false);
>>> }
>>>
>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> }
>>>
>>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
>>> + /*
>>> + * last_frame_number is set from XferNotReady and may be
>>> + * already out of date. DSTS only provides the lower 14 bit
>>> + * of the current frame number. So add the upper two bits of
>>> + * last_frame_number and handle a possible rollover.
>>> + * This will provide the correct frame_number unless more than
>>> + * rollover has happened since XferNotReady.
>>> + */
>>> + u32 frame = __dwc3_gadget_get_frame(dwc);
>>> +
>>> + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame;
>>> + if (frame < (dep->last_frame_number & 0x3fff))
>>> + dep->frame_number += 0x4000;
>> Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We
> Ok.
>
>> started using 0x3fff in multiple places now, can we create a macro for that?
> Makes sense. Any preferences for the name? <something>_MASK I guess, but I
> don't know what the correct name for the 14 bit frame number should be.
Maybe you can use DWC3_DSTS_SOFFN(~0) as a mask.
BR,
Thinh