[Skiboot] [PATCH 04/16] core/pci: Train link of PHB slots when hotplugging

[Alexey Kardashevskiy]

On 25/09/2019 21:57, Frederic Barrat wrote:
> 
> 
> Le 25/09/2019 à 07:14, Alexey Kardashevskiy a écrit :
>>
>>
>> On 09/09/2019 22:31, Frederic Barrat wrote:
>>> The link of PHB slots must be trained after powering on. This can be
>>> done by calling the fundamental reset callback of the slot.
>>>
>>> We could force a reset for all the slots and have a common path in
>>> set_power_state(). But this patch only resets the PHB slot. Some slot
>>> implementations do a power cycle during fundamental reset, so calling
>>> a reset after powering on would repeat that operation.
>>>
>>> Signed-off-by: Frederic Barrat <fbarrat at linux.ibm.com>
>>> ---
>>>   core/pci-opal.c | 130 ++++++++++++++++++++++++++++++++++++++++--------
>>>   1 file changed, 108 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/core/pci-opal.c b/core/pci-opal.c
>>> index 69bd65c6..6c070030 100644
>>> --- a/core/pci-opal.c
>>> +++ b/core/pci-opal.c
>>> @@ -678,11 +678,111 @@ static void remove_slot_devices(struct pci_slot *slot)
>>>       pci_remove_bus(phb, &pd->children);
>>>   }
>>>   +static void link_training_timer(struct timer *t, void *data,
>>> +                uint64_t now __unused)
>>> +{
>>> +    struct pci_slot *slot = data;
>>> +    struct phb *phb = slot->phb;
>>> +    uint8_t link;
>>> +    int64_t rc = 0;
>>> +
>>> +    phb_lock(phb);
>>> +
>>> +    rc = slot->ops.run_sm(slot);
>>> +    if (rc < 0)
>>> +        goto out;
>>> +    if (rc > 0) {
>>> +        schedule_timer(t, rc);
>>> +        phb_unlock(phb);
>>> +        return;
>>> +    }
>>> +
>>> +    if (slot->ops.get_link_state(slot, &link) != OPAL_SUCCESS)
>>> +        link = 0;
>>> +    if (!link) {
>>> +        rc = OPAL_HARDWARE;
>>> +        goto out;
>>> +    }
>>> +
>>> +    rescan_slot_devices(slot);
>>> +out:
>>> +    opal_queue_msg(OPAL_MSG_ASYNC_COMP, NULL, NULL,
>>> +               slot->async_token, get_slot_phandle(slot),
>>> +               slot->power_state, rc <= 0 ? rc : OPAL_BUSY);
>>> +    phb_unlock(phb);
>>> +}
>>> +
>>> +static void link_up_timer(struct timer *t, void *data, uint64_t now __unused)
>>> +{
>>> +    struct pci_slot *slot = data;
>>> +    struct phb *phb = slot->phb;
>>> +    uint8_t link;
>>> +    int64_t rc = OPAL_SUCCESS;
>>> +
>>> +    phb_lock(phb);
>>> +    if (slot->ops.get_link_state(slot, &link) != OPAL_SUCCESS)
>>> +        link = 0;
>>> +    if (!link) {
>>> +        if (slot->retries-- == 0) {
>>> +            rc = OPAL_BUSY;
>>> +            goto out;
>>> +        } else {
>>> +            schedule_timer(t, msecs_to_tb(10));
>>> +            phb_unlock(phb);
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    rescan_slot_devices(slot);
>>> +out:
>>> +    opal_queue_msg(OPAL_MSG_ASYNC_COMP, NULL, NULL,
>>> +               slot->async_token, get_slot_phandle(slot),
>>> +               slot->power_state, rc <= 0 ? rc : OPAL_BUSY);
>>> +    phb_unlock(phb);
>>> +}
>>> +
>>> +static bool training_needed(struct pci_slot *slot)
>>> +{
>>> +    struct phb *phb = slot->phb;
>>> +    struct pci_device *pd = slot->pd;
>>> +
>>> +    if ((pd && !pd->parent) || /* "real" PHB */
>>> +        (!pd && phb->phb_type == phb_type_npu_v2_opencapi))
>>
>>
>>
>> Having several checks for opencapi in otherwise opencapi-agnostic file says that opencapi phb/slot is not modeled
>> correctly.
> 
> Hi Alexey,
> 
> In that file, there are 2 things we do differently for opencapi:
> 1. determine if the link needs to be retrained or not. Note that in the training_needed() function here, the "real" PHB
> case is a bit of a stretch, I don't think it's needed yet, but I believe Oliver has something cooking related to PHB
> hotplug. I should probably remove it for now though.
> 2. populate the correct list of devices when rescanning/removing a slot.
> 
> Both shouldn't be related to opencapi per se, but they could also apply to a "real" PHB, I guess. So we could have a
> marker on a PHB slot and branch off that, it would be more general, but I'm not sure how that would/could be used with
> real PHBs. Is that more or less what you had in mind?

Not exactly, I did not get that far :)

The commit log says it can use a common path but some slots do something sometime and this is where I got lost. I
expected a slot-specific pci_slot_ops::set_link_state/set_power_state rather than branching in training_needed(). But
you can ignore me if Oliver is happy, he knows this code way better :)



> 
>   Fred
> 
> 
> 
>> btw I remember asking if there is a machine I can use to get myself familiar with opencapi and lost the response, can
>> you please remind what was it (if this was you :) )? I'll just ssh to it and do lspci/sysfs kinda thing. Thanks,
>>
>>
>>
> 

-- 
Alexey