Skip to content

cdcecm: early exit on inactive usb interface#12534

Merged
dylad merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/usbus/cdcecm_early_exit_on_inactive
Oct 23, 2019
Merged

cdcecm: early exit on inactive usb interface#12534
dylad merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/usbus/cdcecm_early_exit_on_inactive

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Oct 21, 2019 •
edited
Loading

Contribution description

This adds an early exit when the usb interface with the data endpoints
is not activated. This prevents the cdc_ecm_netdev code from attempting
to send the PDU when the USB device is not yet initialized or activated
by a host.

Also prevents an null pointer access when the netdev code happens to initialize before the usbus code.

Testing procedure

  1. Enable DEBUG in sys/usb/usbus/cdc/ecm/cdc_ecm.c.
  2. flash examples/usbus_cdc_ecm on a supported board.
  3. From the RIOT shell: ping6 ff02::1%5
  4. Verify that this and this debug logging from sys/usb/usbus/cdc/ecm/cdc_ecm.c is not triggered.
  5. Attach to a host with support for CDC ECM
  6. repeat the ping and verify that it triggers debug logging.

Issues/PRs references

None, but maybe test this together with #12533

@bergzand bergzand added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: USB Area: Universal Serial Bus labels Oct 21, 2019
@bergzand bergzand requested review from benpicco and dylad October 21, 2019 18:10
dylad
dylad reviewed Oct 22, 2019
View reviewed changes
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 22, 2019

To clarify the test instructions a bit, there is debug logging in cdc_ecm.c to show transmissions (device perspective) to the USB host here and here. With this PR those should not be triggered if the device is booted without USB connected during boot and during step 1..4 of the test.

bergzand
bergzand commented Oct 22, 2019
View reviewed changes
sys/usb/usbus/cdc/ecm/cdc_ecm_netdev.c Outdated
/* interface with alternative function ID 1 is the interface containing the
* data endpoints, no sense trying to transmit data if it is not active */
if (cdcecm->active_iface != 1) {
return -EBUSY;
Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 22, 2019

Choose a reason for hiding this comment

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

Is -EBUSY the correct error code here? @miri64 Do you have an opinion on this? What should a _send() call return when the interface is unable to transmit a PDU?

Copy link
Copy Markdown
Member

@miri64 miri64 Oct 22, 2019

Choose a reason for hiding this comment

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

Mhhh... -EBUSY was introduced in #11264 to signal the device is busy (even though I abandoned that idea for now for at86rf2xx at least). Is that the case here as well? How about -ENXIO (No such device or address)?

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 22, 2019

Choose a reason for hiding this comment

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

The case here is that either the 'cable' is not connected or the other end of the 'cable' is not configured.

'cable': the point-to-point network data stream over the USB endpoints.

-EBUSY is probably wrong in that case, -ENXIO is a better match, -ENOTCONN could also work and based on a quick look the Linux kernel also seems to use it for the USB gadget framework.

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 22, 2019

Choose a reason for hiding this comment

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

replaced -EBUSY with -ENOTCONN

@dylad dylad added this to the Release 2020.01 milestone Oct 22, 2019
@dylad dylad added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Oct 22, 2019
dylad
dylad approved these changes Oct 22, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Provided testing procedure is OK
Changes look good.
Please squash !

@bergzand
cdcecm: early exit on inactive usb interface
4574557 
This adds an early exit when the usb interface with the data endpoints
is not activated. This prevents the cdc_ecm_netdev code from attempting
to send the PDU when the USB device is not yet initialized or activated
by a host.
@bergzand bergzand force-pushed the pr/usbus/cdcecm_early_exit_on_inactive branch from dc0e52e to 4574557 Compare October 23, 2019 07:41
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 23, 2019

Squashed!

@dylad
Copy link
Copy Markdown
Member

dylad commented Oct 23, 2019

Here we go !

@dylad dylad merged commit 0bbb114 into RIOT-OS:master Oct 23, 2019
@bergzand bergzand mentioned this pull request Oct 23, 2019
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@dylad dylad dylad approved these changes

@benpicco benpicco Awaiting requested review from benpicco

Assignees

No one assigned

Labels

Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@bergzand @dylad @miri64