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

[Stewart Smith]

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.

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 :)


-- 
Stewart Smith
OPAL Architect, IBM.