| Message ID | 20181214204957.16435.29255.stgit@powerkvm6.aus.stglabs.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/pseries: Refactor code to centralize drcinfo parsing | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 15 lines checked |
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 >
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 >
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 >> > >
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 >> > >
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 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
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
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
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
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(+)