[RFC,1/6] powerpc:/drc Define interface to acquire arch-specific drc info
diff mbox series

Message ID 20181214204957.16435.29255.stgit@powerkvm6.aus.stglabs.ibm.com
State Changes Requested
Headers show
Series
  • powerpc/pseries: Refactor code to centralize drcinfo parsing
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch warning total: 0 errors, 0 warnings, 1 checks, 15 lines checked
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Michael Bringmann Dec. 14, 2018, 8:50 p.m. UTC
Define interface to acquire arch-specific drc info to match against
hotpluggable devices.  The current implementation exposes several
pseries-specific dynamic memory properties in generic kernel code.
This patch set provides an interface to pull that code out of the
generic kernel.

Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
---
 include/linux/topology.h |    9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Tyrel Datwyler Jan. 25, 2019, 12:04 a.m. UTC | #1
On 12/14/2018 12:50 PM, Michael Bringmann wrote:
> Define interface to acquire arch-specific drc info to match against
> hotpluggable devices.  The current implementation exposes several
> pseries-specific dynamic memory properties in generic kernel code.
> This patch set provides an interface to pull that code out of the
> generic kernel.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
>  include/linux/topology.h |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index cb0775e..df97f5f 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -44,6 +44,15 @@

As far as I know pseries is the only platform that uses DR connectors, and I
highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
that this is really generic enough to belong in topology.h. If anything I would
suggest putting this in an include in arch/powerpc/include/ named something like
drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
that want/need to use this functionality.

-Tyrel

>  
>  int arch_update_cpu_topology(void);
>  
> +int arch_find_drc_match(struct device_node *dn,
> +			bool (*usercb)(struct device_node *dn,
> +				u32 drc_index, char *drc_name,
> +				char *drc_type, u32 drc_power_domain,
> +				void *data),
> +			char *opt_drc_type, char *opt_drc_name,
> +			bool match_drc_index, bool ck_php_type,
> +			void *data);
> +
>  /* Conform to ACPI 2.0 SLIT distance definitions */
>  #define LOCAL_DISTANCE		10
>  #define REMOTE_DISTANCE		20
>
Tyrel Datwyler Jan. 25, 2019, 12:10 a.m. UTC | #2
On 12/14/2018 12:50 PM, Michael Bringmann wrote:
> Define interface to acquire arch-specific drc info to match against
> hotpluggable devices.  The current implementation exposes several
> pseries-specific dynamic memory properties in generic kernel code.
> This patch set provides an interface to pull that code out of the
> generic kernel.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
>  include/linux/topology.h |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index cb0775e..df97f5f 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -44,6 +44,15 @@
>  
>  int arch_update_cpu_topology(void);

On another note a kern doc comment for this function would also be nice.

-Tyrel

>  
> +int arch_find_drc_match(struct device_node *dn,
> +			bool (*usercb)(struct device_node *dn,
> +				u32 drc_index, char *drc_name,
> +				char *drc_type, u32 drc_power_domain,
> +				void *data),
> +			char *opt_drc_type, char *opt_drc_name,
> +			bool match_drc_index, bool ck_php_type,
> +			void *data);
> +
>  /* Conform to ACPI 2.0 SLIT distance definitions */
>  #define LOCAL_DISTANCE		10
>  #define REMOTE_DISTANCE		20
>
Michael Bringmann Jan. 25, 2019, 4:09 p.m. UTC | #3
Adding Nathan Lynch

On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>> Define interface to acquire arch-specific drc info to match against
>> hotpluggable devices.  The current implementation exposes several
>> pseries-specific dynamic memory properties in generic kernel code.
>> This patch set provides an interface to pull that code out of the
>> generic kernel.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>>  include/linux/topology.h |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e..df97f5f 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -44,6 +44,15 @@
> 
> As far as I know pseries is the only platform that uses DR connectors, and I
> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
> that this is really generic enough to belong in topology.h. If anything I would
> suggest putting this in an include in arch/powerpc/include/ named something like
> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
> that want/need to use this functionality.
> 
> -Tyrel
> 
>>  
>>  int arch_update_cpu_topology(void);
>>  
>> +int arch_find_drc_match(struct device_node *dn,
>> +			bool (*usercb)(struct device_node *dn,
>> +				u32 drc_index, char *drc_name,
>> +				char *drc_type, u32 drc_power_domain,
>> +				void *data),
>> +			char *opt_drc_type, char *opt_drc_name,
>> +			bool match_drc_index, bool ck_php_type,
>> +			void *data);
>> +
>>  /* Conform to ACPI 2.0 SLIT distance definitions */
>>  #define LOCAL_DISTANCE		10
>>  #define REMOTE_DISTANCE		20
>>
> 
>
Michael Bringmann Jan. 25, 2019, 4:11 p.m. UTC | #4
Adding Nathan Lynch.

On 1/24/19 6:10 PM, Tyrel Datwyler wrote:
> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>> Define interface to acquire arch-specific drc info to match against
>> hotpluggable devices.  The current implementation exposes several
>> pseries-specific dynamic memory properties in generic kernel code.
>> This patch set provides an interface to pull that code out of the
>> generic kernel.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>>  include/linux/topology.h |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e..df97f5f 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -44,6 +44,15 @@
>>  
>>  int arch_update_cpu_topology(void);
> 
> On another note a kern doc comment for this function would also be nice.
> 
> -Tyrel
> 
>>  
>> +int arch_find_drc_match(struct device_node *dn,
>> +			bool (*usercb)(struct device_node *dn,
>> +				u32 drc_index, char *drc_name,
>> +				char *drc_type, u32 drc_power_domain,
>> +				void *data),
>> +			char *opt_drc_type, char *opt_drc_name,
>> +			bool match_drc_index, bool ck_php_type,
>> +			void *data);
>> +
>>  /* Conform to ACPI 2.0 SLIT distance definitions */
>>  #define LOCAL_DISTANCE		10
>>  #define REMOTE_DISTANCE		20
>>
> 
>
Michael Bringmann Jan. 28, 2019, 6:23 p.m. UTC | #5
On 1/25/19 10:09 AM, Michael Bringmann wrote:
> Adding Nathan Lynch
> 
> On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
>> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>>> Define interface to acquire arch-specific drc info to match against
>>> hotpluggable devices.  The current implementation exposes several
>>> pseries-specific dynamic memory properties in generic kernel code.
>>> This patch set provides an interface to pull that code out of the
>>> generic kernel.
>>>
>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>> ---
>>>  include/linux/topology.h |    9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>>> index cb0775e..df97f5f 100644
>>> --- a/include/linux/topology.h
>>> +++ b/include/linux/topology.h
>>> @@ -44,6 +44,15 @@
>>
>> As far as I know pseries is the only platform that uses DR connectors, and I
>> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
>> that this is really generic enough to belong in topology.h. If anything I would
>> suggest putting this in an include in arch/powerpc/include/ named something like
>> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
>> that want/need to use this functionality.

It looks like the 'rpaphp' and 'rpadlpar_io' modules are also dependent upon the
powerpc platform.  Shouldn't the relevant source files be moved completely to the
powerpc-specific directories out of drivers/pci/hotplug as well?

drivers/pci/hotplug/Kconfig has:

config HOTPLUG_PCI_RPA
        tristate "RPA PCI Hotplug driver"
        depends on PPC_PSERIES && EEH
        help
          Say Y here if you have a RPA system that supports PCI Hotplug.

          To compile this driver as a module, choose M here: the
          module will be called rpaphp.

          When in doubt, say N.

config HOTPLUG_PCI_RPA_DLPAR
        tristate "RPA Dynamic Logical Partitioning for I/O slots"
        depends on HOTPLUG_PCI_RPA
        help
          Say Y here if your system supports Dynamic Logical Partitioning
          for I/O slots.

          To compile this driver as a module, choose M here: the
          module will be called rpadlpar_io.

          When in doubt, say N.

Michael

>>
>> -Tyrel
>>
>>>  
>>>  int arch_update_cpu_topology(void);
>>>  
>>> +int arch_find_drc_match(struct device_node *dn,
>>> +			bool (*usercb)(struct device_node *dn,
>>> +				u32 drc_index, char *drc_name,
>>> +				char *drc_type, u32 drc_power_domain,
>>> +				void *data),
>>> +			char *opt_drc_type, char *opt_drc_name,
>>> +			bool match_drc_index, bool ck_php_type,
>>> +			void *data);
>>> +
>>>  /* Conform to ACPI 2.0 SLIT distance definitions */
>>>  #define LOCAL_DISTANCE		10
>>>  #define REMOTE_DISTANCE		20
>>>
>>
>>
>
Michael Ellerman Jan. 29, 2019, 9:25 a.m. UTC | #6
Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
> On 1/25/19 10:09 AM, Michael Bringmann wrote:
>> Adding Nathan Lynch
>> 
>> On 1/24/19 6:04 PM, Tyrel Datwyler wrote:
>>> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>>>> Define interface to acquire arch-specific drc info to match against
>>>> hotpluggable devices.  The current implementation exposes several
>>>> pseries-specific dynamic memory properties in generic kernel code.
>>>> This patch set provides an interface to pull that code out of the
>>>> generic kernel.
>>>>
>>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>>> ---
>>>>  include/linux/topology.h |    9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>>>> index cb0775e..df97f5f 100644
>>>> --- a/include/linux/topology.h
>>>> +++ b/include/linux/topology.h
>>>> @@ -44,6 +44,15 @@
>>>
>>> As far as I know pseries is the only platform that uses DR connectors, and I
>>> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
>>> that this is really generic enough to belong in topology.h. If anything I would
>>> suggest putting this in an include in arch/powerpc/include/ named something like
>>> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
>>> that want/need to use this functionality.
>
> It looks like the 'rpaphp' and 'rpadlpar_io' modules are also dependent upon the
> powerpc platform.

Yes that's right.

> Shouldn't the relevant source files be moved completely to the
> powerpc-specific directories out of drivers/pci/hotplug as well?

I don't think so. They are PCI hotplug drivers, so they should sit with
the other PCI hotplug drivers.

It's true that PCI hotplug drivers are more platform specific than other
types of drivers, but still they have some things in common with other
PCI hotplug drivers.

cheers
Michael Ellerman Jan. 29, 2019, 9:31 a.m. UTC | #7
Tyrel Datwyler <turtle.in.the.kernel@gmail.com> writes:
> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>> Define interface to acquire arch-specific drc info to match against
>> hotpluggable devices.  The current implementation exposes several
>> pseries-specific dynamic memory properties in generic kernel code.
>> This patch set provides an interface to pull that code out of the
>> generic kernel.
>> 
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>>  include/linux/topology.h |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e..df97f5f 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -44,6 +44,15 @@
>
> As far as I know pseries is the only platform that uses DR connectors, and I
> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
> that this is really generic enough to belong in topology.h.

Right. This does not belong in include/linux.

> If anything I would
> suggest putting this in an include in arch/powerpc/include/ named something like
> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
> that want/need to use this functionality.

Yeah that would make more sense.

Using "arch" in the name is wrong, it's pseries specific so
pseries_find_drc_match() would be more appropriate.

>> +int arch_find_drc_match(struct device_node *dn,
>> +			bool (*usercb)(struct device_node *dn,
>> +				u32 drc_index, char *drc_name,
>> +				char *drc_type, u32 drc_power_domain,
>> +				void *data),
>> +			char *opt_drc_type, char *opt_drc_name,
>> +			bool match_drc_index, bool ck_php_type,
>> +			void *data);

This function signature is kind of insane.

You end with calls like:

+	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
+			NULL, NULL, false, true, NULL);

Which is impossible to parse.

I feel like maybe this isn't the right level of abstraction.

cheers
Michael Bringmann Jan. 29, 2019, 4:21 p.m. UTC | #8
On 1/29/19 3:31 AM, Michael Ellerman wrote:
> Tyrel Datwyler <turtle.in.the.kernel@gmail.com> writes:
>> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
>>> Define interface to acquire arch-specific drc info to match against
>>> hotpluggable devices.  The current implementation exposes several
>>> pseries-specific dynamic memory properties in generic kernel code.
>>> This patch set provides an interface to pull that code out of the
>>> generic kernel.
>>>
>>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>>> ---
>>>  include/linux/topology.h |    9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>>> index cb0775e..df97f5f 100644
>>> --- a/include/linux/topology.h
>>> +++ b/include/linux/topology.h
>>> @@ -44,6 +44,15 @@
>>
>> As far as I know pseries is the only platform that uses DR connectors, and I
>> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
>> that this is really generic enough to belong in topology.h.
> 
> Right. This does not belong in include/linux.
> 
>> If anything I would
>> suggest putting this in an include in arch/powerpc/include/ named something like
>> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
>> that want/need to use this functionality.
> 
> Yeah that would make more sense.

If you see no objection to referencing a powerpc-specific function from
the code ...

> 
> Using "arch" in the name is wrong, it's pseries specific so
> pseries_find_drc_match() would be more appropriate.
> 
>>> +int arch_find_drc_match(struct device_node *dn,
>>> +			bool (*usercb)(struct device_node *dn,
>>> +				u32 drc_index, char *drc_name,
>>> +				char *drc_type, u32 drc_power_domain,
>>> +				void *data),
>>> +			char *opt_drc_type, char *opt_drc_name,
>>> +			bool match_drc_index, bool ck_php_type,
>>> +			void *data);
> 
> This function signature is kind of insane.
> 
> You end with calls like:
> 
> +	return arch_find_drc_match(dn, rpaphp_add_slot_cb,
> +			NULL, NULL, false, true, NULL);
> 
> Which is impossible to parse.
> 
> I feel like maybe this isn't the right level of abstraction.

...
I had already been considering simplifying the interface for these
calls to something like the following:

int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
			char *drc_type)
{
    return pseries_find_drc_match(dn, drc_type, drc_name);
}
...
int rpaphp_add_slot(struct device_node *dn)
{
       if (!dn->name || strcmp(dn->name, "pci"))
               return 0;

       return pseries_add_drc_slot(dn, rpaphp_add_slot_cb);
}
...

Further details would be hidden within the pseries code.


> 
> cheers

Regards

Patch
diff mbox series

diff --git a/include/linux/topology.h b/include/linux/topology.h
index cb0775e..df97f5f 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -44,6 +44,15 @@ 
 
 int arch_update_cpu_topology(void);
 
+int arch_find_drc_match(struct device_node *dn,
+			bool (*usercb)(struct device_node *dn,
+				u32 drc_index, char *drc_name,
+				char *drc_type, u32 drc_power_domain,
+				void *data),
+			char *opt_drc_type, char *opt_drc_name,
+			bool match_drc_index, bool ck_php_type,
+			void *data);
+
 /* Conform to ACPI 2.0 SLIT distance definitions */
 #define LOCAL_DISTANCE		10
 #define REMOTE_DISTANCE		20