Re: [PATCH 2/2] acpi/pci_root: negotiate CXL _OSC

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

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!