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

adc_read_async callback parameters are dereferenced pointers, making use of CONTAINER_OF impossible #30362

Closed
JvanDooren opened this issue Dec 1, 2020 · 1 comment · Fixed by #30533
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@JvanDooren
Copy link

JvanDooren commented Dec 1, 2020

Describe the bug
When the adc_read_async method is called (zephyr/include/drivers/adc.h) with a sequence pointer placed on the heap, and sequence.options pointer placed on the heap, the same pointers are not returned in the options.callback function pointer.
Instead, in zephyr/drivers/adc/adc_context.h, the given pointers are dereferenced in the method 'adc_context_start_read'.

Essentially, the 'struct adc_sequence' contents are cloned and a pointer to the cloned contents is passed as callback argument.
As this is passed, there is no way of using CONTAINER_OF() in the options.callback function, as the pointer value is different as to what was passed in the adc_read_async.

When adc_read_async is called, passing pointers that are on the stack, the dereferencing makes sense, but in order to do something in the options.callback function, they must be on the heap.

To Reproduce
Steps to reproduce the behavior:

#pragma once

#include <drivers/adc.h>
#include <cassert>

namespace Test::IO {

    // Circumvent leaky abstractions from Zephyr
    #if defined(ADC)
        #undef ADC
    #endif

    class ADC
    {
    private:
        const struct device* _dev;
        uint8_t _channel;

        struct adc_channel_cfg _config;
        uint8_t _resolution;

        struct IsrContext {
            struct k_work work;
            ADC* self;

            struct adc_sequence_options options;
            struct adc_sequence sequence;
            int16_t buffer;

            int16_t sample;
        } _isrContext;

    public:
        ADC(const struct device* dev, const struct adc_channel_cfg& config, uint8_t resolution)
            : _dev(dev)
            , _config(config)
            , _resolution(resolution)
        {
            int result = 0;
            assert(dev != nullptr);

            // Configure ADC channel
            result = adc_channel_setup(_dev, &_config);
            assert(result == 0);

            // Configure soft IRQ
            k_work_init(&_isrContext.work, _soft_isr);
            _isrContext.self = this;
        }

        /**
         * Starts the capture of a continuous sequence of samples.
         * @param interval_us Interval in microseconds between each consecutive sample.
         */
        virtual void readAsync(uint32_t interval_us = 0) override {
            _isrContext.options = {
                .interval_us = interval_us,
                .callback = _hard_isr,
            };

            _isrContext.sequence = {
                .options = &_options,
                .channels = BIT(_config.channel_id),
                .buffer = &_isrContext.buffer,
                .buffer_size = sizeof(_isrContext.buffer),
                .resolution = _resolution,
            };

            int res = adc_read_async(_dev, &_isrContext.sequence, nullptr);
            assert(res == 0);
        }

        /**
         * Cancel async read and stops the capture, does nothing in case there is
         * no capture in progress.
         */
        virtual void cancelRead() override {
            // TODO: To be implemented
        }

    private:
        static void _soft_isr(struct k_work *work) {

        }

        static enum adc_action _hard_isr(const struct device *,
                const struct adc_sequence *sequence,
                uint16_t sampling_index __unused)
        {
            IsrContext* context = CONTAINER_OF(sequence, IsrContext, sequence);

            // Store a copy of the conversion buffer
            context->sample = context->buffer;

            // Forward IRQ to kernel worker thread.
            k_work_submit(&context->work);

            // Continue sampling
            return ADC_ACTION_REPEAT;
        }
    };

}  // namespace Test::IO
  1. Create a main that constructs an ADC instance and calls readAsync()
  2. prj.conf contains CONFIG_ADC=y, CONFIG_ADC_STM32=y, CONFIG_ADC_ASYNC=y
  3. cmake -Bbuild -GNinja -DBOARD=nucleo_h743zi .
  4. make -C build
  5. Witness crash on the _hard_isr due to CONTAINER_OF pointing to a wrong memory location

Expected behavior
adc_read_async sequence.option.callback method uses same passed sequence pointer

Impact
We have created our own k_timer that calls an adc_read (synchronous call) decoupled by a system worker queue item.

Environment (please complete the following information):

  • OS: Linux Ubuntu 18.04
  • Commit SHA or Version used: 1ea7bf0
@JvanDooren JvanDooren added the bug The issue is a bug, or the PR is fixing a bug label Dec 1, 2020
@nashif nashif added the priority: medium Medium impact/importance bug label Dec 1, 2020
@JvanDooren
Copy link
Author

Thanks guys, I can confirm this fixes the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants