[PATCH v2 1/5] powerpc/pci: Access PCI config space directly w/o pci_dn

Sergey Miroshnichenko s.miroshnichenko at yadro.com
Tue Sep 11 01:46:39 AEST 2018


Hello Sam,

On 9/10/18 7:23 AM, Sam Bobroff wrote:
> Hi Sergey,
> 
> On Thu, Sep 06, 2018 at 02:57:48PM +0300, Sergey Miroshnichenko wrote:
>> The pci_dn structures are retrieved from a DT, but hot-plugged PCIe
>> devices don't have them. Don't stop PCIe I/O in absence of pci_dn, so
>> it is now possible to discover new devices.
>>
>> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko at yadro.com>
>> ---
>>  arch/powerpc/kernel/rtas_pci.c       | 97 +++++++++++++++++++---------
>>  arch/powerpc/platforms/powernv/pci.c | 64 ++++++++++++------
>>  2 files changed, 109 insertions(+), 52 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
>> index c2b148b1634a..0611b46d9b5f 100644
>> --- a/arch/powerpc/kernel/rtas_pci.c
>> +++ b/arch/powerpc/kernel/rtas_pci.c
>> @@ -55,10 +55,26 @@ static inline int config_access_valid(struct pci_dn *dn, int where)
>>  	return 0;
>>  }
>>  
>> -int rtas_read_config(struct pci_dn *pdn, int where, int size, u32 *val)
>> +static int rtas_read_raw_config(unsigned long buid, int busno, unsigned int devfn,
>> +				int where, int size, u32 *val)
>>  {
>>  	int returnval = -1;
>> -	unsigned long buid, addr;
>> +	unsigned long addr = rtas_config_addr(busno, devfn, where);
>> +	int ret;
>> +
>> +	if (buid) {
>> +		ret = rtas_call(ibm_read_pci_config, 4, 2, &returnval,
>> +				addr, BUID_HI(buid), BUID_LO(buid), size);
>> +	} else {
>> +		ret = rtas_call(read_pci_config, 2, 2, &returnval, addr, size);
>> +	}
>> +	*val = returnval;
>> +
>> +	return ret;
>> +}
>> +
>> +int rtas_read_config(struct pci_dn *pdn, int where, int size, u32 *val)
>> +{
>>  	int ret;
>>  
>>  	if (!pdn)
>> @@ -71,16 +87,8 @@ int rtas_read_config(struct pci_dn *pdn, int where, int size, u32 *val)
>>  		return PCIBIOS_SET_FAILED;
>>  #endif
>>  
>> -	addr = rtas_config_addr(pdn->busno, pdn->devfn, where);
>> -	buid = pdn->phb->buid;
>> -	if (buid) {
>> -		ret = rtas_call(ibm_read_pci_config, 4, 2, &returnval,
>> -				addr, BUID_HI(buid), BUID_LO(buid), size);
>> -	} else {
>> -		ret = rtas_call(read_pci_config, 2, 2, &returnval, addr, size);
>> -	}
>> -	*val = returnval;
>> -
>> +	ret = rtas_read_raw_config(pdn->phb->buid, pdn->busno, pdn->devfn,
>> +				   where, size, val);
>>  	if (ret)
>>  		return PCIBIOS_DEVICE_NOT_FOUND;
>>  
>> @@ -98,18 +106,44 @@ static int rtas_pci_read_config(struct pci_bus *bus,
>>  
>>  	pdn = pci_get_pdn_by_devfn(bus, devfn);
>>  
>> -	/* Validity of pdn is checked in here */
>> -	ret = rtas_read_config(pdn, where, size, val);
>> -	if (*val == EEH_IO_ERROR_VALUE(size) &&
>> -	    eeh_dev_check_failure(pdn_to_eeh_dev(pdn)))
>> -		return PCIBIOS_DEVICE_NOT_FOUND;
>> +	if (pdn && eeh_enabled()) {
>> +		/* Validity of pdn is checked in here */
>> +		ret = rtas_read_config(pdn, where, size, val);
>> +
>> +		if (*val == EEH_IO_ERROR_VALUE(size) &&
>> +		    eeh_dev_check_failure(pdn_to_eeh_dev(pdn)))
>> +			ret = PCIBIOS_DEVICE_NOT_FOUND;
>> +	} else {
>> +		struct pci_controller *phb = pci_bus_to_host(bus);
>> +
>> +		ret = rtas_read_raw_config(phb->buid, bus->number, devfn,
>> +					   where, size, val);
>> +	}
> 
> In the above block, if pdn is valid but EEH isn't enabled,
> rtas_read_raw_config() will be used instead of rtas_read_config(), so
> config_access_valid() won't be tested. Is that correct?
> 

Thank you for the review!

This was the original intention, but now I can see that if a pdn is
valid, the EEH-branch should be taken even if EEH is disabled, as it was
before this patch; and functions there have checks for eeh_enabled()
inside. I'll fix that in v3 as follows:

-	if (pdn && eeh_enabled()) {
+	if (pdn) {

>>  
>>  	return ret;
>>  }
>>  
>> +static int rtas_write_raw_config(unsigned long buid, int busno, unsigned int devfn,
>> +				 int where, int size, u32 val)
>> +{
>> +	unsigned long addr = rtas_config_addr(busno, devfn, where);
>> +	int ret;
>> +
>> +	if (buid) {
>> +		ret = rtas_call(ibm_write_pci_config, 5, 1, NULL, addr,
>> +				BUID_HI(buid), BUID_LO(buid), size, (ulong)val);
>> +	} else {
>> +		ret = rtas_call(write_pci_config, 3, 1, NULL, addr, size, (ulong)val);
>> +	}
>> +
>> +	if (ret)
>> +		return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>>  int rtas_write_config(struct pci_dn *pdn, int where, int size, u32 val)
>>  {
>> -	unsigned long buid, addr;
>>  	int ret;
>>  
>>  	if (!pdn)
>> @@ -122,15 +156,8 @@ int rtas_write_config(struct pci_dn *pdn, int where, int size, u32 val)
>>  		return PCIBIOS_SET_FAILED;
>>  #endif
>>  
>> -	addr = rtas_config_addr(pdn->busno, pdn->devfn, where);
>> -	buid = pdn->phb->buid;
>> -	if (buid) {
>> -		ret = rtas_call(ibm_write_pci_config, 5, 1, NULL, addr,
>> -			BUID_HI(buid), BUID_LO(buid), size, (ulong) val);
>> -	} else {
>> -		ret = rtas_call(write_pci_config, 3, 1, NULL, addr, size, (ulong)val);
>> -	}
>> -
>> +	ret = rtas_write_raw_config(pdn->phb->buid, pdn->busno, pdn->devfn,
>> +				    where, size, val);
>>  	if (ret)
>>  		return PCIBIOS_DEVICE_NOT_FOUND;
>>  
>> @@ -141,12 +168,20 @@ static int rtas_pci_write_config(struct pci_bus *bus,
>>  				 unsigned int devfn,
>>  				 int where, int size, u32 val)
>>  {
>> -	struct pci_dn *pdn;
>> +	struct pci_dn *pdn = pci_get_pdn_by_devfn(bus, devfn);
>> +	int ret;
>>  
>> -	pdn = pci_get_pdn_by_devfn(bus, devfn);
>> +	if (pdn && eeh_enabled()) {
>> +		/* Validity of pdn is checked in here. */
>> +		ret = rtas_write_config(pdn, where, size, val);
>> +	} else {
>> +		struct pci_controller *phb = pci_bus_to_host(bus);
> 
> Same comment as rtas_pci_read_config() above.
> 

I'll fix that symmetrically.

>>  
>> -	/* Validity of pdn is checked in here. */
>> -	return rtas_write_config(pdn, where, size, val);
>> +		ret = rtas_write_raw_config(phb->buid, bus->number, devfn,
>> +					    where, size, val);
>> +	}
>> +
>> +	return ret;
>>  }
>>  
>>  static struct pci_ops rtas_pci_ops = {
>> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
>> index 13aef2323bbc..3f87a2dc6578 100644
>> --- a/arch/powerpc/platforms/powernv/pci.c
>> +++ b/arch/powerpc/platforms/powernv/pci.c
>> @@ -654,30 +654,29 @@ static void pnv_pci_config_check_eeh(struct pci_dn *pdn)
>>  	}
>>  }
>>  
>> -int pnv_pci_cfg_read(struct pci_dn *pdn,
>> -		     int where, int size, u32 *val)
>> +int pnv_pci_cfg_read_raw(u64 phb_id, int busno, unsigned int devfn,
>> +			 int where, int size, u32 *val)
>>  {
>> -	struct pnv_phb *phb = pdn->phb->private_data;
>> -	u32 bdfn = (pdn->busno << 8) | pdn->devfn;
>> +	u32 bdfn = (busno << 8) | devfn;
>>  	s64 rc;
>>  
>>  	switch (size) {
>>  	case 1: {
>>  		u8 v8;
>> -		rc = opal_pci_config_read_byte(phb->opal_id, bdfn, where, &v8);
>> +		rc = opal_pci_config_read_byte(phb_id, bdfn, where, &v8);
>>  		*val = (rc == OPAL_SUCCESS) ? v8 : 0xff;
>>  		break;
>>  	}
>>  	case 2: {
>>  		__be16 v16;
>> -		rc = opal_pci_config_read_half_word(phb->opal_id, bdfn, where,
>> -						   &v16);
>> +		rc = opal_pci_config_read_half_word(phb_id, bdfn, where,
>> +						    &v16);
>>  		*val = (rc == OPAL_SUCCESS) ? be16_to_cpu(v16) : 0xffff;
>>  		break;
>>  	}
>>  	case 4: {
>>  		__be32 v32;
>> -		rc = opal_pci_config_read_word(phb->opal_id, bdfn, where, &v32);
>> +		rc = opal_pci_config_read_word(phb_id, bdfn, where, &v32);
>>  		*val = (rc == OPAL_SUCCESS) ? be32_to_cpu(v32) : 0xffffffff;
>>  		break;
>>  	}
>> @@ -686,27 +685,28 @@ int pnv_pci_cfg_read(struct pci_dn *pdn,
>>  	}
>>  
>>  	pr_devel("%s: bus: %x devfn: %x +%x/%x -> %08x\n",
>> -		 __func__, pdn->busno, pdn->devfn, where, size, *val);
>> +		 __func__, busno, devfn, where, size, *val);
>> +
>>  	return PCIBIOS_SUCCESSFUL;
>>  }
>>  
>> -int pnv_pci_cfg_write(struct pci_dn *pdn,
>> -		      int where, int size, u32 val)
>> +int pnv_pci_cfg_write_raw(u64 phb_id, int busno, unsigned int devfn,
>> +			  int where, int size, u32 val)
>>  {
>> -	struct pnv_phb *phb = pdn->phb->private_data;
>> -	u32 bdfn = (pdn->busno << 8) | pdn->devfn;
>> +	u32 bdfn = (busno << 8) | devfn;
>>  
>>  	pr_devel("%s: bus: %x devfn: %x +%x/%x -> %08x\n",
>> -		 __func__, pdn->busno, pdn->devfn, where, size, val);
>> +		 __func__, busno, devfn, where, size, val);
>> +
>>  	switch (size) {
>>  	case 1:
>> -		opal_pci_config_write_byte(phb->opal_id, bdfn, where, val);
>> +		opal_pci_config_write_byte(phb_id, bdfn, where, val);
>>  		break;
>>  	case 2:
>> -		opal_pci_config_write_half_word(phb->opal_id, bdfn, where, val);
>> +		opal_pci_config_write_half_word(phb_id, bdfn, where, val);
>>  		break;
>>  	case 4:
>> -		opal_pci_config_write_word(phb->opal_id, bdfn, where, val);
>> +		opal_pci_config_write_word(phb_id, bdfn, where, val);
>>  		break;
>>  	default:
>>  		return PCIBIOS_FUNC_NOT_SUPPORTED;
>> @@ -715,6 +715,24 @@ int pnv_pci_cfg_write(struct pci_dn *pdn,
>>  	return PCIBIOS_SUCCESSFUL;
>>  }
>>  
>> +int pnv_pci_cfg_read(struct pci_dn *pdn,
>> +		     int where, int size, u32 *val)
>> +{
>> +	struct pnv_phb *phb = pdn->phb->private_data;
>> +
>> +	return pnv_pci_cfg_read_raw(phb->opal_id, pdn->busno, pdn->devfn,
>> +				    where, size, val);
>> +}
>> +
>> +int pnv_pci_cfg_write(struct pci_dn *pdn,
>> +		      int where, int size, u32 val)
>> +{
>> +	struct pnv_phb *phb = pdn->phb->private_data;
>> +
>> +	return pnv_pci_cfg_write_raw(phb->opal_id, pdn->busno, pdn->devfn,
>> +				     where, size, val);
>> +}
>> +
>>  #if CONFIG_EEH
>>  static bool pnv_pci_cfg_check(struct pci_dn *pdn)
>>  {
>> @@ -750,13 +768,15 @@ static int pnv_pci_read_config(struct pci_bus *bus,
>>  			       int where, int size, u32 *val)
>>  {
>>  	struct pci_dn *pdn;
>> -	struct pnv_phb *phb;
>> +	struct pci_controller *hose = pci_bus_to_host(bus);
>> +	struct pnv_phb *phb = hose->private_data;
>>  	int ret;
>>  
>>  	*val = 0xFFFFFFFF;
>>  	pdn = pci_get_pdn_by_devfn(bus, devfn);
>>  	if (!pdn)
>> -		return PCIBIOS_DEVICE_NOT_FOUND;
>> +		return pnv_pci_cfg_read_raw(phb->opal_id, bus->number, devfn,
>> +					    where, size, val);
>>  
>>  	if (!pnv_pci_cfg_check(pdn))
>>  		return PCIBIOS_DEVICE_NOT_FOUND;
>> @@ -779,12 +799,14 @@ static int pnv_pci_write_config(struct pci_bus *bus,
>>  				int where, int size, u32 val)
>>  {
>>  	struct pci_dn *pdn;
>> -	struct pnv_phb *phb;
>> +	struct pci_controller *hose = pci_bus_to_host(bus);
>> +	struct pnv_phb *phb = hose->private_data;
>>  	int ret;
>>  
>>  	pdn = pci_get_pdn_by_devfn(bus, devfn);
>>  	if (!pdn)
>> -		return PCIBIOS_DEVICE_NOT_FOUND;
>> +		return pnv_pci_cfg_write_raw(phb->opal_id, bus->number, devfn,
>> +					     where, size, val);
>>  
>>  	if (!pnv_pci_cfg_check(pdn))
>>  		return PCIBIOS_DEVICE_NOT_FOUND;
>> -- 
>> 2.17.1
>>

Best regards,
Serge

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20180910/749fd4ce/attachment.sig>


More information about the Linuxppc-dev mailing list