On Tue, 2022-03-22 at 17:47 -0700, Davidlohr Bueso wrote:
> On Fri, 18 Mar 2022, Vishal Verma wrote:
>
> > +/* Max possible _OSC capability DWORDS */
> > +#define OSC_CAPABILITY_DWORDS_MAX 5
> > +
> > /* Indexes into _OSC Capabilities Buffer (DWORDs 2 & 3 are device-specific) */
> > #define OSC_QUERY_DWORD 0 /* DWORD 1 */
> > #define OSC_SUPPORT_DWORD 1 /* DWORD 2 */
> > #define OSC_CONTROL_DWORD 2 /* DWORD 3 */
> > +#define OSC_CXL_SUPPORT_DWORD 3 /* DWORD 4 */
> > +#define OSC_CXL_CONTROL_DWORD 4 /* DWORD 5 */
>
> Shouldn't all this be in patch 1/2 (and also as enum maybe)? Or at least
> the define OSC_CAPABILITY_DWORDS_MAX instead of having to do:
Yeah moving DWORDS_MAX and the associated changes to patch 1 makes
sense.
>
> > -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.
>
> 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.
>
> Thanks,
> Davidlohr
Thanks for the review!
