Skip to content

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 6, 2021

Contribution description

Make sure in _usbdev_new_ep() that usbdev_ep_t::buf is always aligned to 4 bytes. With this in mind, add intermediate casts to uintptr_t before casting usbdev_ep_t::buf to uint32_t * to silence -Wcast-align, as we now manually enforced correct alignment.

Testing procedure

Everything should still work as before, but unaligned memory access should now prevented. (That is, no crash on architectures that do not support them.)

Issues/PRs references

Split out of #14955

Make sure in `_usbdev_new_ep()` that `usbdev_ep_t::buf` is always aligned to 4
bytes. With this in mind, add intermediate casts to `uintptr_t` before casting
`usbdev_ep_t::buf` to `uint32_t *` to silence `-Wcast-align`, as we now manually
enforced correct alignment.
@maribu maribu added Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Nov 6, 2021
@github-actions github-actions bot added the Platform: ARM Platform: This PR/issue effects ARM-based platforms label Nov 6, 2021
@benpicco benpicco requested a review from bergzand November 6, 2021 22:22
@bergzand bergzand 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 Nov 7, 2021
@@ -513,6 +523,7 @@ static usbdev_ep_t *_usbdev_new_ep(usbdev_t *dev, usb_ep_type_t type,
ep->dev = dev;
ep->len = buf_len;
usbdev->occupied += buf_len;
usbdev->occupied = _round_up_to_multiple_of_four(usbdev->occupied);
Copy link
Member Author

Choose a reason for hiding this comment

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

@bergzand an alternative here would be adding

    assert((buf_len & 3) == 0);

Would that make more sense?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the current solution

Copy link
Member Author

@maribu maribu left a comment

Choose a reason for hiding this comment

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

@bergzand bergzand self-assigned this Nov 16, 2021
@bergzand bergzand removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 16, 2021
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Tested, ACK!

@bergzand bergzand merged commit 76215ad into RIOT-OS:master Nov 16, 2021
@bergzand bergzand mentioned this pull request Nov 16, 2021
6 tasks
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
@maribu maribu deleted the cpu/stm32/periph_usb branch January 23, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms 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.

3 participants