| Message ID | 20200818165848.4117493-1-lorenzo@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] usb: gadget: f_ncm: fix ncm_bitrate for SuperSpeed and above. | expand |
On Tue, Aug 18, 2020 at 9:59 AM Lorenzo Colitti <lorenzo@google.com> wrote: > > Currently, SuperSpeed NCM gadgets report a speed of 851 Mbps > in USB_CDC_NOTIFY_SPEED_CHANGE. But the calculation appears to > assume 16 packets per microframe, and USB 3 and above no longer > use microframes. > > Maximum speed is actually much higher. On a direct connection, > theoretical throughput is about 3.86 Gbps for gen1x1 and > 9.35 Gbps for gen2x1, and I have seen gadget->host speeds > >2 Gbps for gen1x1 and >4 Gbps for gen2x1. > > Unfortunately the ConnectionSpeedChange defined in the CDC spec > only uses 32-bit values, so we can't report accurate numbers for > 10Gbps and above. So always report a speed of 4 Gbps, which is > close enough to the technical maximum of a 5 Gbps link. > > This results in: > > [96033.958723] cdc_ncm 8-2:1.0 enx4643f5db6f40: renamed from usb0 > [96033.997136] cdc_ncm 8-2:1.0 enx4643f5db6f40: 4000 mbit/s downlink 4000 mbit/s uplink > > Fixes: 1650113888fe ("usb: gadget: f_ncm: add SuperSpeed descriptors for CDC NCM") > Signed-off-by: Lorenzo Colitti <lorenzo@google.com> > --- > drivers/usb/gadget/function/f_ncm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c > index 1d900081b1..0c073df225 100644 > --- a/drivers/usb/gadget/function/f_ncm.c > +++ b/drivers/usb/gadget/function/f_ncm.c > @@ -85,8 +85,8 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f) > /* peak (theoretical) bulk transfer rate in bits-per-second */ > static inline unsigned ncm_bitrate(struct usb_gadget *g) > { > - if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER) > - return 13 * 1024 * 8 * 1000 * 8; > + if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER) > + return 4000000000; > else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH) > return 13 * 512 * 8 * 1000 * 8; > else > -- > 2.28.0.220.ged08abb693-goog > Do you know what this actually affects besides the display? My cursory investigation shows it gets printed to kernel log and sent over some sort of ncm notification to the other side... Is it better to underestimate or overestimate? (ie. would it be better to report 3.5 gbps for super and max out at 4.2 gbps for super plus instead?) However, since this is obviously better than the utterly bogus 851,968,000: Reviewed-by: Maciej Żenczykowski <maze@google.com>
Hi Lorenzo,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on balbi-usb/testing/next]
[also build test WARNING on usb/usb-testing peter.chen-usb/ci-for-usb-next v5.9-rc1 next-20200818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lorenzo-Colitti/usb-gadget-f_ncm-fix-ncm_bitrate-for-SuperSpeed-and-above/20200819-010231
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:11,
from drivers/usb/gadget/function/f_ncm.c:14:
include/linux/scatterlist.h: In function 'sg_set_buf':
arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~
include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_ncm.c: In function 'ncm_bitrate':
>> drivers/usb/gadget/function/f_ncm.c:89:3: warning: this decimal constant is unsigned only in ISO C90
89 | return 4000000000;
| ^~~~~~
# https://github.com/0day-ci/linux/commit/e5b632f5aecf9c4d7d1bd3e17cf3ff699c1c3201
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Lorenzo-Colitti/usb-gadget-f_ncm-fix-ncm_bitrate-for-SuperSpeed-and-above/20200819-010231
git checkout e5b632f5aecf9c4d7d1bd3e17cf3ff699c1c3201
vim +89 drivers/usb/gadget/function/f_ncm.c
84
85 /* peak (theoretical) bulk transfer rate in bits-per-second */
86 static inline unsigned ncm_bitrate(struct usb_gadget *g)
87 {
88 if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER)
> 89 return 4000000000;
90 else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
91 return 13 * 512 * 8 * 1000 * 8;
92 else
93 return 19 * 64 * 1 * 1000 * 8;
94 }
95
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Aug 19, 2020 at 6:39 AM Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > - if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER) > > - return 13 * 1024 * 8 * 1000 * 8; > > + if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER) > > + return 4000000000; Will respin to change this to 4000000000U to address the warning reported by the kernel test robot. > Do you know what this actually affects besides the display? > My cursory investigation shows it gets printed to kernel log and sent > over some sort of ncm notification to the other side... Yes, it's sent in the ConnectionSpeedChange notifications which are intended to inform the host about how fast the link is. For a direct connection over a USB cable this does not make much sense, but for, say, a Gigabit Ethernet dongle that uses NCM, you'd probably want to inform the host of whether the connection is 10, 100, or 1000M. This is not what the code does now, obviously. > Is it better to underestimate or overestimate? > (ie. would it be better to report 3.5 gbps for super and max out at > 4.2 gbps for super plus instead?) I don't think it matters much. I'm happy to put 3860000000 for SuperSpeed and 4200000000 for SuperSpeed Plus, or whatever else we think makes sense. The speed is theoretical anyway. I suppose reporting different speeds might be useful to debug whether the connection is using 5G or >= 10G. Felipe, any opinions?
On Wed, Aug 19, 2020 at 6:56 AM Lorenzo Colitti <lorenzo@google.com> wrote: > > On Wed, Aug 19, 2020 at 6:39 AM Maciej Żenczykowski > <zenczykowski@gmail.com> wrote: > > > - if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER) > > > - return 13 * 1024 * 8 * 1000 * 8; > > > + if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER) > > > + return 4000000000; > > Will respin to change this to 4000000000U to address the warning > reported by the kernel test robot. > > > Do you know what this actually affects besides the display? > > My cursory investigation shows it gets printed to kernel log and sent > > over some sort of ncm notification to the other side... > > Yes, it's sent in the ConnectionSpeedChange notifications which are > intended to inform the host about how fast the link is. For a direct > connection over a USB cable this does not make much sense, but for, > say, a Gigabit Ethernet dongle that uses NCM, you'd probably want to > inform the host of whether the connection is 10, 100, or 1000M. This > is not what the code does now, obviously. > > > Is it better to underestimate or overestimate? > > (ie. would it be better to report 3.5 gbps for super and max out at > > 4.2 gbps for super plus instead?) > > I don't think it matters much. I'm happy to put 3860000000 for > SuperSpeed and 4200000000 for SuperSpeed Plus, or whatever else we > think makes sense. The speed is theoretical anyway. I suppose > reporting different speeds might be useful to debug whether the > connection is using 5G or >= 10G. > > Felipe, any opinions? Oh reporting diff speeds to make it more obvious what happened and whether SS+ is in use... that does seem like a win.
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index 1d900081b1..0c073df225 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -85,8 +85,8 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f) /* peak (theoretical) bulk transfer rate in bits-per-second */ static inline unsigned ncm_bitrate(struct usb_gadget *g) { - if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER) - return 13 * 1024 * 8 * 1000 * 8; + if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER) + return 4000000000; else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH) return 13 * 512 * 8 * 1000 * 8; else