Skip to content

USB: netusb: Cleanup and switch to USB transfer API #13985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Mar 13, 2019
Merged

USB: netusb: Cleanup and switch to USB transfer API #13985

merged 5 commits into from
Mar 13, 2019

Conversation

@finikorg
Copy link
Contributor

@finikorg finikorg commented Mar 1, 2019 •
edited
Loading

Cleanup and fixes.

Fixes #14127
Fixes #14416

@finikorg finikorg requested a review from jfischer-no as a code owner March 1, 2019 14:12
@codecov-io
Copy link

codecov-io commented Mar 1, 2019 •
edited
Loading

Codecov Report

Merging #13985 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13985      +/-   ##
==========================================
+ Coverage   51.97%   51.97%   +<.01%     
==========================================
  Files         308      308              
  Lines       45512    45512              
  Branches    10546    10546              
==========================================
+ Hits        23656    23657       +1     
  Misses      17055    17055              
+ Partials     4801     4800       -1
Impacted Files Coverage Δ
arch/posix/core/posix_core.c 91.91% <0%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c78c5e6...04d835a. Read the comment docs.

finikorg added 3 commits March 11, 2019 15:10
@finikorg
usb: netusb: eem: Use sys_get_le16() helper
09e4cc6 
Use helper to make code cleaner.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
@finikorg
usb: netusb: ecm: Remove forward declaration
a5300ff 
Make code more readable.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
@finikorg
usb: netusb: rndis: Cleanup code
2992486 
Remove unnecessary line breaks.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
@finikorg finikorg mentioned this pull request Mar 11, 2019
Closed
@finikorg finikorg changed the title USB: netusb: Cleanup USB: netusb: Cleanup and switch to USB transfer API Mar 11, 2019
@finikorg finikorg requested review from aurel32 and masz-nordic March 11, 2019 13:29
aurel32
aurel32 suggested changes Mar 11, 2019
View reviewed changes
Copy link
Contributor

@aurel32 aurel32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside the issue with the TX buffer size, it looks good to me. It's a nice cleanup and I have tested it fixes the issue reported in #14127.

subsys/usb/class/netusb/function_rndis.c Outdated
Copy link
Contributor

@aurel32 aurel32 Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This array holds the Ethernet packet, but also the RNDIS header. I guess its size should therefore be 1500 + sizeof(struct rndis_payload_packet) or even better `NETUSB_MTU + sizeof(struct rndis_payload_packet).

Copy link
Contributor Author

@finikorg finikorg Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

finikorg added 2 commits March 12, 2019 10:26
@finikorg
usb: rndis: Use usb_transfer() for data transfer
6a331ab 
Use USB transfer API for data transfer. Simplify notification count,
removing delayed work, use usb_transfer() logic instead.

Fixes zephyrproject-rtos#14127

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
@finikorg
usb: netusb: Remove unused helpers and cleanup
04d835a 
Remove try_write() which is not used anymore and general cleanup.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
aurel32
aurel32 approved these changes Mar 12, 2019
View reviewed changes
Copy link
Contributor

@aurel32 aurel32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick update, LGTM now.

@galak galak merged commit d293d0c into zephyrproject-rtos:master Mar 13, 2019
@finikorg
Copy link
Contributor Author

finikorg commented Mar 14, 2019

Beside the issue with the TX buffer size, it looks good to me. It's a nice cleanup and I have tested it fixes the issue reported in #14127.

@aurel32 Have you tested it on Windows? For me it works in Windows but not in Linux, I suspect this is issue in stm32 USB controller driver.

@aurel32
Copy link
Contributor

aurel32 commented Mar 14, 2019

@finikorg Nope I only tested it on Linux (4.19 kernel), with a SAM E70 Xplained board. I can try with an STM32 board in the next days.

@finikorg finikorg deleted the netusb branch March 15, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jfischer-no jfischer-no jfischer-no approved these changes

@masz-nordic masz-nordic masz-nordic approved these changes

+1 more reviewer

@aurel32 aurel32 aurel32 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Coverity CID :195789]Uninitialized variables in /subsys/usb/class/netusb/function_rndis.c netusb: TX path doesn't work in RNDIS driver

6 participants

@finikorg @codecov-io @aurel32 @jfischer-no @masz-nordic @galak