On Wed, Aug 9, 2023 at 11:48 AM Cezary Rojewski <cezary.rojewski@xxxxxxxxx> wrote: > > On 2023-08-09 7:00 AM, andy.shevchenko@xxxxxxxxx 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@xxxxxxxxxxxxxxx> > > > > 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. -- With Best Regards, Andy Shevchenko
