All of lore.kernel.org
 help / color / mirror / Atom feed
From: andy.shevchenko@gmail.com
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: rafael@kernel.org, linux-acpi@vger.kernel.org,
	robert.moore@intel.com, amadeuszx.slawinski@linux.intel.com,
	andriy.shevchenko@linux.intel.com,
	pierre-louis.bossart@linux.intel.com
Subject: Re: [PATCH v4 1/4] ACPI: NHLT: Device configuration access interface
Date: Wed, 9 Aug 2023 08:00:42 +0300	[thread overview]
Message-ID: <ZNMdertpWWvoAJM3@surfacebook> (raw)
In-Reply-To: <20230721154813.310996-2-cezary.rojewski@intel.com>

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?

-- 
With Best Regards,
Andy Shevchenko



  reply	other threads:[~2023-08-09  5:00 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-21 15:48 [PATCH v4 0/4] ACPI: NHLT: Access and query helpers Cezary Rojewski
2023-07-21 15:48 ` [PATCH v4 1/4] ACPI: NHLT: Device configuration access interface Cezary Rojewski
2023-08-09  5:00   ` andy.shevchenko [this message]
2023-08-09  8:48     ` Cezary Rojewski
2023-08-09  9:05       ` Andy Shevchenko
2023-08-09 11:02         ` Cezary Rojewski
2023-07-21 15:48 ` [PATCH v4 2/4] ACPI: NHLT: Introduce acpi_gbl_nhlt Cezary Rojewski
2023-07-21 15:48 ` [PATCH v4 3/4] ACPI: NHLT: Table manipulation helpers Cezary Rojewski
2023-07-21 15:48 ` [PATCH v4 4/4] ACPI: NHLT: Add query functions Cezary Rojewski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZNMdertpWWvoAJM3@surfacebook \
    --to=andy.shevchenko@gmail.com \
    --cc=amadeuszx.slawinski@linux.intel.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=cezary.rojewski@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=rafael@kernel.org \
    --cc=robert.moore@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.