[PATCH v2 0/2] it87: Add param to ignore ACPI resource conflicts

[PATCH v2 0/2] it87: Add param to ignore ACPI resource conflicts

[Ahmad Khalifa]

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.

This requires also checking the device id before assigning force_id to
avoid registering two devices. This may not be a visible issue as ACPI
conflict unregisters the whole driver, but with ignore_resource_conflict
added, both SIO addresses will register a device.

This is useful for those who don't compile their own modules from github
code and just want to configure modprobe with a /force_id/ value that
gives them coverage of the first 3-4 fans/temp/in values.

Code is inspired by the github it87 module floating out there, but didn't
merge in support for additional devices as there were objections to
that approach in past threads [1].

Tested with it8688 on Gigabyte board with id as it8628 and compared
against the out of tree module running the it8688 values (which in turn
is blindly based off of the it8686 values) and the results are the same
for the enabled sensors (i.e. not all 6 fans/temps/in are valid)


v2: updates with comments: [2]
* Add missing patch description, not just cover letter
* split change for the force_id check into separate commit

[1]: https://lore.kernel.org/linux-hwmon/6c8b5fbd514df708af84630544eca6ee12766bbd.camel@crawford.emu.id.au/
[2]: https://lore.kernel.org/linux-hwmon/20221002151148.GA2896730@roeck-us.net/T/#u

Ahmad Khalifa (2):
  it87: Add param to ignore ACPI resource conflicts
  it87: check device is valid before using force_id

 drivers/hwmon/it87.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)


base-commit: c3e0e1e23c70455916ff3472072437b3605c6cfe
-- 
2.30.2

[PATCH v2 1/2] it87: Add param to ignore ACPI resource conflicts

[Ahmad Khalifa]

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(-)

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)
-- 
2.30.2

Re: [PATCH v2 1/2] it87: Add param to ignore ACPI resource conflicts

[Guenter Roeck]

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
> 

Re: [PATCH v2 1/2] it87: Add param to ignore ACPI resource conflicts

[Ahmad Khalifa]

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.


-- 
Regards,
Ahmad

Re: [PATCH v2 1/2] it87: Add param to ignore ACPI resource conflicts

[Guenter Roeck]

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

Re: [PATCH v2 1/2] it87: Add param to ignore ACPI resource conflicts

[Ahmad Khalifa]

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.


-- 
Regards,
Ahmad

Re: [PATCH v2 1/2] it87: Add param to ignore ACPI resource conflicts

[Guenter Roeck]

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

[PATCH v2 2/2] it87: check device is valid before using force_id

[Ahmad Khalifa]

Check if there is a valid device before using force_id parameter value
in order to avoid registering two devices.

Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws>
---
 drivers/hwmon/it87.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index ce579f5aebcf..6d71f6c481c8 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -2401,7 +2401,13 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 		return err;
 
 	err = -ENODEV;
-	chip_type = force_id ? force_id : superio_inw(sioaddr, DEVID);
+	chip_type = superio_inw(sioaddr, DEVID);
+	/* check first so force_id doesn't register a second empty device */
+	if (chip_type == 0xffff)
+		goto exit;
+
+	if (force_id)
+		chip_type = force_id;
 
 	switch (chip_type) {
 	case IT8705F_DEVID:
-- 
2.30.2

Re: [PATCH v2 2/2] it87: check device is valid before using force_id

[Guenter Roeck]

On Sun, Oct 02, 2022 at 06:43:02PM +0100, Ahmad Khalifa wrote:
> Check if there is a valid device before using force_id parameter value
> in order to avoid registering two devices.
> 

For the subject, please use "hwmon: (it87) ..."
in the future.

> Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws>
> ---
>  drivers/hwmon/it87.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index ce579f5aebcf..6d71f6c481c8 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -2401,7 +2401,13 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>  		return err;
>  
>  	err = -ENODEV;
> -	chip_type = force_id ? force_id : superio_inw(sioaddr, DEVID);
> +	chip_type = superio_inw(sioaddr, DEVID);
> +	/* check first so force_id doesn't register a second empty device */

The reasoning is wrong: the ID is checked to avoid registering a
non-existing or a completely incompatible device. It doesn't matter
if the skipped device is the first or the second device.

> +	if (chip_type == 0xffff)
> +		goto exit;
> +
> +	if (force_id)
> +		chip_type = force_id;
>  
>  	switch (chip_type) {
>  	case IT8705F_DEVID:
> -- 
> 2.30.2
> 

Re: [PATCH v2 2/2] it87: check device is valid before using force_id

[Ahmad Khalifa]

On 03/10/2022 19:01, Guenter Roeck wrote:
> On Sun, Oct 02, 2022 at 06:43:02PM +0100, Ahmad Khalifa wrote:
>> Check if there is a valid device before using force_id parameter value
>> in order to avoid registering two devices.
> For the subject, please use "hwmon: (it87) ..."
> in the future.

Will do with v3 of the patch.

>> +	/* check first so force_id doesn't register a second empty device */
> 
> The reasoning is wrong: the ID is checked to avoid registering a
> non-existing or a completely incompatible device. It doesn't matter
> if the skipped device is the first or the second device.

Non-existing is more accurate than empty, I can change to that in v3
The physical device doesn't exist, but the platform device is registered 
with no attributes.


-- 
Regards,
Ahmad