| Message ID | 20170821112657.hrtjoeagxhc67rrr@pd.tnic (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Aug 21, 2017 at 1:27 PM, Borislav Petkov <bp@alien8.de> wrote: > On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote: >> ACPI OEM ID / OEM Table ID / Revision can be used to identify >> a platform based on ACPI firmware info. acpi_blacklisted(), >> intel_pstate_platform_pwr_mgmt_exists(), and some other funcs, >> have been using similar check to detect a list of platforms >> that require special handlings. >> >> Move the platform check in acpi_blacklisted() to a new common >> utility function, acpi_match_platform_list(), so that other >> drivers do not have to implement their own version. >> >> There is no change in functionality. >> >> Signed-off-by: Toshi Kani <toshi.kani@hpe.com> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Cc: Borislav Petkov <bp@alien8.de> >> --- >> drivers/acpi/blacklist.c | 83 ++++++++-------------------------------------- >> drivers/acpi/utils.c | 40 ++++++++++++++++++++++ >> include/linux/acpi.h | 19 +++++++++++ >> 3 files changed, 73 insertions(+), 69 deletions(-) > > ... > >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index b9d956c..998aaf5 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str) >> return 1; >> } >> __setup("acpi_backlight=", acpi_backlight); >> + >> +/** >> + * acpi_match_platform_list - Check if the system matches with a given list >> + * @plat: pointer to acpi_platform_list table terminated by a NULL entry >> + * >> + * Return the matched index if the system is found in the platform list. >> + * Otherwise, return a negative error code. >> + */ >> +int acpi_match_platform_list(const struct acpi_platform_list *plat) >> +{ >> + struct acpi_table_header hdr; >> + int idx = 0; >> + >> + if (acpi_disabled) >> + return -ENODEV; > > Btw, Rafael, should we do something like this: > > diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c > index 010b1c43df92..881b0d5b2838 100644 > --- a/drivers/acpi/acpica/tbxface.c > +++ b/drivers/acpi/acpica/tbxface.c > @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature, > u32 j; > struct acpi_table_header *header; > > + if (acpi_disabled) > + return (AE_ERROR); > + > /* Parameter validation */ > > if (!signature || !out_table_header) { > --- > > in order to avoid that sprinkling of if (acpi_disabled) everywhere? Yes, let's catch this as early as possible. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-08-21 at 13:27 +0200, Borislav Petkov wrote: > On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote: > > ACPI OEM ID / OEM Table ID / Revision can be used to identify > > a platform based on ACPI firmware info. acpi_blacklisted(), > > intel_pstate_platform_pwr_mgmt_exists(), and some other funcs, > > have been using similar check to detect a list of platforms > > that require special handlings. > > > > Move the platform check in acpi_blacklisted() to a new common > > utility function, acpi_match_platform_list(), so that other > > drivers do not have to implement their own version. > > > > There is no change in functionality. : > > + > > + for (; plat->oem_id[0]; plat++, idx++) { > > + if (ACPI_FAILURE(acpi_get_table_header(plat- > > >table, 0, &hdr))) > > + continue; > > + > > + if (strncmp(plat->oem_id, hdr.oem_id, > > ACPI_OEM_ID_SIZE)) > > + continue; > > + > > + if (strncmp(plat->oem_table_id, hdr.oem_table_id, > > + ACPI_OEM_TABLE_ID_SIZE)) > > Let that stick out. Putting to a single line leads to "line over 80 characters" warning from checkpatch.pl. Would you still advice to do that? > > + continue; > > + > > + if ((plat->pred == all_versions) || > > + (plat->pred == less_than_or_equal > > + && hdr.oem_revision <= plat->oem_revision) > > || > > + (plat->pred == greater_than_or_equal > > + && hdr.oem_revision >= plat->oem_revision) > > || > > + (plat->pred == equal > > + && hdr.oem_revision == plat- > > >oem_revision)) > > + return idx; > > Make that more readable: > > if ((plat->pred == all_versions) || > (plat->pred == less_than_or_equal && > hdr.oem_revision <= plat->oem_revision) || > (plat->pred == greater_than_or_equal && > hdr.oem_revision >= plat->oem_revision) || > (plat->pred == equal && > hdr.oem_revision == plat->oem_revision)) > return idx; Same here. These lead to checkpatch warnings. > > + } > > + > > + return -ENODEV; > > +} > > +EXPORT_SYMBOL(acpi_match_platform_list); > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index 27b4b66..a9b6dc2 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -556,6 +556,25 @@ extern acpi_status > > acpi_pci_osc_control_set(acpi_handle handle, > > #define ACPI_OST_SC_DRIVER_LOAD_FAILURE 0x81 > > #define ACPI_OST_SC_INSERT_NOT_SUPPORTED 0x82 > > > > +enum acpi_predicate { > > + all_versions, > > + less_than_or_equal, > > + equal, > > + greater_than_or_equal, > > +}; > > + > > +/* Table must be terminted by a NULL entry */ > > +struct acpi_platform_list { > > + char oem_id[ACPI_OEM_ID_SIZE]; > > + 1 > > > + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; > > + 1 strncmp() is fine without these, but it'd be prudent in case someone decides to print these strings with printk(). Will do. > > + u32 oem_revision; > > + char *table; > > + enum acpi_predicate pred; > > + char *reason; > > + u32 data; > > Ok, turning that into data from is_critical_error is a step in the > right direction. Let's make it even better: > > u32 flags; > > and do > > #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) > > so that future elements add new bits instead of wasting a whole u32 > as a boolean. 'data' here is private to the caller. So, I do not think we need to define the bits. Shall I change the name to 'driver_data' to make it more explicit? Thanks, -Toshi
On Mon, Aug 21, 2017 at 04:41:38PM +0000, Kani, Toshimitsu wrote: > Putting to a single line leads to "line over 80 characters" warning > from checkpatch.pl. Would you still advice to do that? Yes, the 80 cols rule is not a hard one. Rather, it should be overridden by human good judgement, like making the code more readable. > strncmp() is fine without these, but it'd be prudent in case someone > decides to print these strings with printk(). Will do. Someone does already use them in printk(): + pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n", + acpi_blacklist[i].oem_id, + acpi_blacklist[i].oem_table_id, + acpi_blacklist[i].oem_revision); > 'data' here is private to the caller. So, I do not think we need to > define the bits. Shall I change the name to 'driver_data' to make it > more explicit? You changed it to 'data'. It was a u32-used-as-boolean is_critical_error before. So you can just as well make it into flags and people can extend those flags if needed. A flag bit should be enough in most cases anyway. If they really need driver_data, then they can add a void * member.
On Mon, 2017-08-21 at 19:04 +0200, Borislav Petkov wrote: > On Mon, Aug 21, 2017 at 04:41:38PM +0000, Kani, Toshimitsu wrote: > > Putting to a single line leads to "line over 80 characters" warning > > from checkpatch.pl. Would you still advice to do that? > > Yes, the 80 cols rule is not a hard one. Rather, it should be > overridden by human good judgement, like making the code more > readable. I see. I will make these changes. (It's really personal preference, but long lines of if-conditions are not so easy to read to my eyes, though.) > > strncmp() is fine without these, but it'd be prudent in case > > someone decides to print these strings with printk(). Will do. > > Someone does already use them in printk(): > > + pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" > Revision 0x%x has a known ACPI BIOS problem.\n", > + acpi_blacklist[i].oem_id, > + acpi_blacklist[i].oem_table_id, > + acpi_blacklist[i].oem_revision); Oh, you are right about that! > > 'data' here is private to the caller. So, I do not think we need > > to define the bits. Shall I change the name to 'driver_data' to > > make it more explicit? > > You changed it to 'data'. It was a u32-used-as-boolean > is_critical_error before. > > So you can just as well make it into flags and people can extend > those flags if needed. A flag bit should be enough in most cases > anyway. If they really need driver_data, then they can add a void * > member. Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this field for PSS and PCC, which are enum values. I think we should allow drivers to set any values here. I agree that it may need to be void * if we also allow drivers to set a pointer here. Thanks, -Toshi
On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote: > > > 'data' here is private to the caller. So, I do not think we need > > > to define the bits. Shall I change the name to 'driver_data' to > > > make it more explicit? > > > > You changed it to 'data'. It was a u32-used-as-boolean > > is_critical_error before. > > > > So you can just as well make it into flags and people can extend > > those flags if needed. A flag bit should be enough in most cases > > anyway. If they really need driver_data, then they can add a void * > > member. > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this > field for PSS and PCC, which are enum values. I think we should allow > drivers to set any values here. I agree that it may need to be void * > if we also allow drivers to set a pointer here. Let's see what Rafael prefers.
On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> wrote: > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote: >> > > 'data' here is private to the caller. So, I do not think we need >> > > to define the bits. Shall I change the name to 'driver_data' to >> > > make it more explicit? >> > >> > You changed it to 'data'. It was a u32-used-as-boolean >> > is_critical_error before. >> > >> > So you can just as well make it into flags and people can extend >> > those flags if needed. A flag bit should be enough in most cases >> > anyway. If they really need driver_data, then they can add a void * >> > member. >> >> Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses this >> field for PSS and PCC, which are enum values. I think we should allow >> drivers to set any values here. I agree that it may need to be void * >> if we also allow drivers to set a pointer here. > > Let's see what Rafael prefers. I would retain the is_critical_error field and use that for printing the recoverable / non-recoverable message. This is kind of orthogonal to whether or not any extra data is needed and that can be an additional field. In that case unsigned long should be sufficient to accommodate a pointer if need be. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote: > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> > wrote: > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote: > > > > > 'data' here is private to the caller. So, I do not think we > > > > > need to define the bits. Shall I change the name to > > > > > 'driver_data' to make it more explicit? > > > > > > > > You changed it to 'data'. It was a u32-used-as-boolean > > > > is_critical_error before. > > > > > > > > So you can just as well make it into flags and people can > > > > extend those flags if needed. A flag bit should be enough in > > > > most cases anyway. If they really need driver_data, then they > > > > can add a void *member. > > > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses > > > this field for PSS and PCC, which are enum values. I think we > > > should allow drivers to set any values here. I agree that it may > > > need to be void * if we also allow drivers to set a pointer here. > > > > Let's see what Rafael prefers. > > I would retain the is_critical_error field and use that for printing > the recoverable / non-recoverable message. This is kind of > orthogonal to whether or not any extra data is needed and that can be > an additional field. In that case unsigned long should be sufficient > to accommodate a pointer if need be. Yes, we will retain the field. The question is whether this field should be retained as a driver's private data or ACPI-managed flags. My patch implements the former, which lets the callers to define the data values. For instance, acpi_blacklisted() uses this field as is_critical_error value, and intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table value. Boris suggested the latter, which lets ACPI to define the flags, which are then used by the callers. For instance, he suggested ACPI to define bit0 as is_critical_error. #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) Thanks, -Toshi
On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote: > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote: >> On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> >> wrote: >> > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu wrote: >> > > > > 'data' here is private to the caller. So, I do not think we >> > > > > need to define the bits. Shall I change the name to >> > > > > 'driver_data' to make it more explicit? >> > > > >> > > > You changed it to 'data'. It was a u32-used-as-boolean >> > > > is_critical_error before. >> > > > >> > > > So you can just as well make it into flags and people can >> > > > extend those flags if needed. A flag bit should be enough in >> > > > most cases anyway. If they really need driver_data, then they >> > > > can add a void *member. >> > > >> > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() uses >> > > this field for PSS and PCC, which are enum values. I think we >> > > should allow drivers to set any values here. I agree that it may >> > > need to be void * if we also allow drivers to set a pointer here. >> > >> > Let's see what Rafael prefers. >> >> I would retain the is_critical_error field and use that for printing >> the recoverable / non-recoverable message. This is kind of >> orthogonal to whether or not any extra data is needed and that can be >> an additional field. In that case unsigned long should be sufficient >> to accommodate a pointer if need be. > > Yes, we will retain the field. The question is whether this field > should be retained as a driver's private data or ACPI-managed flags. Thanks for the clarification. > My patch implements the former, which lets the callers to define the > data values. For instance, acpi_blacklisted() uses this field as > is_critical_error value, and intel_pstate_platform_pwr_mgmt_exists() > uses it as oem_pwr_table value. > > Boris suggested the latter, which lets ACPI to define the flags, which > are then used by the callers. For instance, he suggested ACPI to > define bit0 as is_critical_error. > > #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) So my point is that we can have both the ACPI-managed flags and the the caller-defined data at the same time as separate items. That would allow of maximum flexibility IMO. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote: > On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.co > m> wrote: > > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote: > > > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> > > > wrote: > > > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu > > > > wrote: > > > > > > > 'data' here is private to the caller. So, I do not think > > > > > > > we need to define the bits. Shall I change the name to > > > > > > > 'driver_data' to make it more explicit? > > > > > > > > > > > > You changed it to 'data'. It was a u32-used-as-boolean > > > > > > is_critical_error before. > > > > > > > > > > > > So you can just as well make it into flags and people can > > > > > > extend those flags if needed. A flag bit should be enough > > > > > > in most cases anyway. If they really need driver_data, then > > > > > > they can add a void *member. > > > > > > > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() > > > > > uses this field for PSS and PCC, which are enum values. I > > > > > think we should allow drivers to set any values here. I > > > > > agree that it may need to be void * if we also allow drivers > > > > > to set a pointer here. > > > > > > > > Let's see what Rafael prefers. > > > > > > I would retain the is_critical_error field and use that for > > > printing the recoverable / non-recoverable message. This is kind > > > of orthogonal to whether or not any extra data is needed and that > > > can be an additional field. In that case unsigned long should be > > > sufficient to accommodate a pointer if need be. > > > > Yes, we will retain the field. The question is whether this field > > should be retained as a driver's private data or ACPI-managed > > flags. > > Thanks for the clarification. > > > My patch implements the former, which lets the callers to define > > the data values. For instance, acpi_blacklisted() uses this field > > as is_critical_error value, and > > intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table > > value. > > > > Boris suggested the latter, which lets ACPI to define the flags, > > which are then used by the callers. For instance, he suggested > > ACPI to define bit0 as is_critical_error. > > > > #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) > > So my point is that we can have both the ACPI-managed flags and the > the caller-defined data at the same time as separate items. > > That would allow of maximum flexibility IMO. I agree in general. Driver private data allows flexibility to drivers when the values are driver-private. ACPI-managed flags allows ACPI to control the interfaces based on the flags. Since we do not have use-case of the latter case yet, i.e. acpi_match_platform_list() does not need to check the flags, I'd suggest that we keep 'data' as driver-private. We can add 'flags' as a separate member to the structure when we find the latter use-case. Thanks, -Toshi
On Tue, Aug 22, 2017 at 12:21 AM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote: > On Mon, 2017-08-21 at 23:49 +0200, Rafael J. Wysocki wrote: >> On Mon, Aug 21, 2017 at 11:06 PM, Kani, Toshimitsu <toshi.kani@hpe.co >> m> wrote: >> > On Mon, 2017-08-21 at 22:31 +0200, Rafael J. Wysocki wrote: >> > > On Mon, Aug 21, 2017 at 7:36 PM, Borislav Petkov <bp@alien8.de> >> > > wrote: >> > > > On Mon, Aug 21, 2017 at 05:23:37PM +0000, Kani, Toshimitsu >> > > > wrote: >> > > > > > > 'data' here is private to the caller. So, I do not think >> > > > > > > we need to define the bits. Shall I change the name to >> > > > > > > 'driver_data' to make it more explicit? >> > > > > > >> > > > > > You changed it to 'data'. It was a u32-used-as-boolean >> > > > > > is_critical_error before. >> > > > > > >> > > > > > So you can just as well make it into flags and people can >> > > > > > extend those flags if needed. A flag bit should be enough >> > > > > > in most cases anyway. If they really need driver_data, then >> > > > > > they can add a void *member. >> > > > > >> > > > > Hmm.. In patch 2, intel_pstate_platform_pwr_mgmt_exists() >> > > > > uses this field for PSS and PCC, which are enum values. I >> > > > > think we should allow drivers to set any values here. I >> > > > > agree that it may need to be void * if we also allow drivers >> > > > > to set a pointer here. >> > > > >> > > > Let's see what Rafael prefers. >> > > >> > > I would retain the is_critical_error field and use that for >> > > printing the recoverable / non-recoverable message. This is kind >> > > of orthogonal to whether or not any extra data is needed and that >> > > can be an additional field. In that case unsigned long should be >> > > sufficient to accommodate a pointer if need be. >> > >> > Yes, we will retain the field. The question is whether this field >> > should be retained as a driver's private data or ACPI-managed >> > flags. >> >> Thanks for the clarification. >> >> > My patch implements the former, which lets the callers to define >> > the data values. For instance, acpi_blacklisted() uses this field >> > as is_critical_error value, and >> > intel_pstate_platform_pwr_mgmt_exists() uses it as oem_pwr_table >> > value. >> > >> > Boris suggested the latter, which lets ACPI to define the flags, >> > which are then used by the callers. For instance, he suggested >> > ACPI to define bit0 as is_critical_error. >> > >> > #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0) >> >> So my point is that we can have both the ACPI-managed flags and the >> the caller-defined data at the same time as separate items. >> >> That would allow of maximum flexibility IMO. > > I agree in general. Driver private data allows flexibility to drivers > when the values are driver-private. ACPI-managed flags allows ACPI to > control the interfaces based on the flags. > > Since we do not have use-case of the latter case yet, i.e. > acpi_match_platform_list() does not need to check the flags, I'd > suggest that we keep 'data' as driver-private. We can add 'flags' as a > separate member to the structure when we find the latter use-case. OK Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c index 010b1c43df92..881b0d5b2838 100644 --- a/drivers/acpi/acpica/tbxface.c +++ b/drivers/acpi/acpica/tbxface.c @@ -226,6 +226,9 @@ acpi_get_table_header(char *signature, u32 j; struct acpi_table_header *header; + if (acpi_disabled) + return (AE_ERROR); + /* Parameter validation */ if (!signature || !out_table_header) {