On 10/3/22 13:30, Ahmad Khalifa wrote:
On 03/10/2022 19:10, Guenter Roeck wrote:
On Sun, Oct 02, 2022 at 06:43:00PM +0100, Ahmad Khalifa wrote:
Add in the parameter to ignore ACPI resource conflicts so that modprobe
can use the force_id parameter when ACPI has the same address mapped.
force_id and ignore_resource_conflict are orthogonal / unrelated to each
other. Why create a dependency or correlation where none exists ?
The reason for introducing ignore_resource_conflict in the driver was that
some systems didn't like the system wide parameter (acpi_enforce_resources)
to ignore resource conflicts and failed to boot if it was set to lax
or disabled.
They're unrelated in their purpose, but adding ignore_resource_conflict creates an unusual situation that doesn't make it safe to use on its own. Because the driver registers two platform devices, the second one will start probing a random base address (0xFFF8).
They are unrelated, period. You only consider systems where a resource
conflict exists. Also, you could (try to) use acpi_enforce_resources=lax
instead.
Everything else is just a coincidence that applies to _your_ system.
It's not random though, because 0xFFFF & ~7 gets you there. But frankly, I don't know what normally lives there, so my initial opinion was that both changes should be a single commit to stop it87_find() from confirming that a device exists.
Just to be clear, currently, without the ignore_resource_conflict param, the driver just unregisters itself after the first of the 2 registers gives the ACPI conflict. So the issue of the second non-existant device, is not an issue.
Could separate the two patches completely, but ideally they're still together. What are your thoughts here?
That isn't the point here. The problem is not with the patches, it is with
the rationale for the patches.
The third parameter is the access permission. It should be 0 or maybe 0000.
Why "false" ?
My mistake, out of tree convention. Missed that the param right above it uses a 0 convention.
If there is such an out-of-tree convention, it is wrong. The
variable is not a boolean (it sets the runtime access permissions)
and must not be initialized with one. Case in point: Using "true"
as initializer would translate to permission 0001 (S_IXOTH,
execute permission for "other") which obviously would not make
any sense.
Guenter
