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

Add missing system calls for ADC driver subsystem #15001

Merged
merged 3 commits into from Mar 30, 2019

Conversation

@andrewboie
Copy link
Collaborator

commented Mar 28, 2019

  • Added syscalls for adc_channel_setup() and adc_read()
  • adc_read_async() support will require significant additional refactoring and possibly some kernel-side memory allocations, punting for this PR
  • Increased privileged stack size to fix observed stack overflows

Tested on frdm_k64f and sam_e70_xplained

Partial fix for #14084

andrewboie added some commits Mar 28, 2019

userspace: add const qualifiers to user copy fns
The source data is never modified.

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
userspace: set default priv stack size to 1024
The original value of 256 was selected more or less randomly
and special cases keep proliferating. Until we have a formal
method of proving maximum syscall stack depth, set to 1024.

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
drivers: adc: add some missing system calls
Setting callbacks is forbidden from user mode.

Some heavier code changes will be needed to support
adc_read_async(), this patch just exposes the config
and read functions for now.

Test case updated to run partially in user mode.

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
@codecov-io

This comment has been minimized.

Copy link

commented Mar 29, 2019

Codecov Report

Merging #15001 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #15001   +/-   ##
=======================================
  Coverage   52.92%   52.92%           
=======================================
  Files         309      309           
  Lines       45268    45268           
  Branches    10451    10451           
=======================================
  Hits        23956    23956           
  Misses      16544    16544           
  Partials     4768     4768

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 9e81cbe...14c5054. Read the comment docs.

@nashif

nashif approved these changes Mar 29, 2019

}

if (dst->options) {
if (z_user_from_copy(options, dst->options,

This comment has been minimized.

Copy link
@ceolin

ceolin Mar 29, 2019

Collaborator

shouldn't copy from src->options ?

This comment has been minimized.

Copy link
@andrewboie

andrewboie Mar 29, 2019

Author Collaborator

at this point in the code, src and dst have the same contents since we just previously copied src (a pointer into user memory) into dst (a pointer on the supervisor mode call stack)

This comment has been minimized.

Copy link
@ceolin

ceolin Mar 29, 2019

Collaborator

this is not making sense to me though. Why do you need this check and copy then ? dst is an uninitialised variable:


Z_SYSCALL_HANDLER(adc_read, dev, user_sequence)
{
	struct adc_sequence sequence;
	struct adc_sequence_options options;

	Z_OOPS(Z_SYSCALL_DRIVER_ADC(dev, read));
	Z_OOPS(Z_SYSCALL_VERIFY_MSG(copy_sequence(&sequence, &options,
					(struct adc_sequence *)user_sequence),
				    "invalid ADC sequence"));
	if (sequence.options != NULL) {
		Z_OOPS(Z_SYSCALL_VERIFY_MSG(sequence.options->callback == NULL,
			    "ADC sequence callbacks forbidden from user mode"));
	}

	return z_impl_adc_read((struct device *)dev, &sequence);
}

This comment has been minimized.

Copy link
@andrewboie

andrewboie Mar 29, 2019

Author Collaborator

you need to copy user_sequence (lives somehere in user memory) into kernel memory (sequence) in order to prevent TOCTOU issues. notice I am not passing user_sequence to the syscall implementation.

This comment has been minimized.

Copy link
@ceolin

ceolin Mar 29, 2019

Collaborator

yeah, but this code is not copying user sequence, it is copying sequence that was allocate in this function stack and was not initialized. Well, it seems that I'm not understanding it :/

This comment has been minimized.

Copy link
@andrewboie

andrewboie Mar 29, 2019

Author Collaborator

you're not reading the code right, you have it backwards. user_sequence=src sequence=dst

This comment has been minimized.

Copy link
@ceolin

ceolin Mar 29, 2019

Collaborator

that is the problem, in copy_sequence you are not using user_sequence=src but sequece=dst

static bool copy_sequence(struct adc_sequence *dst,
			  struct adc_sequence_options *options,
			  struct adc_sequence *src)
{
...

	if (dst->options) { /* dst == sequence that is no uninitialised */
		if (z_user_from_copy(options, dst->options,
				sizeof(struct adc_sequence_options)) != 0) {
			printk("couldn't copy adc_options struct\n");
			return false;
		}
		dst->options = options;
	}
...
}

This comment has been minimized.

Copy link
@andrewboie

andrewboie Mar 29, 2019

Author Collaborator

Please post the whole function.

You will see that immediately before the check to dst->options, all of src got copied into dst. It is initialized.

dst->options points to a different memory buffer provided by the user, we copy it and then update dst->options.

This comment has been minimized.

Copy link
@ceolin

ceolin Mar 29, 2019

Collaborator

I missed that first copy, damn it. My bad, sorry for all noise

@ceolin
Copy link
Collaborator

left a comment

Looks ok, only one comment to check.

@agross-oss
Copy link
Collaborator

left a comment

aside from flavio's comment, this looks good.

@andrewboie andrewboie added this to the v1.14.0 milestone Mar 29, 2019

default 512 if COVERAGE_GCOV
default 384 if ARC
default 256
default 1024

This comment has been minimized.

Copy link
@ioannisg

ioannisg Mar 29, 2019

Member

I agree with the rationale, here, but, I see that the priv stack starts getting too big

This comment has been minimized.

Copy link
@andrewboie

andrewboie Mar 29, 2019

Author Collaborator

not sure what you mean by too big
The actual minimum value for this, that satisfies maximum depth for all kernel/driver syscalls is unknown at this time.

@ioannisg
Copy link
Member

left a comment

Looks fine to me (only did a high-level pass)

@ceolin

ceolin approved these changes Mar 29, 2019

@nashif nashif merged commit 02be448 into zephyrproject-rtos:master Mar 30, 2019

8 checks passed

Codeowners Checks passed
Details
Documentation Checks passed
Details
Gitlint Checks passed
Details
Identity/Emails Checks passed
Details
Kconfig Checks passed
Details
License Checks passed
Details
Shippable Run 38503 status is SUCCESS.
Details
checkpatch Checks passed
Details
@anangl

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

I apologize I haven't managed to add this comment before this PR got merged.
Just to ensure it wasn't overlooked. The ADC API allows to provide a callback also to the adc_read function, not only to adc_read_async. I thought this excluded adc_read from being a candidate for a system call. And that's why I haven't provided system calls for this API when reworking it (#7691 (comment)).

@andrewboie andrewboie deleted the andrewboie:adc-syscalls branch Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.