pinctrl: tegra: Duplicate pinmux functions table

PATCH In review Thierry Reding 2 participants 4 messages May 30, 2023 – Jun 1, 2023

Subsystems (3)

GPIO Tegra architecture support Pin control

lore

Share & tools

lore

Thierry Reding

author

2023-05-30 10:53 UTC #0 lore raw

PATCH 1/2 pinctrl: tegra: Duplicate pinmux functions table

From: Thierry Reding <[email protected]>

The function table is filled with group information based on other instance-specific data at runtime. However, the function table can be shared between multiple instances, causing the ->probe() function for one instance to overwrite the table of a previously probed instance.

Fix this by sharing only the function names and allocating a separate function table for each instance.

Fixes 5a0047360743 ("pinctrl: tegra: Separate Tegra194 instances")

Signed-off-by Thierry Reding <[email protected]>

---
 drivers/pinctrl/tegra/pinctrl-tegra.c    | 15 +++++++++++----
 drivers/pinctrl/tegra/pinctrl-tegra.h    |  3 ++-
 drivers/pinctrl/tegra/pinctrl-tegra114.c |  7 ++-----
 drivers/pinctrl/tegra/pinctrl-tegra124.c |  7 ++-----
 drivers/pinctrl/tegra/pinctrl-tegra194.c |  7 ++-----
 drivers/pinctrl/tegra/pinctrl-tegra20.c  |  7 ++-----
 drivers/pinctrl/tegra/pinctrl-tegra210.c |  7 ++-----
 drivers/pinctrl/tegra/pinctrl-tegra30.c  |  7 ++-----
 8 files changed, 25 insertions(+), 35 deletions(-)

drivers/pinctrl/tegra/pinctrl-tegra.c +11 -4

index 802801dbeb18..10c28534da48 100644
@@ -236,7 +236,7 @@ static const char *tegra_pinctrl_get_func_name(struct pinctrl_dev *pctldev,
 {
 	struct tegra_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
 
-	return pmx->soc->functions[function].name;
+	return pmx->functions[function].name;
 }
 
 static int tegra_pinctrl_get_func_groups(struct pinctrl_dev *pctldev,

@@ -246,8 +246,8 @@ static int tegra_pinctrl_get_func_groups(struct pinctrl_dev *pctldev,
 {
 	struct tegra_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
 
-	*groups = pmx->soc->functions[function].groups;
-	*num_groups = pmx->soc->functions[function].ngroups;
+	*groups = pmx->functions[function].groups;
+	*num_groups = pmx->functions[function].ngroups;
 
 	return 0;
 }

@@ -798,10 +798,17 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 	if (!pmx->group_pins)
 		return -ENOMEM;
 
+	pmx->functions = devm_kcalloc(&pdev->dev, pmx->soc->nfunctions,
+				      sizeof(*pmx->functions), GFP_KERNEL);
+	if (!pmx->functions)
+		return -ENOMEM;
+
 	group_pins = pmx->group_pins;
+
 	for (fn = 0; fn < soc_data->nfunctions; fn++) {
-		struct tegra_function *func = &soc_data->functions[fn];
+		struct tegra_function *func = &pmx->functions[fn];
 
+		func->name = pmx->soc->functions[fn];
 		func->groups = group_pins;
 
 		for (gn = 0; gn < soc_data->ngroups; gn++) {

drivers/pinctrl/tegra/pinctrl-tegra.h +2 -1

index 6130cba7cce5..b3289bdf727d 100644
@@ -13,6 +13,7 @@ struct tegra_pmx {
 	struct pinctrl_dev *pctl;
 
 	const struct tegra_pinctrl_soc_data *soc;
+	struct tegra_function *functions;
 	const char **group_pins;
 
 	struct pinctrl_gpio_range gpio_range;

@@ -191,7 +192,7 @@ struct tegra_pinctrl_soc_data {
 	const char *gpio_compatible;
 	const struct pinctrl_pin_desc *pins;
 	unsigned npins;
-	struct tegra_function *functions;
+	const char * const *functions;
 	unsigned nfunctions;
 	const struct tegra_pingroup *groups;
 	unsigned ngroups;

drivers/pinctrl/tegra/pinctrl-tegra114.c +2 -5

index e72ab1eb2398..3d425b2018e7 100644
@@ -1452,12 +1452,9 @@ enum tegra_mux {
 	TEGRA_MUX_VI_ALT3,
 };
 
-#define FUNCTION(fname)					\
-	{						\
-		.name = #fname,				\
-	}
+#define FUNCTION(fname) #fname
 
-static struct tegra_function tegra114_functions[] = {
+static const char * const tegra114_functions[] = {
 	FUNCTION(blink),
 	FUNCTION(cec),
 	FUNCTION(cldvfs),

drivers/pinctrl/tegra/pinctrl-tegra124.c +2 -5

index 26096c6b967e..2a50c5c7516c 100644
@@ -1611,12 +1611,9 @@ enum tegra_mux {
 	TEGRA_MUX_VIMCLK2_ALT,
 };
 
-#define FUNCTION(fname)					\
-	{						\
-		.name = #fname,				\
-	}
+#define FUNCTION(fname) #fname
 
-static struct tegra_function tegra124_functions[] = {
+static const char * const tegra124_functions[] = {
 	FUNCTION(blink),
 	FUNCTION(ccla),
 	FUNCTION(cec),

drivers/pinctrl/tegra/pinctrl-tegra194.c +2 -5

index 277973c88434..69f58df62897 100644
@@ -1189,12 +1189,9 @@ enum tegra_mux_dt {
 };
 
 /* Make list of each function name */
-#define TEGRA_PIN_FUNCTION(lid)			\
-	{					\
-		.name = #lid,			\
-	}
+#define TEGRA_PIN_FUNCTION(lid) #lid
 
-static struct tegra_function tegra194_functions[] = {
+static const char * const tegra194_functions[] = {
 	TEGRA_PIN_FUNCTION(rsvd0),
 	TEGRA_PIN_FUNCTION(rsvd1),
 	TEGRA_PIN_FUNCTION(rsvd2),

drivers/pinctrl/tegra/pinctrl-tegra20.c +2 -5

index 0dc2cf0d05b1..737fc2000f66 100644
@@ -1889,12 +1889,9 @@ enum tegra_mux {
 	TEGRA_MUX_XIO,
 };
 
-#define FUNCTION(fname)					\
-	{						\
-		.name = #fname,				\
-	}
+#define FUNCTION(fname) #fname
 
-static struct tegra_function tegra20_functions[] = {
+static const char * const tegra20_functions[] = {
 	FUNCTION(ahb_clk),
 	FUNCTION(apb_clk),
 	FUNCTION(audio_sync),

drivers/pinctrl/tegra/pinctrl-tegra210.c +2 -5

index b480f607fa16..9bb29146dfff 100644
@@ -1185,12 +1185,9 @@ enum tegra_mux {
 	TEGRA_MUX_VIMCLK2,
 };
 
-#define FUNCTION(fname)					\
-	{						\
-		.name = #fname,				\
-	}
+#define FUNCTION(fname) #fname
 
-static struct tegra_function tegra210_functions[] = {
+static const char * const tegra210_functions[] = {
 	FUNCTION(aud),
 	FUNCTION(bcl),
 	FUNCTION(blink),

drivers/pinctrl/tegra/pinctrl-tegra30.c +2 -5

index 7299a371827f..de5aa2d4d28d 100644
@@ -2010,12 +2010,9 @@ enum tegra_mux {
 	TEGRA_MUX_VI_ALT3,
 };
 
-#define FUNCTION(fname)					\
-	{						\
-		.name = #fname,				\
-	}
+#define FUNCTION(fname) #fname
 
-static struct tegra_function tegra30_functions[] = {
+static const char * const tegra30_functions[] = {
 	FUNCTION(blink),
 	FUNCTION(cec),
 	FUNCTION(clk_12m_out),

Signature · 1 line

2.40.1

Thierry Reding

author

2023-05-30 10:53 UTC #1 lore raw

PATCH 2/2 pinctrl: tegra: Consistently refer to SoC data

From: Thierry Reding <[email protected]>

The SoC-specific data is stored in pmx->soc and that's used throughout the driver to access this data. The probe function has access to a local version of that copy and uses it in some occasions. Replace them with the more standard pmx->soc access for more consistency.

Signed-off-by Thierry Reding <[email protected]>

---
 drivers/pinctrl/tegra/pinctrl-tegra.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

drivers/pinctrl/tegra/pinctrl-tegra.c +6 -7

index 10c28534da48..cd991bd504b9 100644
@@ -792,9 +792,8 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 	 * Each mux group will appear in 4 functions' list of groups.
 	 * This over-allocates slightly, since not all groups are mux groups.
 	 */
-	pmx->group_pins = devm_kcalloc(&pdev->dev,
-		soc_data->ngroups * 4, sizeof(*pmx->group_pins),
-		GFP_KERNEL);
+	pmx->group_pins = devm_kcalloc(&pdev->dev, pmx->soc->ngroups * 4,
+				       sizeof(*pmx->group_pins), GFP_KERNEL);
 	if (!pmx->group_pins)
 		return -ENOMEM;

@@ -805,14 +804,14 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 
 	group_pins = pmx->group_pins;
 
-	for (fn = 0; fn < soc_data->nfunctions; fn++) {
+	for (fn = 0; fn < pmx->soc->nfunctions; fn++) {
 		struct tegra_function *func = &pmx->functions[fn];
 
 		func->name = pmx->soc->functions[fn];
 		func->groups = group_pins;
 
-		for (gn = 0; gn < soc_data->ngroups; gn++) {
-			const struct tegra_pingroup *g = &soc_data->groups[gn];
+		for (gn = 0; gn < pmx->soc->ngroups; gn++) {
+			const struct tegra_pingroup *g = &pmx->soc->groups[gn];
 
 			if (g->mux_reg == -1)
 				continue;

@@ -824,7 +823,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 				continue;
 
 			BUG_ON(group_pins - pmx->group_pins >=
-				soc_data->ngroups * 4);
+				pmx->soc->ngroups * 4);
 			*group_pins++ = g->name;
 			func->ngroups++;
 		}

Signature · 1 line

2.40.1

Linus Walleij

2023-06-01 11:19 UTC #2 lore raw

In reply to Thierry Reding

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>
The function table is filled with group information based on other instance-specific data at runtime. However, the function table can be shared between multiple instances, causing the ->probe() function for one instance to overwrite the table of a previously probed instance.

Quoted 5 lines

Fix this by sharing only the function names and allocating a separate function table for each instance.
Fixes 5a0047360743 ("pinctrl: tegra: Separate Tegra194 instances")
Signed-off-by Thierry Reding <[email protected]>

Both patches applied!

I can't figure out if the problem is urgent or just wasting memory, so I applied it as non-urgent fix for now, tell me if this needs to go upstream pronto.

Yours,
Linus Walleij

Thierry Reding

author

2023-06-01 12:27 UTC #3 lore raw

Quoted 13 lines · Linus Walleij

Quoted 10 lines · Thierry Reding
From: Thierry Reding <[email protected]>
The function table is filled with group information based on other instance-specific data at runtime. However, the function table can be shared between multiple instances, causing the ->probe() function for one instance to overwrite the table of a previously probed instance.
Fix this by sharing only the function names and allocating a separate function table for each instance.

Quoted 8 lines

Quoted 2 lines
Fixes 5a0047360743 ("pinctrl: tegra: Separate Tegra194 instances")
Signed-off-by Thierry Reding <[email protected]>
Both patches applied!
I can't figure out if the problem is urgent or just wasting memory, so I applied it as non-urgent fix for now, tell me if this needs to go upstream pronto.

I think you might be able to write a device tree that triggers this, but so far I've only seen it cause an WARN splat when accessing the debugfs attribute that reads out the pinmux functions. I think Prathamesh had mentioned it could also hang the system when you access debugfs.

Overall I don't think this is very urgent. We only observed this as part of testing the Tegra234 patches that are under review, so it's probably uncommon for people to run the problematic code paths.

No matter what you decide, the Fixes: tag should make sure it goes into stable releases which is probably good enough.

Thierry