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

[NTOS:PNP] IopEnumerateDetectedDevices(): General refactoring #5276

Merged
merged 1 commit into from May 27, 2023

Conversation

HBelusca
Copy link
Contributor

**NOTE: Made on behalf of @binarymaster **

Purpose

  • Deduplicate a while-loop by adding one more recursive call.
  • Add IopMapDetectedDeviceId() helper function with a structure in order to reduce hardcoded constants and checks.

JIRA issue: CORE-18962

@HBelusca HBelusca added enhancement For PRs with an enhancement/new feature. review pending For PRs undergoing review. kernel&hal Code changes to the ntoskrnl and HAL labels May 10, 2023
@Extravert-ir
Copy link
Member

This stuff should be moved to HAL, the kernel must not be responsible for mapping devices on a particular type of system.
Also, the way it is done is very hacky - one must not write resources directly to HKLM/System/CCS/Enum, devices should be exposed as PDOs on HAL bus.

That would be a refactoring!

@HBelusca
Copy link
Contributor Author

the kernel must not be responsible for mapping devices on a particular type of system

Is this because the kernel should stay as much as possible system-agnostic, while the HAL indeed can depend on the system it supports?

@HBelusca
Copy link
Contributor Author

HBelusca commented May 12, 2023

Reply from @binarymaster :

This stuff should be moved to HAL, the kernel must not be responsible for mapping devices on a particular type of system.

I can agree with this, specifically for the controllers that may have different PnP identifiers on different types of systems (e.g. PC/AT vs. PC-98), however some known devices would always have the same identifiers (most keyboards and mices). Redesigning it the way you described is a good idea, but it's out of scope of this PR.

Also, the way it is done is very hacky - one must not write resources directly to HKLM/System/CCS/Enum, devices should be exposed as PDOs on HAL bus.

This is correct, however the goal of this PR/patch is to simplify the function, reducing amount of code, without breaking current logic in any way (even if it's not perfect). Changing the things in a more correct way that you described would rather be a "bugfix"/"enhancement" than refactoring. 🙂

@disean
Copy link
Contributor

disean commented May 14, 2023

This stuff should be moved to HAL

Why not leave this device mapping logic in the kernel for compatibility?

@Extravert-ir
Copy link
Member

Is this because the kernel should stay as much as possible system-agnostic, while the HAL indeed can depend on the system it supports?

There is a general code in the kernel for managing device objects and its resources, and it is PnP device management code. This "firmware mapper" thing creates a parallel code path which does the same thing. I want to remove it to avoid code duplication and bugs.

Why not leave this device mapping logic in the kernel for compatibility?

For compatibility with what?

@disean
Copy link
Contributor

disean commented May 14, 2023

For compatibility with what?

Example: replacing the hal.dll with one from Windows

@Extravert-ir
Copy link
Member

replacing the hal.dll with one from Windows

Such scenario will not be supported by ReactOS. Supporting HALs from Windows (2003) is a quite an amount of work, and puts too many restrictions on internal design of things

CORE-18962

- Deduplicate a while-loop by adding one more recursive call.
- Add IopMapDetectedDeviceId() helper function with a structure
  in order to reduce hardcoded constants and checks.
@HBelusca HBelusca merged commit 85ca8af into reactos:master May 27, 2023
@HBelusca HBelusca deleted the ntos_pnp_refactor_stasm branch May 27, 2023 10:24
@binarymaster binarymaster removed the review pending For PRs undergoing review. label Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For PRs with an enhancement/new feature. kernel&hal Code changes to the ntoskrnl and HAL
Projects
None yet
4 participants