Re: [PATCH v3 3/3] acpi/pci_root: negotiate CXL _OSC

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Wed, 2022-03-30 at 14:01 -0500, Bjorn Helgaas wrote:
> Don't just make up new prefixes for the subject line.  Previous ones
> look like this:
> 
>   PCI/ACPI: Fix acpi_pci_osc_control_set() kernel-doc comment
>   ACPI: Use acpi_fetch_acpi_dev() instead of acpi_bus_get_device()
>   PCI/ACPI: Check for _OSC support in acpi_pci_osc_control_set()
>   PCI/ACPI: Move _OSC query checks to separate function
>   PCI/ACPI: Move supported and control calculations to separate functions
>   PCI/ACPI: Remove OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS
>   ACPI: pci_root: Unify the message printing
>   PCI/ACPI: Clarify message about _OSC failure
>   PCI/ACPI: Remove unnecessary osc_lock
>   PCI/ACPI: Make acpi_pci_osc_control_set() static
>   PCI/ACPI: Replace open coded variant of resource_union()
> 
> So I think "PCI/ACPI: " would be a good choice.  Also capitalize the
> next word as all the above do.

Yep will change to "PCI/ACPI:"

> 
> On Wed, Mar 30, 2022 at 12:14:34PM -0600, Vishal Verma wrote:
> > Add full support for negotiating _OSC as defined in the CXL 2.0 spec, as
> 
> Please include a section reference.

Ok

> 
> > applicable to CXL-enabled platforms. Advertise support for the CXL
> > features we support - 'CXL 2.0 port/device register access', 'Protocol
> > Error Reporting', and 'CL Native Hot Plug'. Request control for 'CXL
> 
> "CL" looks like a typo for "CXL"?

Yep, fixed for v4.

> 
> > Memory Error Reporting'. The requests are dependent on CONFIG_* based
> > pre-requisites, and prior PCI enabling, similar to how the standard PCI
> 
> s/pre-requisites/prerequisites/

Ack.

> 
> > _OSC bits are determined.
> > 
> > The CXL specification does not define any additional constraints on
> > the hotplug flow beyond PCIe native hotplug, so a kernel that supports
> > native PCIe hotplug, supports CXL hotplug. For error handling protocol
> > and link errors just use PCIe AER. There is nascent support for
> > amending AER events with CXL specific status [1], but there's
> > otherwise no additional OS responsibility for CXL errors beyond PCIe
> > AER. CXL Memory Errors behave the same as typical memory errors so
> > CONFIG_MEMORY_FAILURE is sufficient to indicate support to platform
> > firmware.
> > 
> > [1]: https://lore.kernel.org/linux-cxl/164740402242.3912056.8303625392871313860.stgit@dwillia2-desk3.amr.corp.intel.com/
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Robert Moore <robert.moore@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> What was reported by the robot?  If it just complained about something
> in v1 or v2, I think there's no point in mentioning this here.  It's
> the same as ordinary review comments (like these I'm composing), and
> they don't need to be acknowledged.  I think "Reported-by" is great
> when giving credit for bug fixes, but that's not what's happening
> here.

Correct it was a compile warning, and actually it wasn't on-list - 0day
sent it privately, because it was on the RFC version. It makes sense to
treat it as a normal review comment - I only added the tag because the
0day emails ask you to (I suppose they use the trailers for metrics on
actionable feedback generated by the bot). I'm happy to drop it if
that's preferred here.

> 
> Bjorn