Skip to content

Conversation

seijikun
Copy link
Contributor

@seijikun seijikun commented May 7, 2025

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@seijikun
Copy link
Contributor Author

seijikun commented May 7, 2025

No idea what the CI is complaining about, tbh.
Is it because the variants are called Uint8, Uint16, etc.?

I'm still quite unsure about how we should handle this: *mut Self here, because a normal read may (depending on the device) change the PCI device's internal state just as much as a write.
Should we just make both take a mutable pointer?

@seijikun seijikun force-pushed the mr-pciroot branch 2 times, most recently from 7357644 to 4adcc5c Compare May 7, 2025 16:29
@seijikun seijikun marked this pull request as draft May 7, 2025 16:32
@nicholasbishop
Copy link
Member

No idea what the CI is complaining about, tbh.

Currently just a couple minor clippy things, add must_use and const: https://github.com/rust-osdev/uefi-rs/actions/runs/14888659415/job/41814946880?pr=1658

Should we just make both take a mutable pointer?

Yep, when in doubt let's go with *mut in uefi-raw.

@seijikun seijikun force-pushed the mr-pciroot branch 2 times, most recently from 397aafc to 4dd2a16 Compare May 8, 2025 08:20
@seijikun
Copy link
Contributor Author

seijikun commented May 8, 2025

Aight, when in doubt, *mut it out.
I made every method of which I think might change the device's state/memory in any way as *mut.

This is the error I don't really understand:
image

@seijikun seijikun marked this pull request as ready for review May 8, 2025 11:57
@phip1611
Copy link
Member

phip1611 commented May 8, 2025

The error reporting of the check-raw step is rather poor, I agree. You can run cargo xtask check-raw locally (sources are in <repo>/xtask/src) and debug into to look up what's wrong; feel free to add an improvement to the error reporting as well

@nicholasbishop
Copy link
Member

nicholasbishop commented May 8, 2025

Ah sorry I missed that error. It's because you're using a Rust enum, which is not quite compatible with a C enum (because it's undefined behavior to create a Rust enum from an invalid value).

This can be fixed by using the newtype_enum macro.

(I put up #1660 to improve this error message.)

@seijikun seijikun force-pushed the mr-pciroot branch 3 times, most recently from 6fd55f5 to 45b4059 Compare May 8, 2025 19:24
@seijikun
Copy link
Contributor Author

seijikun commented May 8, 2025

This can be fixed by using the newtype_enum macro.

Oof, that's embarrassing, the last merge request was only a few weeks ago and I've already forgotten about it.

I needed some iterations for the addressing stuff (PciIoAddress), but now I'm quite happy with it.

@seijikun seijikun force-pushed the mr-pciroot branch 5 times, most recently from 497dbf1 to 6f20e03 Compare May 12, 2025 19:14
@seijikun seijikun requested a review from nicholasbishop May 15, 2025 08:21
@seijikun seijikun force-pushed the mr-pciroot branch 3 times, most recently from c59b71f to 568725f Compare May 19, 2025 09:01
Copy link
Member

@nicholasbishop nicholasbishop left a comment

Choose a reason for hiding this comment

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

Lgtm, but needs a git conflict resolved

@nicholasbishop nicholasbishop enabled auto-merge May 19, 2025 15:43
@nicholasbishop nicholasbishop added this pull request to the merge queue May 19, 2025
Merged via the queue into rust-osdev:main with commit 6d8185f May 19, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants