| Message ID | 20221002174259.14609-2-ahmad@khalifa.ws (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | it87: Add param to ignore ACPI resource conflicts | expand |
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. > > Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws> > --- > drivers/hwmon/it87.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index 0e543dbe0a6b..ce579f5aebcf 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -69,6 +69,10 @@ static unsigned short force_id; > module_param(force_id, ushort, 0); > MODULE_PARM_DESC(force_id, "Override the detected device ID"); > > +static bool ignore_resource_conflict; > +module_param(ignore_resource_conflict, bool, false); The third parameter is the access permission. It should be 0 or maybe 0000. Why "false" ? > +MODULE_PARM_DESC(ignore_resource_conflict, "Ignore ACPI resource conflict"); > + > static struct platform_device *it87_pdev[2]; > > #define REG_2E 0x2e /* The register to read/write */ > @@ -3261,8 +3265,10 @@ static int __init it87_device_add(int index, unsigned short address, > int err; > > err = acpi_check_resource_conflict(&res); > - if (err) > - return err; > + if (err){ Formatting: Space between ) and { required. Guenter > + if (!ignore_resource_conflict) > + return err; > + } > > pdev = platform_device_alloc(DRVNAME, address); > if (!pdev) > -- > 2.30.2 >
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). 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? > 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 (err){ > Formatting: Space between ) and { required. Will fix in v3.
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
On 03/10/2022 21:50, Guenter Roeck wrote: > 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: > 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. > [...] > That isn't the point here. The problem is not with the patches, it is with > the rationale for the patches. > Fair point. The force_id change is warranted on its own, even without the ignore_resource_conflict because: - Chipsets will reply with all 1s if no LPC peripheral is connected Useful for lots of systems with a single device - IT87 will reply with all 1s if you don't enter config space correctly Useful for those who override with a known id, but the device is quite different - The driver supports 2 sio devices, but the use of force_id is mostly a single-device use case. It doesn't support forcing two different devices, so it's safe to assume a non-existent device is not expected for those users. My systems run mostly tainted Kernels and I can even grab the temps with a shell script, so I'm not worried about my systems, but they're the only ones I can test with :) The kernel parameter is an option that works, but it's a bit more technical for general users to update their bootloader and change modprobe config at the same time. This whole patch is about making the it87 more usable with less compile/install work. I can prep v3 with both changes if that's still a good approach.
On 10/4/22 09:24, Ahmad Khalifa wrote: > On 03/10/2022 21:50, Guenter Roeck wrote: >> 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: >> 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. >> > [...] >> That isn't the point here. The problem is not with the patches, it is with >> the rationale for the patches. >> > Fair point. > > The force_id change is warranted on its own, even without the ignore_resource_conflict because: > - Chipsets will reply with all 1s if no LPC peripheral is connected > Useful for lots of systems with a single device > - IT87 will reply with all 1s if you don't enter config space correctly > Useful for those who override with a known id, but the device is > quite different > - The driver supports 2 sio devices, but the use of force_id is > mostly a single-device use case. It doesn't support forcing two > different devices, so it's safe to assume a non-existent device > is not expected for those users. > > My systems run mostly tainted Kernels and I can even grab the temps with a shell script, so I'm not worried about my systems, but they're the > only ones I can test with :) > > The kernel parameter is an option that works, but it's a bit more > technical for general users to update their bootloader and change > modprobe config at the same time. This whole patch is about making > the it87 more usable with less compile/install work. > That is not an argument. If acpi_enable_resources=lax would work, there would be no reason for the ignore_resource_conflict module parameter. > > I can prep v3 with both changes if that's still a good approach. > > Here is the original rationale for introducing ignore_resource_conflict. It appears that some BIOSes reserve ACPI resources without using them, and acpi_enable_resources=lax seems to result in failures to load certain drivers. Introduce a 'ignore_resource_conflict' module parameter as alternate means to ignore ACPI resource conflicts. The rationale for the force_id change was: If the reported chip ID is 0xffff, there is no chip, and forcing the chip type does not add value. That is it. No other rationale is needed, and the patch introducing ignore_resource_conflict needs to explain (like the above) why acpi_enable_resources=lax is insufficient. Guenter
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index 0e543dbe0a6b..ce579f5aebcf 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -69,6 +69,10 @@ static unsigned short force_id; module_param(force_id, ushort, 0); MODULE_PARM_DESC(force_id, "Override the detected device ID"); +static bool ignore_resource_conflict; +module_param(ignore_resource_conflict, bool, false); +MODULE_PARM_DESC(ignore_resource_conflict, "Ignore ACPI resource conflict"); + static struct platform_device *it87_pdev[2]; #define REG_2E 0x2e /* The register to read/write */ @@ -3261,8 +3265,10 @@ static int __init it87_device_add(int index, unsigned short address, int err; err = acpi_check_resource_conflict(&res); - if (err) - return err; + if (err){ + if (!ignore_resource_conflict) + return err; + } pdev = platform_device_alloc(DRVNAME, address); if (!pdev)
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. Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws> --- drivers/hwmon/it87.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)