Skip to contentSkip to conversation

pinctrl: mcp23s08: Use full chunk of memory for regmap configuration

PATCH In review Andy Shevchenko 5 participants 7 messages Oct 9, 2020 – Nov 5, 2020

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/2 pinctrl: mcp23s08: Use full chunk of memory for regmap configuration

It appears that simplification of mcp23s08_spi_regmap_init() made a regression due to wrong size calculation for dev_kmemdup() call. It mises the fact that config variable is already a pointer, thus the sizeof() calculation is wrong and only 4 or 8 bytes were copied.

Fix the parameters to devm_kmemdup() to copy full chunk of memory.

Fixes 0874758ecb2b ("pinctrl: mcp23s08: Refactor mcp23s08_spi_regmap_init()")

---
v2: fixed few typos in the commit message

 drivers/pinctrl/pinctrl-mcp23s08_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/pinctrl-mcp23s08_spi.c +1 -1
index 1f47a661b0a7..7c72cffe1412 100644
@@ -119,7 +119,7 @@ static int mcp23s08_spi_regmap_init(struct mcp23s08 *mcp, struct device *dev,
 		return -EINVAL;
 	}
 
-	copy = devm_kmemdup(dev, &config, sizeof(config), GFP_KERNEL);
+	copy = devm_kmemdup(dev, config, sizeof(*config), GFP_KERNEL);
 	if (!copy)
 		return -ENOMEM;
 
Signature · 1 line
2.28.0
#1 lore raw

PATCH v2 2/2 pinctrl: mcp23s08: Print error message when regmap init fails

It is useful for debugging to have the error message printed when regmap initialisation fails. Add it to the driver.

Cc Martin Hundebøll <[email protected]>

---
v2: no changes

 drivers/pinctrl/pinctrl-mcp23s08_spi.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/pinctrl/pinctrl-mcp23s08_spi.c +2 -0
index 7c72cffe1412..9ae10318f6f3 100644
@@ -126,6 +126,8 @@ static int mcp23s08_spi_regmap_init(struct mcp23s08 *mcp, struct device *dev,
 	copy->name = name;
 
 	mcp->regmap = devm_regmap_init(dev, &mcp23sxx_spi_regmap, mcp, copy);
+	if (IS_ERR(mcp->regmap))
+		dev_err(dev, "regmap init failed for %s\n", mcp->chip.label);
 	return PTR_ERR_OR_ZERO(mcp->regmap);
 }
 
Signature · 1 line
2.28.0
Tested-by on v2 1/2
#2 lore raw
In reply to Andy Shevchenko

Hi Linus,

You can add my

Thanks,
Martin

Quoted 29 lines · Andy Shevchenko

It appears that simplification of mcp23s08_spi_regmap_init() made a regression due to wrong size calculation for dev_kmemdup() call. It mises the fact that config variable is already a pointer, thus the sizeof() calculation is wrong and only 4 or 8 bytes were copied.

Fix the parameters to devm_kmemdup() to copy full chunk of memory.

Fixes 0874758ecb2b ("pinctrl: mcp23s08: Refactor mcp23s08_spi_regmap_init()")

---
v2: fixed few typos in the commit message

  drivers/pinctrl/pinctrl-mcp23s08_spi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/pinctrl-mcp23s08_spi.c +1 -1
index 1f47a661b0a7..7c72cffe1412 100644
@@ -119,7 +119,7 @@ static int mcp23s08_spi_regmap_init(struct mcp23s08 *mcp, struct device *dev,
  		return -EINVAL;
  	}
 
-	copy = devm_kmemdup(dev, &config, sizeof(config), GFP_KERNEL);
+	copy = devm_kmemdup(dev, config, sizeof(*config), GFP_KERNEL);
  	if (!copy)
  		return -ENOMEM;
 
#3 lore raw
Quoted 5 lines · Martin Hundebøll

Hi Linus,

You can add my

Thank you!
Linus, below more typo fixes, but I'm not going to send a new version (until you explicitly require), so please take them into consideration when applying.

Quoted 8 lines

Thanks,
Martin

Quoted 4 lines · Andy Shevchenko

It appears that simplification of mcp23s08_spi_regmap_init() made a regression due to wrong size calculation for dev_kmemdup() call.

It mises the fact that config variable is already a pointer, thus

mises -> misses

Quoted 3 lines
Quoted 3 lines

the sizeof() calculation is wrong and only 4 or 8 bytes were copied.

Fix the parameters to devm_kmemdup() to copy full chunk of memory.

copy full -> copy a full

Quoted 23 lines
Quoted 23 lines
Fixes 0874758ecb2b ("pinctrl: mcp23s08: Refactor mcp23s08_spi_regmap_init()")

---
v2: fixed few typos in the commit message

  drivers/pinctrl/pinctrl-mcp23s08_spi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pinctrl/pinctrl-mcp23s08_spi.c +1 -1
index 1f47a661b0a7..7c72cffe1412 100644
@@ -119,7 +119,7 @@ static int mcp23s08_spi_regmap_init(struct mcp23s08 *mcp, struct device *dev,
              return -EINVAL;
      }
 
-     copy = devm_kmemdup(dev, &config, sizeof(config), GFP_KERNEL);
+     copy = devm_kmemdup(dev, config, sizeof(*config), GFP_KERNEL);
      if (!copy)
              return -ENOMEM;
 
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#4 lore raw
In reply to Andy Shevchenko
Quoted 5 lines · Andy Shevchenko

It appears that simplification of mcp23s08_spi_regmap_init() made a regression due to wrong size calculation for dev_kmemdup() call. It mises the fact that config variable is already a pointer, thus the sizeof() calculation is wrong and only 4 or 8 bytes were copied.

Quoted 7 lines

Fix the parameters to devm_kmemdup() to copy full chunk of memory.

Fixes 0874758ecb2b ("pinctrl: mcp23s08: Refactor mcp23s08_spi_regmap_init()")

---
v2: fixed few typos in the commit message

Patches applied, fixed the extra typos in the process, thanks!

Linus Walleij

Tested-by
#5 lore raw
Quoted 2 lines

Patches applied, fixed the extra typos in the process, thanks!

Hi Linus,
are these patches already pushed? I've checked the gpio and pinctrl trees, and I don't think you've pushed them.

Anyway, they fix a boot-time oops with 5.9.0 for me, so:

With kind regards,
Jan

#6 lore raw
Quoted 10 lines · Jan Kundrát
Quoted 2 lines

Patches applied, fixed the extra typos in the process, thanks!

Hi Linus,
are these patches already pushed? I've checked the gpio and pinctrl trees, and I don't think you've pushed them.

Anyway, they fix a boot-time oops with 5.9.0 for me, so:

I applied them as non-critical fixes but now I will move them to fixes, add your tested-by and merge ASAP.

Yours,
Linus Walleij