NHLT: Declare device configuration types

[crojewsk-intel]

Device configuration structures are plenty so declare a struct for each known variant. While these kind of duplicate few types present in actbl2.h already, change is motivated by usage improvements - simplicity and shorten wording. Intention is to have them replacing the existing members in the future.

[crojewsk-intel]

Changes in v2:

• dropped struct acpi_nhlt_devcfg0 as its not used anywhere, applications utilize only devcfg, mic_devcfg and mic_defcfg-descendants

[crojewsk-intel]

Referenced in ACPI: NHLT: Device configuration access interface series present on linux-acpi@vger.kernel.org mailing list.

[sacdintel]

Looks good to me! Thanks!

[andy-shev]

Hmm... First of all, I have been stumbed over the duplication, then read the commit message and understood the motivation. BUT

• the update doesn't cover all existing descriptions
• the update doesn't follow (as far as I understand it) the ACPICA naming convention (fully spelled word in data type names)

Is my knowledge incorrect / outdated? What did I miss?

(Personally I think this patch shouldn't be merged, at least in the current form, and sorry for the late reply.)

[crojewsk-intel]

• the update doesn't cover all existing descriptions

I didn't mean to cover all of them. As the PR description and the commit message states, it's all about device-specific-configuration types which are, unfortunately, a mess on NHLT side.

• the update doesn't follow (as far as I understand it) the ACPICA naming convention (fully spelled word in data type names)

So, this was my main motivation for the change - we were reaching character-limit way to quickly and, the naming pattern utilized throughout the years by the audio teams (who are the main users of the spec) is different. There's no magic here.

When the NHLT addition was first added to the ACPICA on my request [1], neither me or any other audio team member were looped in for the review. So, we ended up with code that none uses. Decided to change that and start modelling the functionality in more user-friendly fashion.

Is my knowledge incorrect / outdated? What did I miss?

(Personally I think this patch shouldn't be merged, at least in the current form, and sorry for the late reply.)

If the changes break ACPICA standard, I have no problem with reverting the patch. However, I'd suggest that the users (audio teams) are not ignored and together we come up with something friendly for both parties.

[1]: https://bugs.acpica.org/show_bug.cgi?id=1525

[andy-shev]

I believe this change brings more confusion and problems that it tries to solve and has to be reverted (it even has no reviews done according to the side bar of this very page). I tried to explained that in my reply in the mailing list. Since the discussion is about ACPICA change (and not about part that is Linux kernel specific) I will copy'n'paste my answer and let's continue here. (But to the ACPICA maintainers I really don't want to see a next release with this be in. OTOH not sure if my opinion will be respected.)

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: #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.

[crojewsk-intel]

Either I had some kind of outlook malfunction or I have received this message twice, once as a reply to the 1/4 patch on the kernel mailing list and the other copy navigating here. Replying with exact same message I did send over the mailing list.

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.

[andy-shev]

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

I disagree on this. I think we need an author of that specification to be present in the discussion.

struct acpi_nhlt_device_specific_config_a
	bogus, there is no '_a', it's called mic device config instead

It may be _a is generic for some cases and mic is vendor specific (See CSRT data types, for example).

struct acpi_nhlt_device_specific_config_d
bogus, such thing does not exist
it breaks the spec as the "CapabilitiesSize" is missing

(Hmm... There is no _d variant in the code, okay)

I believe here is the misinterpretation of how it should look like.
I think it has to be something like

union ... {
    struct acpi_nhlt_device_specific_config
    struct acpi_nhlt_device_specific_config_a
    struct acpi_nhlt_device_specific_config_b
    struct acpi_nhlt_device_specific_config_c
    struct acpi_nhlt_render_device_specific_config
    ...data type X...
    ...data type Y...
  }
}

Something similar to the format data structures with all those constants be used. It's all over the ACPICA (I mean the design), and your change breaks this AFAICS.

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.

Renaming constants may be fine, but I'm not talking about them.

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.

Yes, but then why touching ACPICA parts which are already there?

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.

Yes, and I'm fully in support of this. My objection is how ACPICA change is done here.

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.

[crojewsk-intel]

In general, many types are bogus or redundant. I'd argue that having types defined as _a, _b, _c, _d make the life harder :)
I disagree on this. I think we need an author of that specification to be present in the discussion.

Office of the author is right in front of mine. I can ask Marcin to comment on the ACPICA NHLT implementation but I'm pretty sure he will NACK it, just like I would. No one of the audio team was CCed when the structs were added. Again, we ended up with functionality that's either hard to use or unusable.

struct acpi_nhlt_device_specific_config_a
   bogus, there is no '_a', it's called mic device config instead

It may be _a is generic for some cases and mic is vendor specific (See CSRT data types, for example).
Unfortunate truth is: it's not part of the spec and hurts readability. Let's use the proper names.

(Hmm... There is no _d variant in the code, okay)

crojewsk@crojewsk-ctrl:~/reef/acpica/source/include$ grep device_specific_config actbl2.h

typedef struct acpi_nhlt_device_specific_config
typedef struct acpi_nhlt_device_specific_config_a
typedef struct acpi_nhlt_device_specific_config_d
typedef struct acpi_nhlt_device_specific_config_b
typedef struct acpi_nhlt_device_specific_config_c
typedef struct acpi_nhlt_render_device_specific_config
typedef struct acpi_nhlt_mic_device_specific_config
typedef struct acpi_nhlt_vendor_mic_device_specific_config
typedef struct acpi_nhlt_render_feedback_device_specific_config

What am I missing?

I believe here is the misinterpretation of how it should look like.
I think it has to be something like

union ... {
   struct acpi_nhlt_device_specific_config
   struct acpi_nhlt_device_specific_config_a
   struct acpi_nhlt_device_specific_config_b
   struct acpi_nhlt_device_specific_config_c
   struct acpi_nhlt_render_device_specific_config
   ...data type X...
   ...data type Y...
 }
}

Something similar to the format data structures with all those constants be used. It's all over the ACPICA (I mean the design), > and your change breaks this AFAICS.

union {
    struct acpi_nhlt_cfg
    struct acpi_nhlt_devcfg
    struct acpi_nhlt_mic_devcfg
    struct acpi_nhlt_vendor_mic_devcfg
};

should work just fine? It pictures how things are in the field nicely.

[sacdintel]

Hi @andy-shev and @crojewsk-intel ,

Here are my thoughts regarding this discussion:

While I have merged this PR already, I can always revert it if there are changes needed (and warranted) and if some things are not "up to the mark" depending on all the data/insights available and what it points to. Since this discussion here is only going back and forth and seeing this is an Intel internal issue, I would like to propose having you both and even the creator/designer of the NHLT table in the next OS/Firmware meeting (hosted by @rafaeljw ) to have an intellectual and positive debate not only on the ACPICA related patch but also any applicable/related kernel patches.

The next scheduled meeting is on August 22nd Tuesday (since Tuesday the 15th is a holiday in Poland) at 10 AM PST (the exact time right now + a few more minutes). I would really appreciate if you both show up to the meeting since a few minutes of discussion there could save hours of future debate here or on the kernel mailing list. Please check your email in a few minutes from now for the invitation. We will have all the key decision makers in that meeting (@rafaeljw , @acpibob , Intel Fellow Mark Doran, Ankit Sinha, etc.) from Intel to discuss their thoughts about this and we can hopefully proceed ahead with a consensus on what can be done or changed.

Personally, I would like to help the audio team in their efforts to make their lives easier, but like Andy pointed out things need to be followed according to a certain standard to effectively have your patches merged upstream (kernel, ACPICA etc.) and hopefully we can soon find some common ground on this discussion here. Kindly let me know if this is something we could agree to do to come to a solution. Thanks a lot!

• Saket Dumbre (ACPICA, Linux Power Management. Intel)

[andy-shev]

Sure, but I mentioned the author of the initial implementation of the NHLT in the ACPICA. Is that the same person who wants to NAK it now? I am confused.

[andy-shev]

Hi @andy-shev and @crojewsk-intel ,

Here are my thoughts regarding this discussion:

While I have merged this PR already, I can always revert it if there are changes needed (and warranted) and if some things are not "up to the mark" depending on all the data/insights available and what it points to. Since this discussion here is only going back and forth and seeing this is an Intel internal issue, I would like to propose having you both and even the creator/designer of the NHLT table in the next OS/Firmware meeting (hosted by @rafaeljw ) to have an intellectual and positive debate not only on the ACPICA related patch but also any applicable/related kernel patches.

The next scheduled meeting is on August 22nd Tuesday (since Tuesday the 15th is a holiday in Poland) at 10 AM PST (the exact time right now + a few more minutes). I would really appreciate if you both show up to the meeting since a few minutes of discussion there could save hours of future debate here or on the kernel mailing list. Please check your email in a few minutes from now for the invitation. We will have all the key decision makers in that meeting (@rafaeljw , @acpibob , Intel Fellow Mark Doran, Ankit Sinha, etc.) from Intel to discuss their thoughts about this and we can hopefully proceed ahead with a consensus on what can be done or changed.

Personally, I would like to help the audio team in their efforts to make their lives easier, but like Andy pointed out things need to be followed according to a certain standard to effectively have your patches merged upstream (kernel, ACPICA etc.) and hopefully we can soon find some common ground on this discussion here. Kindly let me know if this is something we could agree to do to come to a solution. Thanks a lot!

• Saket Dumbre (ACPICA, Linux Power Management. Intel)

Can you send an invitation?

[crojewsk-intel]

Sure, but I mentioned the author of the initial implementation of the NHLT in the ACPICA. Is that the same person who wants to NAK it now? I am confused.

Misunderstanding on my part, sorry. I thought you meant author/designer of the NHLT spec. Marcin did not contribute to the implementation found in ACPICA.

[sacdintel]

Apologies for the delayed response, but I am back in the office now. Please check your inbox right now for the meeting invite which starts in about 15 minutes, thanks!