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

Fix spinlock headers #477

Conversation

marcrittinghaus
Copy link
Member

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.pl on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): [arm64 and generic]
  • Platform(s): [N/A]
  • Application(s): [N/A]

Additional configuration

Description of changes

This PR adds some missing includes and checks to the spinlock headers as well as fixing spelling mistakes.
Thanks @Sairajkodilkar for pointing out the missing essentials.h! 😃

The spinlock main header uses the `CONFIG_HAVE_SMP` and should therefore
include `uk/config.h`.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
Commit b85c0df ("plat/kvm/arm: Add alignament attribute to spinlock struct")
introduces the `__align()` macro in the definition of the
spinlock without including `essentials.h`.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
To ensure that required macros are defined by the spinlock
implementation, we add a preprocessor check. This commit
further fixes the [in,out] parameters to comply with the doxygen
specification.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
@marcrittinghaus marcrittinghaus requested review from a team as code owners June 17, 2022 07:02
@marcrittinghaus marcrittinghaus added lang/c Issues or PRs to do with C/C++ arch/arm64 bug/fix This PR fixes a bug area/include Part of include/uk topic/concurrency Topics to do with concurrency, locking labels Jun 17, 2022
@razvand razvand removed request for a team June 23, 2022 08:52
@razvand razvand added this to the v0.10.0 (Phoebe) milestone Jun 23, 2022
@razvanvirtan razvanvirtan reopened this Jul 5, 2022
Copy link
Contributor

@razvanvirtan razvanvirtan left a comment

Choose a reason for hiding this comment

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

All good on my side.

Reviewed-by: Răzvan Vîrtan virtanrazvan@gmail.com

@unikraft-bot
Copy link
Member

⚠️ Checkpatch passed with warnings

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered warnings:

SHA commit checkpatch
c4022a2 include/arch: Add config.h
8970b67 include/arm64: Add essentials header ⚠️
387a75f include/arch: Add macro check and fix spelling

Truncated logs starting from first warning 8970b67:

WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#6: 
Commit b85c0dfa163a ("plat/kvm/arm: Add alignament attribute to spinlock struct")

total: 0 errors, 1 warnings, 7 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0002-include-arm64-Add-essentials-header.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

@marcrittinghaus
Copy link
Member Author

@michpappas Also here. Could you have a quick look and approve if everything is fine from your side?

@michpappas
Copy link
Member

Approved-by: Michalis Pappas mpappas@fastmail.fm

unikraft-bot pushed a commit that referenced this pull request Aug 15, 2022
Commit b85c0df ("plat/kvm/arm: Add alignament attribute to spinlock struct")
introduces the `__align()` macro in the definition of the
spinlock without including `essentials.h`.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
Reviewed-by: Răzvan Vîrtan <virtanrazvan@gmail.com>
Approved-by: Michalis Pappas <mpappas@fastmail.fm>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #477
unikraft-bot pushed a commit that referenced this pull request Aug 15, 2022
To ensure that required macros are defined by the spinlock
implementation, we add a preprocessor check. This commit
further fixes the [in,out] parameters to comply with the doxygen
specification.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
Reviewed-by: Răzvan Vîrtan <virtanrazvan@gmail.com>
Approved-by: Michalis Pappas <mpappas@fastmail.fm>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #477
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 15, 2022
@marcrittinghaus marcrittinghaus deleted the mritting/pr_fix_spinlock_headers branch May 26, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch/arm64 area/include Part of include/uk bug/fix This PR fixes a bug ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ topic/concurrency Topics to do with concurrency, locking
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants