| Message ID | 2e2261b4-c0b3-3b6d-d649-541bc8802546@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | powerpc/drcinfo: Fix bugs 'ibm,drc-info' property | expand |
On 05/22/2018 11:37 AM, Michael Bringmann wrote: > This patch provides a common parse function for the ibm,drc-info > property that can be modified by a callback function. The caller > provides a pointer to the function and a pointer to their unique > data, and the parser provides the current lmb set from the struct. > The callback function may return codes indicating that the parsing > is complete, or should continue, along with an error code that may > be returned to the caller. > > Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> > Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar > e feature" -- end of patch series applied to powerpc next) > --- > Changes in V4: > -- Update code to account for latest kernel checkins. > -- Rebased to 4.17-rc5 kernel > -- Some patch cleanup including file combination > --- > arch/powerpc/include/asm/prom.h | 7 +++++ > arch/powerpc/platforms/pseries/of_helpers.c | 37 +++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h > index b04c5ce..2e947b3 100644 > --- a/arch/powerpc/include/asm/prom.h > +++ b/arch/powerpc/include/asm/prom.h > @@ -94,6 +94,13 @@ struct of_drc_info { > extern int of_read_drc_info_cell(struct property **prop, > const __be32 **curval, struct of_drc_info *data); > > +extern int drc_info_parser(struct device_node *dn, > + int (*usercb)(struct of_drc_info *drc, > + void *data, > + void *optional_data, The optional_data parameter to the callback routine doesn't seem to be used. -Nathan > + int *ret_code), > + char *opt_drc_type, > + void *data); > > /* > * There are two methods for telling firmware what our capabilities are. > diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c > index 11b2ef1..a588ee6 100644 > --- a/arch/powerpc/platforms/pseries/of_helpers.c > +++ b/arch/powerpc/platforms/pseries/of_helpers.c > @@ -6,6 +6,9 @@ > #include <asm/prom.h> > > #include "of_helpers.h" > +#include "pseries.h" > + > +#define MAX_DRC_NAME_LEN 64 > > /** > * pseries_of_derive_parent - basically like dirname(1) > @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, > return 0; > } > EXPORT_SYMBOL(of_read_drc_info_cell); > + > +int drc_info_parser(struct device_node *dn, > + int (*usercb)(struct of_drc_info *drc, > + void *data, > + void *optional_data, > + int *ret_code), > + char *opt_drc_type, > + void *data) > +{ > + struct property *info; > + unsigned int entries; > + struct of_drc_info drc; > + const __be32 *value; > + int j, done = 0, ret_code = -EINVAL; > + > + info = of_find_property(dn, "ibm,drc-info", NULL); > + if (info == NULL) > + return -EINVAL; > + > + value = info->value; > + entries = of_read_number(value++, 1); > + > + for (j = 0, done = 0; (j < entries) && (!done); j++) { > + of_read_drc_info_cell(&info, &value, &drc); > + > + if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type)) > + continue; > + > + done = usercb(&drc, data, NULL, &ret_code); > + } > + > + return ret_code; > +} > +EXPORT_SYMBOL(drc_info_parser); >
On 05/22/2018 11:37 AM, Michael Bringmann wrote: > This patch provides a common parse function for the ibm,drc-info > property that can be modified by a callback function. The caller > provides a pointer to the function and a pointer to their unique > data, and the parser provides the current lmb set from the struct. > The callback function may return codes indicating that the parsing > is complete, or should continue, along with an error code that may > be returned to the caller. > > Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> > Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar > e feature" -- end of patch series applied to powerpc next) > --- > Changes in V4: > -- Update code to account for latest kernel checkins. > -- Rebased to 4.17-rc5 kernel > -- Some patch cleanup including file combination > --- > arch/powerpc/include/asm/prom.h | 7 +++++ > arch/powerpc/platforms/pseries/of_helpers.c | 37 +++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h > index b04c5ce..2e947b3 100644 > --- a/arch/powerpc/include/asm/prom.h > +++ b/arch/powerpc/include/asm/prom.h > @@ -94,6 +94,13 @@ struct of_drc_info { > extern int of_read_drc_info_cell(struct property **prop, > const __be32 **curval, struct of_drc_info *data); > > +extern int drc_info_parser(struct device_node *dn, > + int (*usercb)(struct of_drc_info *drc, > + void *data, > + void *optional_data, > + int *ret_code), > + char *opt_drc_type, > + void *data); After looking at the patch 3 in this series, I think a couple of comments and a small change may help. It was not clear at first what the call back function was supposed to return. After reading users of this routine it appears that the callback function is returning a bool value indicating whether or not the parser should continue. I documenting this and having the callback routine return a bool may make this clearer. Also, I see other places in the kernel name these types of routines as walk_*, perhaps a slight name change to walk_drc_info_entries() may also make it clearer what the code is doing. -Nathan > > /* > * There are two methods for telling firmware what our capabilities are. > diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c > index 11b2ef1..a588ee6 100644 > --- a/arch/powerpc/platforms/pseries/of_helpers.c > +++ b/arch/powerpc/platforms/pseries/of_helpers.c > @@ -6,6 +6,9 @@ > #include <asm/prom.h> > > #include "of_helpers.h" > +#include "pseries.h" > + > +#define MAX_DRC_NAME_LEN 64 > > /** > * pseries_of_derive_parent - basically like dirname(1) > @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, > return 0; > } > EXPORT_SYMBOL(of_read_drc_info_cell); > + > +int drc_info_parser(struct device_node *dn, > + int (*usercb)(struct of_drc_info *drc, > + void *data, > + void *optional_data, > + int *ret_code), > + char *opt_drc_type, > + void *data) > +{ > + struct property *info; > + unsigned int entries; > + struct of_drc_info drc; > + const __be32 *value; > + int j, done = 0, ret_code = -EINVAL; > + > + info = of_find_property(dn, "ibm,drc-info", NULL); > + if (info == NULL) > + return -EINVAL; > + > + value = info->value; > + entries = of_read_number(value++, 1); > + > + for (j = 0, done = 0; (j < entries) && (!done); j++) { > + of_read_drc_info_cell(&info, &value, &drc); > + > + if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type)) > + continue; > + > + done = usercb(&drc, data, NULL, &ret_code); > + } > + > + return ret_code; > +} > +EXPORT_SYMBOL(drc_info_parser); >
See below. On 05/22/2018 04:02 PM, Nathan Fontenot wrote: > On 05/22/2018 11:37 AM, Michael Bringmann wrote: >> This patch provides a common parse function for the ibm,drc-info >> property that can be modified by a callback function. The caller >> provides a pointer to the function and a pointer to their unique >> data, and the parser provides the current lmb set from the struct. >> The callback function may return codes indicating that the parsing >> is complete, or should continue, along with an error code that may >> be returned to the caller. >> >> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> >> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar >> e feature" -- end of patch series applied to powerpc next) >> --- >> Changes in V4: >> -- Update code to account for latest kernel checkins. >> -- Rebased to 4.17-rc5 kernel >> -- Some patch cleanup including file combination >> --- >> arch/powerpc/include/asm/prom.h | 7 +++++ >> arch/powerpc/platforms/pseries/of_helpers.c | 37 +++++++++++++++++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h >> index b04c5ce..2e947b3 100644 >> --- a/arch/powerpc/include/asm/prom.h >> +++ b/arch/powerpc/include/asm/prom.h >> @@ -94,6 +94,13 @@ struct of_drc_info { >> extern int of_read_drc_info_cell(struct property **prop, >> const __be32 **curval, struct of_drc_info *data); >> >> +extern int drc_info_parser(struct device_node *dn, >> + int (*usercb)(struct of_drc_info *drc, >> + void *data, >> + void *optional_data, > > The optional_data parameter to the callback routine doesn't seem to be used. > > -Nathan There was one odd case when parsing the 'ibm,drc-info' code where I remember needing extra data. I think that I can compress it all into the other pointer though. Will remove the extra argument. Michael > >> + int *ret_code), >> + char *opt_drc_type, >> + void *data); >> >> /* >> * There are two methods for telling firmware what our capabilities are. >> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c >> index 11b2ef1..a588ee6 100644 >> --- a/arch/powerpc/platforms/pseries/of_helpers.c >> +++ b/arch/powerpc/platforms/pseries/of_helpers.c >> @@ -6,6 +6,9 @@ >> #include <asm/prom.h> >> >> #include "of_helpers.h" >> +#include "pseries.h" >> + >> +#define MAX_DRC_NAME_LEN 64 >> >> /** >> * pseries_of_derive_parent - basically like dirname(1) >> @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, >> return 0; >> } >> EXPORT_SYMBOL(of_read_drc_info_cell); >> + >> +int drc_info_parser(struct device_node *dn, >> + int (*usercb)(struct of_drc_info *drc, >> + void *data, >> + void *optional_data, >> + int *ret_code), >> + char *opt_drc_type, >> + void *data) >> +{ >> + struct property *info; >> + unsigned int entries; >> + struct of_drc_info drc; >> + const __be32 *value; >> + int j, done = 0, ret_code = -EINVAL; >> + >> + info = of_find_property(dn, "ibm,drc-info", NULL); >> + if (info == NULL) >> + return -EINVAL; >> + >> + value = info->value; >> + entries = of_read_number(value++, 1); >> + >> + for (j = 0, done = 0; (j < entries) && (!done); j++) { >> + of_read_drc_info_cell(&info, &value, &drc); >> + >> + if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type)) >> + continue; >> + >> + done = usercb(&drc, data, NULL, &ret_code); >> + } >> + >> + return ret_code; >> +} >> +EXPORT_SYMBOL(drc_info_parser); >> > >
See below. On 05/22/2018 04:23 PM, Nathan Fontenot wrote: > On 05/22/2018 11:37 AM, Michael Bringmann wrote: >> This patch provides a common parse function for the ibm,drc-info >> property that can be modified by a callback function. The caller >> provides a pointer to the function and a pointer to their unique >> data, and the parser provides the current lmb set from the struct. >> The callback function may return codes indicating that the parsing >> is complete, or should continue, along with an error code that may >> be returned to the caller. >> >> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> >> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar >> e feature" -- end of patch series applied to powerpc next) >> --- >> Changes in V4: >> -- Update code to account for latest kernel checkins. >> -- Rebased to 4.17-rc5 kernel >> -- Some patch cleanup including file combination >> --- >> arch/powerpc/include/asm/prom.h | 7 +++++ >> arch/powerpc/platforms/pseries/of_helpers.c | 37 +++++++++++++++++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h >> index b04c5ce..2e947b3 100644 >> --- a/arch/powerpc/include/asm/prom.h >> +++ b/arch/powerpc/include/asm/prom.h >> @@ -94,6 +94,13 @@ struct of_drc_info { >> extern int of_read_drc_info_cell(struct property **prop, >> const __be32 **curval, struct of_drc_info *data); >> >> +extern int drc_info_parser(struct device_node *dn, >> + int (*usercb)(struct of_drc_info *drc, >> + void *data, >> + void *optional_data, >> + int *ret_code), >> + char *opt_drc_type, >> + void *data); > > After looking at the patch 3 in this series, I think a couple of comments and > a small change may help. It was not clear at first what the call back function > was supposed to return. After reading users of this routine it appears that the > callback function is returning a bool value indicating whether or not the parser > should continue. I documenting this and having the callback routine return a bool > may make this clearer. Okay. > > Also, I see other places in the kernel name these types of routines as walk_*, > perhaps a slight name change to walk_drc_info_entries() may also make it clearer > what the code is doing. Okay. > > -Nathan Michael > >> >> /* >> * There are two methods for telling firmware what our capabilities are. >> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c >> index 11b2ef1..a588ee6 100644 >> --- a/arch/powerpc/platforms/pseries/of_helpers.c >> +++ b/arch/powerpc/platforms/pseries/of_helpers.c >> @@ -6,6 +6,9 @@ >> #include <asm/prom.h> >> >> #include "of_helpers.h" >> +#include "pseries.h" >> + >> +#define MAX_DRC_NAME_LEN 64 >> >> /** >> * pseries_of_derive_parent - basically like dirname(1) >> @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, >> return 0; >> } >> EXPORT_SYMBOL(of_read_drc_info_cell); >> + >> +int drc_info_parser(struct device_node *dn, >> + int (*usercb)(struct of_drc_info *drc, >> + void *data, >> + void *optional_data, >> + int *ret_code), >> + char *opt_drc_type, >> + void *data) >> +{ >> + struct property *info; >> + unsigned int entries; >> + struct of_drc_info drc; >> + const __be32 *value; >> + int j, done = 0, ret_code = -EINVAL; >> + >> + info = of_find_property(dn, "ibm,drc-info", NULL); >> + if (info == NULL) >> + return -EINVAL; >> + >> + value = info->value; >> + entries = of_read_number(value++, 1); >> + >> + for (j = 0, done = 0; (j < entries) && (!done); j++) { >> + of_read_drc_info_cell(&info, &value, &drc); >> + >> + if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type)) >> + continue; >> + >> + done = usercb(&drc, data, NULL, &ret_code); >> + } >> + >> + return ret_code; >> +} >> +EXPORT_SYMBOL(drc_info_parser); >> > >
diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h index b04c5ce..2e947b3 100644 --- a/arch/powerpc/include/asm/prom.h +++ b/arch/powerpc/include/asm/prom.h @@ -94,6 +94,13 @@ struct of_drc_info { extern int of_read_drc_info_cell(struct property **prop, const __be32 **curval, struct of_drc_info *data); +extern int drc_info_parser(struct device_node *dn, + int (*usercb)(struct of_drc_info *drc, + void *data, + void *optional_data, + int *ret_code), + char *opt_drc_type, + void *data); /* * There are two methods for telling firmware what our capabilities are. diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c index 11b2ef1..a588ee6 100644 --- a/arch/powerpc/platforms/pseries/of_helpers.c +++ b/arch/powerpc/platforms/pseries/of_helpers.c @@ -6,6 +6,9 @@ #include <asm/prom.h> #include "of_helpers.h" +#include "pseries.h" + +#define MAX_DRC_NAME_LEN 64 /** * pseries_of_derive_parent - basically like dirname(1) @@ -87,3 +90,37 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval, return 0; } EXPORT_SYMBOL(of_read_drc_info_cell); + +int drc_info_parser(struct device_node *dn, + int (*usercb)(struct of_drc_info *drc, + void *data, + void *optional_data, + int *ret_code), + char *opt_drc_type, + void *data) +{ + struct property *info; + unsigned int entries; + struct of_drc_info drc; + const __be32 *value; + int j, done = 0, ret_code = -EINVAL; + + info = of_find_property(dn, "ibm,drc-info", NULL); + if (info == NULL) + return -EINVAL; + + value = info->value; + entries = of_read_number(value++, 1); + + for (j = 0, done = 0; (j < entries) && (!done); j++) { + of_read_drc_info_cell(&info, &value, &drc); + + if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type)) + continue; + + done = usercb(&drc, data, NULL, &ret_code); + } + + return ret_code; +} +EXPORT_SYMBOL(drc_info_parser);
This patch provides a common parse function for the ibm,drc-info property that can be modified by a callback function. The caller provides a pointer to the function and a pointer to their unique data, and the parser provides the current lmb set from the struct. The callback function may return codes indicating that the parsing is complete, or should continue, along with an error code that may be returned to the caller. Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> Fixes: 3f38000eda48 ("powerpc/firmware: Add definitions for new drc-info firmwar e feature" -- end of patch series applied to powerpc next) --- Changes in V4: -- Update code to account for latest kernel checkins. -- Rebased to 4.17-rc5 kernel -- Some patch cleanup including file combination --- arch/powerpc/include/asm/prom.h | 7 +++++ arch/powerpc/platforms/pseries/of_helpers.c | 37 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)