Re: [PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity

[Aaron Sierra <asierra@xes-inc.com>]

----- Original Message -----
> From: "Bjorn Helgaas" <helgaas@kernel.org>
> To: "Aaron Sierra" <asierra@xes-inc.com>
> Sent: Wednesday, January 30, 2019 4:57:53 PM
> Subject: Re: [PATCH v2 2/2] PCI/ACPI: Improve _OSC control request granularity

> On Thu, Oct 25, 2018 at 11:01:32AM -0500, Aaron Sierra wrote:
>> This patch reorganizes negotiate_os_control() to be less ASPM-centric in
>> order to:
>> 
>>     1. allow other features (notably AER) to work without enabling ASPM
>>     2. better isolate feature-specific tests for readability/maintenance
> 
> I really like this idea; thanks for working it up!

Hi Bjorn,

Thanks for reviewing this. I've added follow-ups to each of your comments
below.

>> Each feature (ASPM, PCIe hotplug, SHPC hotplug, and AER) now has its own
>> inline function for setting its _OSC control requests.
>> 
>> Part of making this function more generic, required eliminating a test
>> for overall success/failure that previously caused two different types
>> of messages to be printed. Now, printed messages are streamlined to
>> always show requested _OSC control versus what was granted.
>> 
>> Previous output (success):
>> 
>>   acpi PNP0A08:00: _OSC: OS now controls [PME AER PCIeCapability LTR]
>> 
>> Previous output (failure):
>> 
>>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>>   acpi PNP0A08:00: _OSC: platform willing to grant []
>> 
>> Now:
>> 
>>   acpi PNP0A08:00: _OSC: OS requested [PME AER PCIeCapability LTR]
>>   acpi PNP0A08:00: _OSC: platform granted [PME AER PCIeCapability LTR]
>> 
>> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> ---
>>  drivers/acpi/pci_root.c | 118 ++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 85 insertions(+), 33 deletions(-)
>> 
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index eb9f14e..9685aba 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -53,9 +53,10 @@ static int acpi_pci_root_scan_dependent(struct acpi_device
>> *adev)
>>  }
>>  
>>  #define ACPI_PCIE_REQ_SUPPORT (OSC_PCI_EXT_CONFIG_SUPPORT \
>> -				| OSC_PCI_ASPM_SUPPORT \
>> -				| OSC_PCI_CLOCK_PM_SUPPORT \
>>  				| OSC_PCI_MSI_SUPPORT)
>> +#define ACPI_PCIE_ASPM_SUPPORT (ACPI_PCIE_REQ_SUPPORT \
>> +				| OSC_PCI_ASPM_SUPPORT \
>> +				| OSC_PCI_CLOCK_PM_SUPPORT)
>>  
>>  static const struct acpi_device_id root_device_ids[] = {
>>  	{"PNP0A03", 0},
>> @@ -421,6 +422,72 @@ acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>> u32 *mask, u32 req)
>>  }
>>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>  
>> +static inline bool __osc_have_support(u32 support, u32 required)
>> +{
>> +	return ((support & required) == required);
>> +}
> 
> I'm not really a fan of function names with leading underscores, except
> maybe for "raw" things that don't acquire locks.

No problem, I will strip the underscores in v3.

>> +static inline int __osc_set_aspm_control(struct acpi_pci_root *root,
>> +					 u32 support, u32 *control)
>> +{
>> +	if (!IS_ENABLED(CONFIG_PCIEASPM) ||
>> +	    !__osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT))
>> +		return -ENODEV;
>> +
>> +	*control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>> +		    OSC_PCI_EXPRESS_LTR_CONTROL |
>> +		    OSC_PCI_EXPRESS_PME_CONTROL;
> 
> I think this would be more readable if we could avoid the double
> negatives, e.g.,
> 
>  if (IS_ENABLED(CONFIG_PCIEASPM) &&
>      __osc_have_support(support, ACPI_PCIE_ASPM_SUPPORT)) {
>          *control |= OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>                      OSC_PCI_EXPRESS_LTR_CONTROL |
>                      OSC_PCI_EXPRESS_PME_CONTROL;
>          return 0;
>  }
> 
>  return -ENODEV;

I tend to try to avoid avoidable indentation and to get out as early as
possible. In the case of these tiny functions, the style you suggested doesn't
cause any additional line wrapping or complexity. I will make this change,
except for __osc_set_aer_control(), where my original structure seems to be a
better fit.

> Since the caller ignores the return values anyway, I wonder if this
> could also work by *returning* the new mask bits instead of using
> "control" as a reference parameter, e.g.,
> 
>  if (IS_ENABLED(...))
>    return OSC_PCI_EXPRESS_CAPABILITY_CONTROL |
>           OSC_PCI_EXPRESS_LTR_CONTROL |
>           OSC_PCI_EXPRESS_PME_CONTROL;
> 
>  return 0;
> 
> Then the calls would look like:
> 
>  control |= __osc_set_pciehp_control(root, support);
>  control |= __osc_set_shpchp_control(root, support);
>  ...

Yes, I do like that better. I had a draft with that structure, but I changed
for some reason. FYI, I'm inclined to rename these bit-setting functions to
"osc_get_X_control_bits()" to try to avoid confusion. Hopefully that sits well
with you.

-Aaron