[Skiboot] [PATCH 03/16] core/pci: Use proper phandle during hotplug for PHB slots

[Andrew Donnellan]

On 9/9/19 2:31 pm, Frederic Barrat wrote:
> PHB slots don't have an associated device (slot->pd = NULL). They were
> not used by the PCI hotplug framework so far, but with opencapi
> virtual PHBs, that's changing. With opencapi, devices are directly
> under the PHB (no root complex or intermediate bridge) and the slot
> used for hotplug is the PHB slot.
> 
> This patch uses the proper phandle when replying asynchronously to the
> OS when using a PHB slot.
> 
> Signed-off-by: Frederic Barrat <fbarrat at linux.ibm.com>

Looks sensible.

I've also had a look through opal_pci_set_power_state() and I don't see 
any way that we can trigger a null dereference by trying to call that 
API with a virtual PHB in the current code, as an OpenCAPI PHB has 
neither a pd or the slot ops required for slot power-on/off.

Reviewed-by: Andrew Donnellan <ajd at linux.ibm.com>

> ---
>   core/pci-opal.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/core/pci-opal.c b/core/pci-opal.c
> index 175810d0..69bd65c6 100644
> --- a/core/pci-opal.c
> +++ b/core/pci-opal.c
> @@ -647,6 +647,17 @@ static int64_t opal_pci_get_power_state(uint64_t id, uint64_t data)
>   }
>   opal_call(OPAL_PCI_GET_POWER_STATE, opal_pci_get_power_state, 2);
>   
> +static u32 get_slot_phandle(struct pci_slot *slot)
> +{
> +	struct phb *phb = slot->phb;
> +	struct pci_device *pd = slot->pd;
> +
> +	if (pd)
> +		return pd->dn->phandle;
> +	else
> +		return phb->dt_node->phandle;
> +}
> +
>   static void rescan_slot_devices(struct pci_slot *slot)
>   {
>   	struct phb *phb = slot->phb;
> @@ -671,8 +682,6 @@ static void set_power_timer(struct timer *t __unused, void *data,
>   			    uint64_t now __unused)
>   {
>   	struct pci_slot *slot = data;
> -	struct pci_device *pd = slot->pd;
> -	struct dt_node *dn = pd->dn;
>   	uint8_t link;
>   	struct phb *phb = slot->phb;
>   
> @@ -682,7 +691,7 @@ static void set_power_timer(struct timer *t __unused, void *data,
>   		if (slot->retries-- == 0) {
>   			pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
>   			opal_queue_msg(OPAL_MSG_ASYNC_COMP, NULL, NULL,
> -				       slot->async_token, dn->phandle,
> +				       slot->async_token, get_slot_phandle(slot),
>   				       slot->power_state, OPAL_BUSY);
>   		} else {
>   			schedule_timer(&slot->timer, msecs_to_tb(10));
> @@ -694,7 +703,7 @@ static void set_power_timer(struct timer *t __unused, void *data,
>   			remove_slot_devices(slot);
>   			pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
>   			opal_queue_msg(OPAL_MSG_ASYNC_COMP, NULL, NULL,
> -				       slot->async_token, dn->phandle,
> +				       slot->async_token, get_slot_phandle(slot),
>   				       OPAL_PCI_SLOT_POWER_OFF, OPAL_SUCCESS);
>   			break;
>   		}
> @@ -706,12 +715,12 @@ static void set_power_timer(struct timer *t __unused, void *data,
>   			rescan_slot_devices(slot);
>   			pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
>   			opal_queue_msg(OPAL_MSG_ASYNC_COMP, NULL, NULL,
> -				       slot->async_token, dn->phandle,
> +				       slot->async_token, get_slot_phandle(slot),
>   				       OPAL_PCI_SLOT_POWER_ON, OPAL_SUCCESS);
>   		} else if (slot->retries-- == 0) {
>   			pci_slot_set_state(slot, PCI_SLOT_STATE_NORMAL);
>   			opal_queue_msg(OPAL_MSG_ASYNC_COMP, NULL, NULL,
> -				       slot->async_token, dn->phandle,
> +				       slot->async_token, get_slot_phandle(slot),
>   				       OPAL_PCI_SLOT_POWER_ON, OPAL_BUSY);
>   		} else {
>   			schedule_timer(&slot->timer, msecs_to_tb(10));
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd at linux.ibm.com             IBM Australia Limited