| Message ID | 20230721154813.310996-2-cezary.rojewski@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: NHLT: Access and query helpers | expand |
Fri, Jul 21, 2023 at 05:48:10PM +0200, Cezary Rojewski kirjoitti: > Device configuration structures are plenty so declare a struct for each > known variant. As neither of them shall be accessed without verifying > the memory block first, introduce macros to make it easy to do so. > > Link: https://github.com/acpica/acpica/pull/881 Thinking of this over night (as I replied in the above)... > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Sorry, but seems I have to retract my tag and even more, NAK to the ACPICA changes. I have thought that this is something new to the header there, but it appears that it duplicates (in a wrong way in my opinion) existing data types. Existing data types are crafted (as far as I get them) in a way to be able to be combined in the union. In the similar way how _CRS is parsed in DSDT (first that comes to my mind). Hence that "simplification" is quite wrong in a few ways: - it breaks ACPICA agreement on naming schema - it duplicates existing data - it made it even partially - it is fine and correct in ACPICA to have long dereferenced data, again see for the union of acpi_object I trully believe now that the above change in ACPICA must be reverted. Again, sorry for this late bad news from my side. I have no clue why it was merged, perhaps lack of review? Or anything subtle I so miserably missed?
On 2023-08-09 7:00 AM, andy.shevchenko@gmail.com wrote: > Fri, Jul 21, 2023 at 05:48:10PM +0200, Cezary Rojewski kirjoitti: >> Device configuration structures are plenty so declare a struct for each >> known variant. As neither of them shall be accessed without verifying >> the memory block first, introduce macros to make it easy to do so. >> >> Link: https://github.com/acpica/acpica/pull/881 > > Thinking of this over night (as I replied in the above)... > >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sorry, but seems I have to retract my tag and even more, NAK to the ACPICA changes. > > I have thought that this is something new to the header there, but it appears that > it duplicates (in a wrong way in my opinion) existing data types. > > Existing data types are crafted (as far as I get them) in a way to be able to be > combined in the union. In the similar way how _CRS is parsed in DSDT (first that > comes to my mind). Hence that "simplification" is quite wrong in a few ways: > - it breaks ACPICA agreement on naming schema > - it duplicates existing data > - it made it even partially > - it is fine and correct in ACPICA to have long dereferenced data, again see > for the union of acpi_object > > I trully believe now that the above change in ACPICA must be reverted. > > Again, sorry for this late bad news from my side. I have no clue why > it was merged, perhaps lack of review? Or anything subtle I so miserably > missed? First, you took the review seriously and provided a ton of valid feedback. And your reviews and expertise helped me grow as a developer, so from my perspective no need to sorry about spotting bad things late. Now, I admit, a bit surprised given the number of revisions and age of the initial patchset. The cover-letter, attached for each revision, made the intentions clear. Our goal is to help actual users of NHLT i.e.: audio teams. While part of ACPICA, NHLT-code is hidden within sound/ so no one asks questions. Leaving things at status quo does not improve the situation. Thus I believe simple "no" is not an option here. To make the code better overall, relevant pieces should be made part of drivers/acpi. Original problems stem from the fact that audio teams were not looped in during initial integration of NHLT-code. Turned out that no users utilize it in its current form. The problems are subtle, but a discussion wouldn't hurt. To avoid double posting, should we continue the discussion here or in the PR on github? Kind regards, Czarek
On Wed, Aug 9, 2023 at 11:48 AM Cezary Rojewski <cezary.rojewski@intel.com> wrote: > > On 2023-08-09 7:00 AM, andy.shevchenko@gmail.com wrote: > > Fri, Jul 21, 2023 at 05:48:10PM +0200, Cezary Rojewski kirjoitti: > >> Device configuration structures are plenty so declare a struct for each > >> known variant. As neither of them shall be accessed without verifying > >> the memory block first, introduce macros to make it easy to do so. > >> > >> Link: https://github.com/acpica/acpica/pull/881 > > > > Thinking of this over night (as I replied in the above)... > > > >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Sorry, but seems I have to retract my tag and even more, NAK to the ACPICA changes. > > > > I have thought that this is something new to the header there, but it appears that > > it duplicates (in a wrong way in my opinion) existing data types. > > > > Existing data types are crafted (as far as I get them) in a way to be able to be > > combined in the union. In the similar way how _CRS is parsed in DSDT (first that > > comes to my mind). Hence that "simplification" is quite wrong in a few ways: > > - it breaks ACPICA agreement on naming schema > > - it duplicates existing data > > - it made it even partially > > - it is fine and correct in ACPICA to have long dereferenced data, again see > > for the union of acpi_object > > > > I trully believe now that the above change in ACPICA must be reverted. > > > > Again, sorry for this late bad news from my side. I have no clue why > > it was merged, perhaps lack of review? Or anything subtle I so miserably > > missed? > > First, you took the review seriously and provided a ton of valid > feedback. And your reviews and expertise helped me grow as a developer, > so from my perspective no need to sorry about spotting bad things late. > > Now, I admit, a bit surprised given the number of revisions and age of > the initial patchset. The cover-letter, attached for each revision, made > the intentions clear. As you may notice I'm not against code that is done as a part of the Linux kernel and my surprise is the ACPICA change. My focus for review was a Linux kernel and it was just by a chance I looked at the PR on GitHub. There is neither good explanation in the commit message nor discussion of the change. What I probably miss (and that may help me to understand better the change) are: - the examples of the code snippets that are using data types before and after - explanation why not all data types were covered (there are more "strange" names like with _a, _b suffix) - how this is supposed to be maintained as the ACPICA has users outside of the kernel and how the change makes their life easier (to me it's the opposite). > Our goal is to help actual users of NHLT i.e.: > audio teams. While part of ACPICA, NHLT-code is hidden within sound/ so > no one asks questions. Leaving things at status quo does not improve the > situation. What situation? To me it makes it worse. (Again, I'm talking solely for ACPICA change, the rest I have reviewed and I am fine with the direction taken.) > Thus I believe simple "no" is not an option here. To make the > code better overall, relevant pieces should be made part of drivers/acpi. > > Original problems stem from the fact that audio teams were not looped in > during initial integration of NHLT-code. Turned out that no users > utilize it in its current form. The problems are subtle, but a > discussion wouldn't hurt. > > To avoid double posting, should we continue the discussion here or in > the PR on github? Let's do it there, as it's purely about ACPICA. The kernel part will be affected depending on the result of the discussion.
On 2023-08-09 11:05 AM, Andy Shevchenko wrote: > On Wed, Aug 9, 2023 at 11:48 AM Cezary Rojewski > <cezary.rojewski@intel.com> wrote: ... >> First, you took the review seriously and provided a ton of valid >> feedback. And your reviews and expertise helped me grow as a developer, >> so from my perspective no need to sorry about spotting bad things late. >> >> Now, I admit, a bit surprised given the number of revisions and age of >> the initial patchset. The cover-letter, attached for each revision, made >> the intentions clear. > > As you may notice I'm not against code that is done as a part of the > Linux kernel and my surprise is the ACPICA change. My focus for review > was a Linux kernel and it was just by a chance I looked at the PR on > GitHub. There is neither good explanation in the commit message nor > discussion of the change. What I probably miss (and that may help me > to understand better the change) are: > - the examples of the code snippets that are using data types before and after > - explanation why not all data types were covered (there are more > "strange" names like with _a, _b suffix) > - how this is supposed to be maintained as the ACPICA has users > outside of the kernel and how the change > makes their life easier (to me it's the opposite). In general, many types are bogus or redundant. I'd argue that having types defined as _a, _b, _c, _d make the life harder :) struct acpi_nhlt_device_specific_config_a bogus, there is no '_a', it's called mic device config instead struct acpi_nhlt_device_specific_config_d bogus, such thing does not exist it breaks the spec as the "CapabilitiesSize" is missing struct acpi_nhlt_device_specific_config_b bogus, such thing does not exist it's the header of any dev config but just header alone is invalid struct acpi_nhlt_device_specific_config_c bogus, describes an invalid type. Such thing can be encountered only when parsing damaged NHLT struct acpi_nhlt_render_device_specific_config redundant Then we have constants such as: #define ACPI_NHLT_PDM 2 #define ACPI_NHLT_SSP 3 _PDM/_SSP what? There is no NHLT of type PDM or of type SSP. These specify link types but it's not mentioned in constants names. I believe that by now you see where am going. The patch focuses on device-specific-config area as it's the area that requires most help. >> Our goal is to help actual users of NHLT i.e.: >> audio teams. While part of ACPICA, NHLT-code is hidden within sound/ so >> no one asks questions. Leaving things at status quo does not improve the >> situation. > > What situation? To me it makes it worse. (Again, I'm talking solely > for ACPICA change, the rest I have reviewed and I am fine with the > direction taken.) Situation = on top of above, NHLT-code is currently within sound/. Let the handlers be part of drivers/acpi as is the case for all ACPI tables. The handlers themselves do not (and shall not) belong to ACPICA if I'm not mistaken. >> Thus I believe simple "no" is not an option here. To make the >> code better overall, relevant pieces should be made part of drivers/acpi. >> >> Original problems stem from the fact that audio teams were not looped in >> during initial integration of NHLT-code. Turned out that no users >> utilize it in its current form. The problems are subtle, but a >> discussion wouldn't hurt. >> >> To avoid double posting, should we continue the discussion here or in >> the PR on github? > > Let's do it there, as it's purely about ACPICA. > The kernel part will be affected depending on the result of the discussion. Ok.
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h index 0029336775a9..c4c9a3a89ba6 100644 --- a/include/acpi/actbl2.h +++ b/include/acpi/actbl2.h @@ -2014,6 +2014,25 @@ struct acpi_nhlt_vendor_mic_count { u8 microphone_count; }; +/* The only guaranteed configuration space header. Any other requires validation. */ +struct acpi_nhlt_cfg { + u32 capabilities_size; + u8 capabilities[]; +}; + +struct acpi_nhlt_devcfg { + u32 capabilities_size; + u8 virtual_slot; + u8 config_type; +}; + +struct acpi_nhlt_mic_devcfg { + u32 capabilities_size; + u8 virtual_slot; + u8 config_type; + u8 array_type; +}; + struct acpi_nhlt_vendor_mic_config { u8 type; u8 panel; @@ -2030,6 +2049,15 @@ struct acpi_nhlt_vendor_mic_config { u16 work_horizontal_angle_end; /* -180 - + 180 with 2 deg step */ }; +struct acpi_nhlt_vendor_mic_devcfg { + u32 capabilities_size; + u8 virtual_slot; + u8 config_type; + u8 array_type; + u8 num_mics; + struct acpi_nhlt_vendor_mic_config mics[]; +}; + /* Values for Type field above */ #define ACPI_NHLT_MIC_OMNIDIRECTIONAL 0 diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h new file mode 100644 index 000000000000..af3ec45ba4f9 --- /dev/null +++ b/include/acpi/nhlt.h @@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright(c) 2023 Intel Corporation. All rights reserved. + * + * Authors: Cezary Rojewski <cezary.rojewski@intel.com> + * Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> + */ + +#ifndef __ACPI_NHLT_H__ +#define __ACPI_NHLT_H__ + +#include <linux/acpi.h> +#include <linux/overflow.h> +#include <linux/types.h> + +#define __acpi_nhlt_endpoint_cfg(ep) ((void *)((ep) + 1)) + +/* + * As device configuration spaces present in NHLT tables around the world are + * not following single pattern, first check if 'capabilities_size' is correct + * in respect to size of the specified space type before returning the pointer. + */ +#define __acpi_nhlt_endpoint_devcfg(ep, type) ({ \ + struct acpi_nhlt_cfg *__cfg = __acpi_nhlt_endpoint_cfg(ep); \ + __cfg->capabilities_size >= sizeof(type) ? \ + ((type *)__cfg) : NULL; }) + +/* + * acpi_nhlt_endpoint_devcfg - Test and access device configuration. + * @ep: endpoint for which to retrieve device configuration. + * + * Return: A pointer to device configuration space or NULL if the space's + * 'capabilities_size' is insufficient to cover the nested structure. + */ +#define acpi_nhlt_endpoint_devcfg(ep) \ + __acpi_nhlt_endpoint_devcfg(ep, struct acpi_nhlt_devcfg) + +/* + * acpi_nhlt_endpoint_mic_devcfg - Test and access device configuration. + * @ep: endpoint for which to retrieve device configuration. + * + * Return: A pointer to device configuration space or NULL if the space's + * 'capabilities_size' is insufficient to cover the nested structure. + */ +#define acpi_nhlt_endpoint_mic_devcfg(ep) \ + __acpi_nhlt_endpoint_devcfg(ep, struct acpi_nhlt_mic_devcfg) + +/* + * acpi_nhlt_endpoint_vendor_mic_devcfg - Test and access device configuration. + * @ep: endpoint for which to retrieve device configuration. + * @ptr: pointer to a device configuration structure. + * + * This is the same as acpi_nhlt_endpoint_devcfg(), except that it verifies + * if size of the flexible array following the structure header is also + * reflected in 'capabilities_size'. + * + * Return: A pointer to device configuration space or NULL if the space's + * 'capabilities_size' is insufficient to cover the nested structure. + */ +#define acpi_nhlt_endpoint_vendor_mic_devcfg(ep) ({ \ + struct acpi_nhlt_vendor_mic_devcfg *__cfg = __acpi_nhlt_endpoint_cfg(ep); \ + __cfg->capabilities_size >= sizeof(*__cfg) && \ + __cfg->capabilities_size == struct_size(__cfg, mics, __cfg->num_mics) ? \ + __cfg : NULL; }) + +#endif /* __ACPI_NHLT_H__ */