On Wed, Mar 23, 2022 at 11:29 AM Verma, Vishal L
<vishal.l.verma@xxxxxxxxx> wrote:
[..]
> > > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 support)
> > > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask,
> > > + u32 support, u32 *cxl_mask,
> > > + u32 cxl_support)
> > > {
> > > u32 req = OSC_PCI_EXPRESS_CAPABILITY_CONTROL;
> > > struct acpi_pci_root *root;
> > > acpi_status status;
> > > - u32 ctrl, capbuf[6];
> > > + u32 ctrl, cxl_ctrl = 0, capbuf[OSC_CAPABILITY_DWORDS_MAX];
> >
> > ... which btw why is capbuf 6 in the previous patch and now 5 in this one?
> > Sorry if I missed anything obvious here, just seems odd.
>
> Oh, I just noticed patch 1 changes it to 6 in the first place (I
> thought it was just 6 even before this set), that is wrong. Previously,
> the PCI only _OSC had 3, and with CXL it becomes 5. I'll fix this up.
Yeah, my fault for making it 6 think I was confusing the DWORD numbers
as 0-based.
> > And also it's ugly to just add extra params to acpi_pci_osc_control_set()
> > and have callers do do:
> >
> > > + status = acpi_pci_osc_control_set(handle, &control, support,
> > > + &cxl_control, cxl_support);
> >
> > And this sort of thing happens all over the patch with struct acpi_pci_root.
> > So the whole handling of the _OSC state of support/control bits imo really
> > wants to be consolidated between CXL and non-CXL.
>
> I don't disagree :) - Any thoughts on what can be done to consolidate
> things further? This seemed like the lowest touch approach that kept
> existing PCI-only paths as-is.
That sounds like a follow-on cleanup opportunity to me, but we have
time since this series seems like v5.19 material given the merge
window is already open.
