Skip to content
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.

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

cpu/native/can/candev_linux: add check for real can #13342

Merged
merged 1 commit into from
Feb 24, 2020

Conversation

wosym
Copy link
Member

@wosym wosym commented Feb 11, 2020

Contribution description

This is the fix I proposed in #13309. Like I said there: it's still up for debate whether this fix is the one we want/need, or if there is something else that needs patching (because it's possible that this patch only suppresses the symptoms, and does not address the root cause).

Anyhow, for me it seems it does not break conn_can (which, as far as I can tell is currently the only app using the code that is altered by this PR), and it does fix the issue for other uses of the candev_native.
Also: it makes this function call consistent with other calls, because the same check is done before these others (see line 305 in candev_linux.c).
And even more so: it makes conn_can compliant with its own "rules".

Testing procedure

  • Run tests/conn_can: it should not be broken after this fix

Issues/PRs references

Fixes #13309.

@miri64 miri64 added Area: drivers Area: Device drivers Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 11, 2020
@miri64 miri64 requested a review from vincent-d February 11, 2020 14:30
@benpicco benpicco requested a review from kaspar030 February 11, 2020 14:56
@kaspar030
Copy link
Contributor

@wysman can you elaborate on why and how this fix works?

@wosym
Copy link
Member Author

wosym commented Feb 11, 2020

@kaspar030 I guess you meant to tag me instead of wysman?

I wrote down my entire thoughtprocess in #13309.
In short:
With libsocketcan you can enable and disable can-interfaces using the can_do_stop/start() functions. This should always return 0 or -1, according to the docs. However, it doesn't (always) do this when the can interface is a virtual one (vcanx). Then segfaults can occur under some (currently still unknown) circumstances.
The docs don't specify that you're not allowed to use these functions on vcans (and to be honest: I would see no reason why you shouldn't be), but I read other reports on Google of people having issues using these libsocketcan functions on vcans. My guess is that this is more an issue with libsocketcan than with RIOT.
The README of conn_can also mentions this:
You can also set a bitrate (this won't work on native with vcan, only with real interfaces).
(The set_bitrate function is the function using the faulty functioncalls. )

Also: I think this patch is how it was originally intended I think, because on line 305 the exact same check is done when calling _set_bitrate. It was probably overlooked at the time of writing.

@kaspar030
Copy link
Contributor

I guess you meant to tag me instead of wysman?

Yes :) sorry @wysman.

Thanks for the detailed explanation. Could you add a short comment, as in, /* only set bitrate on real can interfaces. Not supported on virtual can interfaces ("vcanX") */?

@wosym
Copy link
Member Author

wosym commented Feb 11, 2020

@kaspar030 : Sure, done!
I added it to both the original check, and to the new one. (Because I think if one place requires the comment, it also requires it the second time such a check is done)
I hope this is ok?

@vincent-d
Copy link
Member

I didn't remember there was this check already in the code. This looks a bit hacky to me because it relies on the name, and I can create a virtual CAN called can0. But I guess we don't have a better solution to check if the interface is a virtual one or a real one.

@wosym
Copy link
Member Author

wosym commented Feb 12, 2020

Fair point @vincent-d . Maybe we should add some warning for it in the readme?
Something along the lines of
Warning: when using virtual can devices, they must always be named 'vcanX'

@bergzand bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 13, 2020
@wosym
Copy link
Member Author

wosym commented Feb 17, 2020

@kaspar030 : what do you think? Add the extra warning, or merge straight away?

@smlng
Copy link
Member

smlng commented Feb 20, 2020

otherwise this looks good to me

@wosym
Copy link
Member Author

wosym commented Feb 21, 2020

I squashed the commits to make Travis happy. (He complained about the length of the commit-message)
However, now Murdock seems to be complaining, but I'm not sure what's happening:
"dwqw: command timed out"
Is the CI broken somehow? Other PR's seem to be having a similar issue.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 21, 2020
@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 21, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 21, 2020
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 21, 2020
@wosym
Copy link
Member Author

wosym commented Feb 24, 2020

CI issue seems to be fixed. Ready to ACK?

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030 kaspar030 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: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Feb 24, 2020
@kaspar030 kaspar030 merged commit 58450c6 into RIOT-OS:master Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers 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: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cpu/native/can: Segmentation fault when manipulating vcan's through socketcan
8 participants