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

2018-09-10 Thread Sergey Miroshnichenko 
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 
>> ---
>>  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, ,
>> +addr, BUID_HI(buid), BUID_LO(buid), size);
>> +} else {
>> +ret = rtas_call(read_pci_config, 2, 2, , 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, ,
>> -addr, BUID_HI(buid), BUID_LO(buid), size);
>> -} else {
>> -ret = rtas_call(read_pci_config, 2, 2, , 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 =

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

2018-09-09 Thread Sam Bobroff 
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 
> ---
>  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, ,
> + addr, BUID_HI(buid), BUID_LO(buid), size);
> + } else {
> + ret = rtas_call(read_pci_config, 2, 2, , 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, ,
> - addr, BUID_HI(buid), BUID_LO(buid), size);
> - } else {
> - ret = rtas_call(read_pci_config, 2, 2, , 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?

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

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

2018-09-06 Thread Sergey Miroshnichenko 
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 
---
 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, ,
+   addr, BUID_HI(buid), BUID_LO(buid), size);
+   } else {
+   ret = rtas_call(read_pci_config, 2, 2, , 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, ,
-   addr, BUID_HI(buid), BUID_LO(buid), size);
-   } else {
-   ret = rtas_call(read_pci_config, 2, 2, , 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);
+   }
 
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);
+