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

[Frederic Barrat]

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?

   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,
> 
> 
>