| Message ID | 20190425095542.726-4-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Thu, Apr 25, 2019 at 11:55:41AM +0200, Geert Uytterhoeven wrote: > The pinmux groups and functions arrays may contain two parts, to ease > supporting SoCs that expose pin subsets of other related SoCs. Both > parts need to be declared with explicit sizes, which thus need to be > updated when adding support for more groups and functions. > > If a size is too small, the compiler will detect this at build time > ("excess elements in array initializer"). > If a size is too large, this may go undetected (for pin groups), lead to > pin controller registration failures (for pin functions: "pinmux ops has > no name for functionN"), or crash the optional run-time debug code (for > pin groups). > > Extend the run-time debug code with checks to detect this, to help > catching bugs early. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > drivers/pinctrl/sh-pfc/core.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c > index 868b9551438efb0a..889e5af281022e1d 100644 > --- a/drivers/pinctrl/sh-pfc/core.c > +++ b/drivers/pinctrl/sh-pfc/core.c > @@ -780,9 +780,15 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info) > > for (i = 0; i < info->nr_functions; i++) { > func = &info->functions[i]; > + if (!func->name) { > + pr_err("%s: empty function %u\n", drvname, i); > + sh_pfc_errors++; Not strictly related to this patch but did you consider not having sh_pfc_errors global to this file? > + continue; > + } > for (j = 0; j < func->nr_groups; j++) { > for (k = 0; k < info->nr_groups; k++) { > - if (!strcmp(func->groups[j], > + if (info->groups[k].name && > + !strcmp(func->groups[j], > info->groups[k].name)) { > refcnts[k]++; > break; > @@ -798,6 +804,11 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info) > } > > for (i = 0; i < info->nr_groups; i++) { > + if (!info->groups[i].name) { > + pr_err("%s: empty group %u\n", drvname, i); > + sh_pfc_errors++; > + continue; > + } > if (!refcnts[i]) { > pr_err("%s: orphan group %s\n", drvname, > info->groups[i].name); > -- > 2.17.1 >
Hi Simon, On Fri, Apr 26, 2019 at 11:35 AM Simon Horman <horms@verge.net.au> wrote: > On Thu, Apr 25, 2019 at 11:55:41AM +0200, Geert Uytterhoeven wrote: > > The pinmux groups and functions arrays may contain two parts, to ease > > supporting SoCs that expose pin subsets of other related SoCs. Both > > parts need to be declared with explicit sizes, which thus need to be > > updated when adding support for more groups and functions. > > > > If a size is too small, the compiler will detect this at build time > > ("excess elements in array initializer"). > > If a size is too large, this may go undetected (for pin groups), lead to > > pin controller registration failures (for pin functions: "pinmux ops has > > no name for functionN"), or crash the optional run-time debug code (for > > pin groups). > > > > Extend the run-time debug code with checks to detect this, to help > > catching bugs early. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> Thank you! > > --- a/drivers/pinctrl/sh-pfc/core.c > > +++ b/drivers/pinctrl/sh-pfc/core.c > > @@ -780,9 +780,15 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info) > > > > for (i = 0; i < info->nr_functions; i++) { > > func = &info->functions[i]; > > + if (!func->name) { > > + pr_err("%s: empty function %u\n", drvname, i); > > + sh_pfc_errors++; > > Not strictly related to this patch but did > you consider not having sh_pfc_errors global to this file? Do you have an alternative? I could hide it, and combine it with the pr_err() in a sh_pfc_report_error() function, but it would still be global. Gr{oetje,eeting}s, Geert
On Fri, Apr 26, 2019 at 11:42:11AM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > On Fri, Apr 26, 2019 at 11:35 AM Simon Horman <horms@verge.net.au> wrote: > > On Thu, Apr 25, 2019 at 11:55:41AM +0200, Geert Uytterhoeven wrote: > > > The pinmux groups and functions arrays may contain two parts, to ease > > > supporting SoCs that expose pin subsets of other related SoCs. Both > > > parts need to be declared with explicit sizes, which thus need to be > > > updated when adding support for more groups and functions. > > > > > > If a size is too small, the compiler will detect this at build time > > > ("excess elements in array initializer"). > > > If a size is too large, this may go undetected (for pin groups), lead to > > > pin controller registration failures (for pin functions: "pinmux ops has > > > no name for functionN"), or crash the optional run-time debug code (for > > > pin groups). > > > > > > Extend the run-time debug code with checks to detect this, to help > > > catching bugs early. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > > Thank you! > > > > --- a/drivers/pinctrl/sh-pfc/core.c > > > +++ b/drivers/pinctrl/sh-pfc/core.c > > > @@ -780,9 +780,15 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info) > > > > > > for (i = 0; i < info->nr_functions; i++) { > > > func = &info->functions[i]; > > > + if (!func->name) { > > > + pr_err("%s: empty function %u\n", drvname, i); > > > + sh_pfc_errors++; > > > > Not strictly related to this patch but did > > you consider not having sh_pfc_errors global to this file? > > Do you have an alternative? > I could hide it, and combine it with the pr_err() in a sh_pfc_report_error() > function, but it would still be global. I didn't look carefully for an alternative. If its not practical to pass around a local variable then I'm happy with the current setup.
diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c index 868b9551438efb0a..889e5af281022e1d 100644 --- a/drivers/pinctrl/sh-pfc/core.c +++ b/drivers/pinctrl/sh-pfc/core.c @@ -780,9 +780,15 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info) for (i = 0; i < info->nr_functions; i++) { func = &info->functions[i]; + if (!func->name) { + pr_err("%s: empty function %u\n", drvname, i); + sh_pfc_errors++; + continue; + } for (j = 0; j < func->nr_groups; j++) { for (k = 0; k < info->nr_groups; k++) { - if (!strcmp(func->groups[j], + if (info->groups[k].name && + !strcmp(func->groups[j], info->groups[k].name)) { refcnts[k]++; break; @@ -798,6 +804,11 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info) } for (i = 0; i < info->nr_groups; i++) { + if (!info->groups[i].name) { + pr_err("%s: empty group %u\n", drvname, i); + sh_pfc_errors++; + continue; + } if (!refcnts[i]) { pr_err("%s: orphan group %s\n", drvname, info->groups[i].name);
The pinmux groups and functions arrays may contain two parts, to ease supporting SoCs that expose pin subsets of other related SoCs. Both parts need to be declared with explicit sizes, which thus need to be updated when adding support for more groups and functions. If a size is too small, the compiler will detect this at build time ("excess elements in array initializer"). If a size is too large, this may go undetected (for pin groups), lead to pin controller registration failures (for pin functions: "pinmux ops has no name for functionN"), or crash the optional run-time debug code (for pin groups). Extend the run-time debug code with checks to detect this, to help catching bugs early. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/pinctrl/sh-pfc/core.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)