| Message ID | 20221121025718.160667-1-frank@crawford.emu.id.au (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/1] hwmon: (it87) Automatic handling of ACPI resource failure | expand |
On Mon, Nov 21, 2022 at 01:57:18PM +1100, Frank Crawford wrote: > On some Gigabyte boards sensors are marked as ACPI regions but not > really handled by ACPI calls, as they return no data. > Most commonly this is seen on boards with multiple ITE chips. > In this case we just ignore the failure and continue on. > > This is effectively the same as the use of either > acpi_enforce_resources=lax (kernel) > or > ignore_resource_conflict=1 (it87) > but set programatically. > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > --- Sorry, I can not accept this patch. Ignoring resource conflicts may have unintentional side effects and _has_ to be explicitly requested by the user. Guenter > > Changes in this patch set: > > * Add a flag, set in the DMI table, for ignoring ACPI resource conflict. > > * Print an message if a conflict occurs, but otherwise ignore the issue. > > * Add known, tested boards to the DMI table. > > drivers/hwmon/it87.c | 62 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 61 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index 9997f76b1f4a..991f1ab5f82c 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -570,6 +570,7 @@ struct it87_data { > /* Board specific settings from DMI matching */ > struct it87_dmi_data { > u8 skip_pwm; /* pwm channels to skip for this board */ > + bool skip_acpi_res; /* ignore acpi failures on this board */ > }; > > /* Global for results from DMI matching, if needed */ > @@ -3264,7 +3265,9 @@ static int __init it87_device_add(int index, unsigned short address, > > err = acpi_check_resource_conflict(&res); > if (err) { > - if (!ignore_resource_conflict) > + if (dmi_data && dmi_data->skip_acpi_res) > + pr_info("Ignoring expected ACPI resource conflict\n"); > + else if (!ignore_resource_conflict) > return err; > } > > @@ -3322,6 +3325,21 @@ static struct it87_dmi_data nvidia_fn68pt = { > .skip_pwm = BIT(1), > }; > > +/* > + * On some Gigabyte boards sensors are marked as ACPI regions but not > + * really handled by ACPI calls, as they return no data. > + * Most commonly this is seen on boards with multiple ITE chips. > + * In this case we just ignore the failure and continue on. > + * This is effectively the same as the use of either > + * acpi_enforce_resources=lax (kernel) > + * or > + * ignore_resource_conflict=1 (it87) > + * but set programatically. > + */ > +static struct it87_dmi_data gigabyte_acpi_ignore = { > + .skip_acpi_res = true, > +}; > + > #define IT87_DMI_MATCH_VND(vendor, name, cb, data) \ > { \ > .callback = cb, \ > @@ -3332,7 +3350,49 @@ static struct it87_dmi_data nvidia_fn68pt = { > .driver_data = data, \ > } > > +#define IT87_DMI_MATCH_GBT(name, cb, data) \ > + IT87_DMI_MATCH_VND("Gigabyte Technology Co., Ltd.", name, cb, data) > + > static const struct dmi_system_id it87_dmi_table[] __initconst = { > + IT87_DMI_MATCH_GBT("TRX40 AORUS XTREME", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E + IT8792E/IT8795E */ > + IT87_DMI_MATCH_GBT("Z390 AORUS ULTRA-CF", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E + IT8792E/IT8795E */ > + IT87_DMI_MATCH_GBT("Z490 AORUS ELITE AC", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E */ > + IT87_DMI_MATCH_GBT("B550 AORUS PRO AC", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E + IT8792E/IT8795E */ > + IT87_DMI_MATCH_GBT("B560I AORUS PRO AX", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8689E */ > + IT87_DMI_MATCH_GBT("X570 AORUS ELITE WIFI", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E */ > + IT87_DMI_MATCH_GBT("X570 AORUS MASTER", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E + IT8792E/IT8795E */ > + IT87_DMI_MATCH_GBT("X570 AORUS PRO WIFI", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E + IT8792E/IT8795E */ > + IT87_DMI_MATCH_GBT("X570 I AORUS PRO WIFI", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8688E */ > + IT87_DMI_MATCH_GBT("X570S AERO G", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8689E + IT87952E */ > + IT87_DMI_MATCH_GBT("X670E AORUS MASTER", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8689E - Note there may also be a second chip */ > + IT87_DMI_MATCH_GBT("Z690 AORUS PRO DDR4", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8689E + IT87952E */ > + IT87_DMI_MATCH_GBT("Z690 AORUS PRO", it87_dmi_cb, > + &gigabyte_acpi_ignore), > + /* IT8689E + IT87952E */ > IT87_DMI_MATCH_VND("nVIDIA", "FN68PT", it87_dmi_cb, &nvidia_fn68pt), > { } >
On Tue, 2022-11-22 at 10:46 -0800, Guenter Roeck wrote: > On Mon, Nov 21, 2022 at 01:57:18PM +1100, Frank Crawford wrote: > > On some Gigabyte boards sensors are marked as ACPI regions but not > > really handled by ACPI calls, as they return no data. > > Most commonly this is seen on boards with multiple ITE chips. > > In this case we just ignore the failure and continue on. > > > > This is effectively the same as the use of either > > acpi_enforce_resources=lax (kernel) > > or > > ignore_resource_conflict=1 (it87) > > but set programatically. > > > > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> > > --- > > Sorry, I can not accept this patch. Ignoring resource conflicts > may have unintentional side effects and _has_ to be explicitly > requested by the user. I do have two comments on that decision, firstly, for the bulk of the boards listed I've dumped the ACPI tables and data and the conflicting address ranges do nothing with ACPI. It looks like Gigabyte planned to implement WMI access, but stopped after developing some code for single chipset boards, and just nulled out anything to do with boards with two chips. However, getting any information from Gigabyte about this is impossible, as you know. Secondly, unfortunately most users have no idea what the ACPI error means, and just follow random comments on the Internet, which currently is to set "acpi_enforce_resources=lax" which is even more dangerous. At least the recent addition of "ignore_resource_conflict=1" restricts the issue to a known area, and this would take it one step further in that we are just automating it for known "safe" boards. However, if you are not willing to accept it, I'll just drop it there. > > Guenter Regards Frank > >
On 11/22/22 15:28, Frank Crawford wrote: > > > On Tue, 2022-11-22 at 10:46 -0800, Guenter Roeck wrote: >> On Mon, Nov 21, 2022 at 01:57:18PM +1100, Frank Crawford wrote: >>> On some Gigabyte boards sensors are marked as ACPI regions but not >>> really handled by ACPI calls, as they return no data. >>> Most commonly this is seen on boards with multiple ITE chips. >>> In this case we just ignore the failure and continue on. >>> >>> This is effectively the same as the use of either >>> acpi_enforce_resources=lax (kernel) >>> or >>> ignore_resource_conflict=1 (it87) >>> but set programatically. >>> >>> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> >>> --- >> >> Sorry, I can not accept this patch. Ignoring resource conflicts >> may have unintentional side effects and _has_ to be explicitly >> requested by the user. > > I do have two comments on that decision, firstly, for the bulk of the > boards listed I've dumped the ACPI tables and data and the conflicting > address ranges do nothing with ACPI. It looks like Gigabyte planned to > implement WMI access, but stopped after developing some code for single > chipset boards, and just nulled out anything to do with boards with two > chips. However, getting any information from Gigabyte about this is > impossible, as you know. > > Secondly, unfortunately most users have no idea what the ACPI error > means, and just follow random comments on the Internet, which currently > is to set "acpi_enforce_resources=lax" which is even more dangerous. > At least the recent addition of "ignore_resource_conflict=1" restricts > the issue to a known area, and this would take it one step further in That is exactly why I had added to flag in the out-of-tree driver. > that we are just automating it for known "safe" boards. > > However, if you are not willing to accept it, I'll just drop it there. Sorry, I'd rather live with 100 users mad with me for not permitting the patch than permitting it and having to deal with one user who ended up with broken hardware or random reboots. This _has_ to be a conscious decision made by users. Also, ignoring ACPI resource conflicts always was and always will be risky. There is no such thing as a "known safe board". Who knows what Gigabyte is going to do in the next version of their BIOS. Guenter
On Tue, 2022-11-22 at 15:49 -0800, Guenter Roeck wrote: > On 11/22/22 15:28, Frank Crawford wrote: ... > That is exactly why I had added to flag in the out-of-tree driver. What do you mean here? Are you recommending that is should be take out of there as well? > > > that we are just automating it for known "safe" boards. > > > > However, if you are not willing to accept it, I'll just drop it > > there. > > Sorry, I'd rather live with 100 users mad with me for not permitting > the patch than permitting it and having to deal with one user who > ended up with broken hardware or random reboots. This _has_ to be a > conscious decision made by users. Yes, and I understand, and yes, that is your call. > > Also, ignoring ACPI resource conflicts always was and always will be > risky. > There is no such thing as a "known safe board". Who knows what > Gigabyte > is going to do in the next version of their BIOS. I would suggest that the possibility of Gigabyte doing that is almost zero. From what I can tell they are not doing anything further with the WMI method, which is where this come in, and even with some recent single chip boards it no longer seems to be supported. But that is a different matter. Unfortunately, they will just leave in the useless stub that does nothing but causes ACPI errors. > > Guenter Regards Frank
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index 9997f76b1f4a..991f1ab5f82c 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -570,6 +570,7 @@ struct it87_data { /* Board specific settings from DMI matching */ struct it87_dmi_data { u8 skip_pwm; /* pwm channels to skip for this board */ + bool skip_acpi_res; /* ignore acpi failures on this board */ }; /* Global for results from DMI matching, if needed */ @@ -3264,7 +3265,9 @@ static int __init it87_device_add(int index, unsigned short address, err = acpi_check_resource_conflict(&res); if (err) { - if (!ignore_resource_conflict) + if (dmi_data && dmi_data->skip_acpi_res) + pr_info("Ignoring expected ACPI resource conflict\n"); + else if (!ignore_resource_conflict) return err; } @@ -3322,6 +3325,21 @@ static struct it87_dmi_data nvidia_fn68pt = { .skip_pwm = BIT(1), }; +/* + * On some Gigabyte boards sensors are marked as ACPI regions but not + * really handled by ACPI calls, as they return no data. + * Most commonly this is seen on boards with multiple ITE chips. + * In this case we just ignore the failure and continue on. + * This is effectively the same as the use of either + * acpi_enforce_resources=lax (kernel) + * or + * ignore_resource_conflict=1 (it87) + * but set programatically. + */ +static struct it87_dmi_data gigabyte_acpi_ignore = { + .skip_acpi_res = true, +}; + #define IT87_DMI_MATCH_VND(vendor, name, cb, data) \ { \ .callback = cb, \ @@ -3332,7 +3350,49 @@ static struct it87_dmi_data nvidia_fn68pt = { .driver_data = data, \ } +#define IT87_DMI_MATCH_GBT(name, cb, data) \ + IT87_DMI_MATCH_VND("Gigabyte Technology Co., Ltd.", name, cb, data) + static const struct dmi_system_id it87_dmi_table[] __initconst = { + IT87_DMI_MATCH_GBT("TRX40 AORUS XTREME", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E + IT8792E/IT8795E */ + IT87_DMI_MATCH_GBT("Z390 AORUS ULTRA-CF", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E + IT8792E/IT8795E */ + IT87_DMI_MATCH_GBT("Z490 AORUS ELITE AC", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E */ + IT87_DMI_MATCH_GBT("B550 AORUS PRO AC", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E + IT8792E/IT8795E */ + IT87_DMI_MATCH_GBT("B560I AORUS PRO AX", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8689E */ + IT87_DMI_MATCH_GBT("X570 AORUS ELITE WIFI", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E */ + IT87_DMI_MATCH_GBT("X570 AORUS MASTER", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E + IT8792E/IT8795E */ + IT87_DMI_MATCH_GBT("X570 AORUS PRO WIFI", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E + IT8792E/IT8795E */ + IT87_DMI_MATCH_GBT("X570 I AORUS PRO WIFI", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8688E */ + IT87_DMI_MATCH_GBT("X570S AERO G", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8689E + IT87952E */ + IT87_DMI_MATCH_GBT("X670E AORUS MASTER", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8689E - Note there may also be a second chip */ + IT87_DMI_MATCH_GBT("Z690 AORUS PRO DDR4", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8689E + IT87952E */ + IT87_DMI_MATCH_GBT("Z690 AORUS PRO", it87_dmi_cb, + &gigabyte_acpi_ignore), + /* IT8689E + IT87952E */ IT87_DMI_MATCH_VND("nVIDIA", "FN68PT", it87_dmi_cb, &nvidia_fn68pt), { }
On some Gigabyte boards sensors are marked as ACPI regions but not really handled by ACPI calls, as they return no data. Most commonly this is seen on boards with multiple ITE chips. In this case we just ignore the failure and continue on. This is effectively the same as the use of either acpi_enforce_resources=lax (kernel) or ignore_resource_conflict=1 (it87) but set programatically. Signed-off-by: Frank Crawford <frank@crawford.emu.id.au> --- Changes in this patch set: * Add a flag, set in the DMI table, for ignoring ACPI resource conflict. * Print an message if a conflict occurs, but otherwise ignore the issue. * Add known, tested boards to the DMI table. drivers/hwmon/it87.c | 62 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-)