[Skiboot] [PATCH v11 20/23] core/pci: Power off empty hotpluggable slot

[Gavin Shan]

On Wed, Jun 08, 2016 at 05:01:22PM +1000, Stewart Smith wrote:
>Gavin Shan <gwshan at linux.vnet.ibm.com> writes:
>> This powers off the empty hotpluggable slot during PCI enumeration
>> for two purpose: power saving and initialize the slot to starting
>> (power-off) state.
>>
>> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
>> ---
>>  core/pci.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/core/pci.c b/core/pci.c
>> index 21fa823..68b6033 100644
>> --- a/core/pci.c
>> +++ b/core/pci.c
>> @@ -476,6 +476,51 @@ void pci_remove_bus(struct phb *phb, struct list_head *list)
>>  	}
>>  }
>>
>> +/*
>> + * Turn off slot's power supply if there are nothing connected for
>> + * 2 purposes: power saving obviously and initialize the slot to
>> + * to initial power-off state for hotplug.
>> + */
>> +static void pci_slot_power_off(struct phb *phb, struct pci_device *pd)
>> +{
>> +	struct pci_slot *slot;
>> +	int32_t wait = 100;
>> +	int64_t rc;
>> +
>> +	if (!pd || !pd->slot)
>> +		return;
>> +
>> +	slot = pd->slot;
>> +	if (!slot->pluggable || !slot->ops.set_power_state)
>> +		return;
>> +
>> +	/* Bail if there're something connected */
>> +	if (!list_empty(&pd->children))
>> +		return;
>> +
>> +	pci_slot_add_flags(slot, PCI_SLOT_FLAG_BOOTUP);
>> +	rc = slot->ops.set_power_state(slot, PCI_SLOT_POWER_OFF);
>> +	if (rc != OPAL_ASYNC_COMPLETION) {
>> +		pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
>> +		PCINOTICE(phb, pd->bdfn, "Error %lld powering off slot\n", rc);
>> +		return;
>> +	}
>> +
>> +	do {
>> +		if (slot->state == PCI_SLOT_STATE_SPOWER_DONE)
>> +			break;
>> +
>> +		check_timers(false);
>> +		time_wait(10);
>> +	} while (--wait >= 0);
>
>I don't like the time_wait(10) here, as further waiting around for
>things while not doing other useful work isn't so great to have.
>
>The logic around powering on/off slots seems to possibly be platform
>specific in that it either completes instantly (and thus should return
>OPAL_SUCCESS rather than OPAL_ASYNC_COMPLETION) or could be an i2c
>request.
>
>Here, it looks like this code is buggy. e.g. on something like p7ioc (if
>we ever supported this there, which we don't, but anyway) it'll print an
>error even though it could have succeeded as it's checking for rc !=
>OPAL_ASYNC_COMPLETION to be error rather than also checking for
>OPAL_SUCCESS.
>

Yeah, OPAL_SUCCESS is missed to be checked here. I will do in next revision.

>I'd prefer we use a callback rather than busy looping (and there's other
>places in PCI code that does this too, but this appears to be new, so it
>gets unfairly picked on :)
>

I'm afraid we cannot relinquish the CPU for other tasks. This function is
called when skiboot is booting, meaning it's executed in a job context.
I guess we don't support stop/resume job in current implementation. Even
with stop/resume job supported, more information needed to track the states
even that's supported - a bit complexity than the polling mechanism. Lets
have the polling mechanism for now and I will think about it in future.

Thanks,
Gavin

>
>-- 
>Stewart Smith
>OPAL Architect, IBM.