Re: [PATCH v8 36/45] powerpc/powernv: Support PCI slot ID

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 04/20/2016 12:28 PM, Gavin Shan wrote:
> On Tue, Apr 19, 2016 at 07:28:20PM +1000, Alexey Kardashevskiy wrote:
>> On 02/17/2016 02:44 PM, Gavin Shan wrote:
>>> PowerNV platforms runs on top of skiboot firmware that includes
>>> changes to support PCI slots. PCI slots are identified by PHB's
>>> ID or the combo of that and PCI slot ID.
>>>
>>> This changes the EEH PowerNV backend to support PCI slots:
>>>
>>>     * Rename arguments of opal_pci_reset() and opal_pci_poll().
>>>     * One more argument (PCI slot's state) added to opal_pci_poll().
>>>     * Drop pnv_eeh_phb_poll() and introduce a enhanced similar
>>>       function pnv_pci_poll() that will be used by PowerNV hotplug
>>>       backends.
>>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>>   arch/powerpc/include/asm/opal.h              |  4 +--
>>>   arch/powerpc/platforms/powernv/eeh-powernv.c | 42 ++++++----------------------
>>>   arch/powerpc/platforms/powernv/pci.c         | 21 ++++++++++++++
>>>   arch/powerpc/platforms/powernv/pci.h         |  1 +
>>>   4 files changed, 32 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
>>> index 07a99e6..9e0039f 100644
>>> --- a/arch/powerpc/include/asm/opal.h
>>> +++ b/arch/powerpc/include/asm/opal.h
>>> @@ -131,7 +131,7 @@ int64_t opal_pci_map_pe_dma_window(uint64_t phb_id, uint16_t pe_number, uint16_t
>>>   int64_t opal_pci_map_pe_dma_window_real(uint64_t phb_id, uint16_t pe_number,
>>>   					uint16_t dma_window_number, uint64_t pci_start_addr,
>>>   					uint64_t pci_mem_size);
>>> -int64_t opal_pci_reset(uint64_t phb_id, uint8_t reset_scope, uint8_t assert_state);
>>> +int64_t opal_pci_reset(uint64_t id, uint8_t reset_scope, uint8_t assert_state);
>>>
>>>   int64_t opal_pci_get_hub_diag_data(uint64_t hub_id, void *diag_buffer,
>>>   				   uint64_t diag_buffer_len);
>>> @@ -148,7 +148,7 @@ int64_t opal_get_dpo_status(__be64 *dpo_timeout);
>>>   int64_t opal_set_system_attention_led(uint8_t led_action);
>>>   int64_t opal_pci_next_error(uint64_t phb_id, __be64 *first_frozen_pe,
>>>   			    __be16 *pci_error_type, __be16 *severity);
>>> -int64_t opal_pci_poll(uint64_t phb_id);
>>> +int64_t opal_pci_poll(uint64_t id, uint8_t *state);
>>>   int64_t opal_return_cpu(void);
>>>   int64_t opal_check_token(uint64_t token);
>>>   int64_t opal_reinit_cpus(uint64_t flags);
>>> diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> index c7454ba..e23b063 100644
>>> --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>> @@ -717,28 +717,11 @@ static int pnv_eeh_get_state(struct eeh_pe *pe, int *delay)
>>>   	return ret;
>>>   }
>>>
>>> -static s64 pnv_eeh_phb_poll(struct pnv_phb *phb)
>>> -{
>>> -	s64 rc = OPAL_HARDWARE;
>>> -
>>> -	while (1) {
>>> -		rc = opal_pci_poll(phb->opal_id);
>>> -		if (rc <= 0)
>>> -			break;
>>> -
>>> -		if (system_state < SYSTEM_RUNNING)
>>> -			udelay(1000 * rc);
>>> -		else
>>> -			msleep(rc);
>>> -	}
>>> -
>>> -	return rc;
>>> -}
>>> -
>>>   int pnv_eeh_phb_reset(struct pci_controller *hose, int option)
>>>   {
>>>   	struct pnv_phb *phb = hose->private_data;
>>>   	s64 rc = OPAL_HARDWARE;
>>> +	int ret;
>>>
>>>   	pr_debug("%s: Reset PHB#%x, option=%d\n",
>>>   		 __func__, hose->global_number, option);
>>> @@ -753,8 +736,6 @@ int pnv_eeh_phb_reset(struct pci_controller *hose, int option)
>>>   		rc = opal_pci_reset(phb->opal_id,
>>>   				    OPAL_RESET_PHB_COMPLETE,
>>>   				    OPAL_DEASSERT_RESET);
>>> -	if (rc < 0)
>>> -		goto out;
>>>
>>>   	/*
>>>   	 * Poll state of the PHB until the request is done
>>> @@ -762,24 +743,22 @@ int pnv_eeh_phb_reset(struct pci_controller *hose, int option)
>>>   	 * reset followed by hot reset on root bus. So we also
>>>   	 * need the PCI bus settlement delay.
>>>   	 */
>>> -	rc = pnv_eeh_phb_poll(phb);
>>> -	if (option == EEH_RESET_DEACTIVATE) {
>>> +	ret = pnv_pci_poll(phb->opal_id, rc, NULL);
>>> +	if (option == EEH_RESET_DEACTIVATE && !ret) {
>>>   		if (system_state < SYSTEM_RUNNING)
>>>   			udelay(1000 * EEH_PE_RST_SETTLE_TIME);
>>>   		else
>>>   			msleep(EEH_PE_RST_SETTLE_TIME);
>>>   	}
>>> -out:
>>> -	if (rc != OPAL_SUCCESS)
>>> -		return -EIO;
>>>
>>> -	return 0;
>>> +	return ret;
>>>   }
>>>
>>>   static int pnv_eeh_root_reset(struct pci_controller *hose, int option)
>>>   {
>>>   	struct pnv_phb *phb = hose->private_data;
>>>   	s64 rc = OPAL_HARDWARE;
>>> +	int ret;
>>>
>>>   	pr_debug("%s: Reset PHB#%x, option=%d\n",
>>>   		 __func__, hose->global_number, option);
>>> @@ -801,18 +780,13 @@ static int pnv_eeh_root_reset(struct pci_controller *hose, int option)
>>>   		rc = opal_pci_reset(phb->opal_id,
>>>   				    OPAL_RESET_PCI_HOT,
>>>   				    OPAL_DEASSERT_RESET);
>>> -	if (rc < 0)
>>> -		goto out;
>>>
>>>   	/* Poll state of the PHB until the request is done */
>>> -	rc = pnv_eeh_phb_poll(phb);
>>> -	if (option == EEH_RESET_DEACTIVATE)
>>> +	ret = pnv_pci_poll(phb->opal_id, rc, NULL);
>>> +	if (option == EEH_RESET_DEACTIVATE && !ret)
>>>   		msleep(EEH_PE_RST_SETTLE_TIME);
>>> -out:
>>> -	if (rc != OPAL_SUCCESS)
>>> -		return -EIO;
>>>
>>> -	return 0;
>>> +	return ret;
>>>   }
>>>
>>>   static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option)
>>> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
>>> index b87a315..a458703 100644
>>> --- a/arch/powerpc/platforms/powernv/pci.c
>>> +++ b/arch/powerpc/platforms/powernv/pci.c
>>> @@ -42,6 +42,27 @@
>>>   #define cfg_dbg(fmt...)	do { } while(0)
>>>   //#define cfg_dbg(fmt...)	printk(fmt)
>>>
>>> +int pnv_pci_poll(uint64_t id, int64_t rval, uint8_t *state)
>>> +{
>>> +	while (rval > 0) {
>>> +		if (system_state < SYSTEM_RUNNING)
>>> +			udelay(1000 * rval);
>>> +		else
>>> +			msleep(rval);
>>> +
>>> +		rval = opal_pci_poll(id, state);
>>> +	}
>>> +
>>> +	/*
>>> +	 * The caller expects to retrieve additional
>>> +	 * information if the last argument isn't NULL.
>>> +	 */
>>> +	if (rval == OPAL_SUCCESS && state)
>>> +		rval = opal_pci_poll(id, state);
>>
>>
>> Old OPAL won't touch @state so whatever garbage was there will stay there as
>> the only caller which is passing not-NULL there is pnv_php_get_power_state()
>> and it does not initialize @power_state (it is in "[PATCH v8 45/45]
>> PCI/hotplug: PowerPC PowerNV PCI hotplug driver").
>>
>
> Old OPAL without exposing hotpluggable slots won't have this case. I mean
> pnv_php_get_power_state() won't be called on old OPAL.


What exactly does guarantee that hotplug will work with new OPAL only?

>
>>
>> btw how will new OPAL react if old kernel is running, i.e. not passing @state
>> at all? If it is initialized to NULL somewher - fine but what exactly does
>> this initialization and makes sure that OPAL won't see garbage as a second
>> parameter?
>>
>
> @state is always NULL for old kernel + new OPAL.

What piece of code writes NULL to "state"? Old kernel will do 
opal_pci_poll(id) and omit the "state" so something has to take care of it 
and write NULL where OPAL expects to see a pointer to "state" (which we set 
to NULL) and that place would be some GPR which may have garbage.

I am looking at
#define OPAL_CALL(name, token)

and cannot see where the second parameter (which old kernel omits) is 
reset, i.e. gpr4 (or gpr5?) is cleared.


> @state is used in
> PCI hotplug functionality in OPAL only. As old kernel doesn't support
> PCI hotplug, @state is never used. I'm not sure it's the answer you
> want?

No, it is not :)

>> When ABI like this changes, I expect to see opal_pci_poll2() or
>> opal_pci_poll_ex() rather than just an additional parameter to
>> opal_pci_poll()...
>>
>
> It's a good suggestion but it would be nicer if you raised this
> early. One question I have: current opal_pci_poll() is enough
> to cover all cases, why we need introduce and maintain another
> similar one? Sorry that I don't see the reason from your context
> and could you please provide more details?
>
>>> +
>>> +	return (rval == OPAL_SUCCESS) ? 0 : -EIO;
>>> +}
>>> +
>>>   #ifdef CONFIG_PCI_MSI
>>>   int pnv_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
>>>   {
>>> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
>>> index 0cddde3..6857703 100644
>>> --- a/arch/powerpc/platforms/powernv/pci.h
>>> +++ b/arch/powerpc/platforms/powernv/pci.h
>>> @@ -192,6 +192,7 @@ extern int pnv_tce_xchg(struct iommu_table *tbl, long index,
>>>   		unsigned long *hpa, enum dma_data_direction *direction);
>>>   extern unsigned long pnv_tce_get(struct iommu_table *tbl, long index);
>>>
>>> +int pnv_pci_poll(uint64_t id, int64_t rval, uint8_t *state);
>>>   void pnv_pci_dump_phb_diag_data(struct pci_controller *hose,
>>>   				unsigned char *log_buff);
>>>   int pnv_pci_cfg_read(struct pci_dn *pdn,
>>>
>>
>>
>> --
>> Alexey
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Alexey