[PATCH] regulator: hi6421v600: Fix setting wrong driver_data

[PATCH] regulator: hi6421v600: Fix setting wrong driver_data

[Axel Lin]

Current code set "config.driver_data = sreg" but sreg only init the mutex,
the othere fields are just zero. Fix it by pass *info to config.driver_data
so each regulator can get corresponding data by rdev_get_drvdata().

Separate enable_mutex from struct hi6421_spmi_reg_info since only need one
mutex for the driver.

Fixes: d2dfd50a0b57 ("staging: hikey9xx: hi6421v600-regulator: move LDO config from DT")
Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/hi6421v600-regulator.c | 26 ++++++++++++++----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/hi6421v600-regulator.c b/drivers/regulator/hi6421v600-regulator.c
index b5a19938fd3a..21153ee03a3f 100644
--- a/drivers/regulator/hi6421v600-regulator.c
+++ b/drivers/regulator/hi6421v600-regulator.c
@@ -16,13 +16,15 @@
 #include <linux/regulator/driver.h>
 #include <linux/spmi.h>
 
+struct hi6421_spmi_reg_priv {
+	/* Serialize regulator enable logic */
+	struct mutex enable_mutex;
+};
+
 struct hi6421_spmi_reg_info {
 	struct regulator_desc	desc;
 	u8			eco_mode_mask;
 	u32			eco_uA;
-
-	/* Serialize regulator enable logic */
-	struct mutex enable_mutex;
 };
 
 static const unsigned int ldo3_voltages[] = {
@@ -96,11 +98,12 @@ static const unsigned int ldo34_voltages[] = {
 
 static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
 {
-	struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev);
+	struct hi6421_spmi_reg_priv *priv;
 	int ret;
 
+	priv = dev_get_drvdata(rdev->dev.parent);
 	/* cannot enable more than one regulator at one time */
-	mutex_lock(&sreg->enable_mutex);
+	mutex_lock(&priv->enable_mutex);
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				 rdev->desc->enable_mask,
@@ -109,7 +112,7 @@ static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
 	/* Avoid powering up multiple devices at the same time */
 	usleep_range(rdev->desc->off_on_delay, rdev->desc->off_on_delay + 60);
 
-	mutex_unlock(&sreg->enable_mutex);
+	mutex_unlock(&priv->enable_mutex);
 
 	return ret;
 }
@@ -225,7 +228,7 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
 {
 	struct device *pmic_dev = pdev->dev.parent;
 	struct regulator_config config = { };
-	struct hi6421_spmi_reg_info *sreg;
+	struct hi6421_spmi_reg_priv *priv;
 	struct hi6421_spmi_reg_info *info;
 	struct device *dev = &pdev->dev;
 	struct hi6421_spmi_pmic *pmic;
@@ -241,17 +244,18 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
 	if (WARN_ON(!pmic))
 		return -ENODEV;
 
-	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
-	if (!sreg)
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
-	mutex_init(&sreg->enable_mutex);
+	mutex_init(&priv->enable_mutex);
+	platform_set_drvdata(pdev, priv);
 
 	for (i = 0; i < ARRAY_SIZE(regulator_info); i++) {
 		info = &regulator_info[i];
 
 		config.dev = pdev->dev.parent;
-		config.driver_data = sreg;
+		config.driver_data = info;
 		config.regmap = pmic->regmap;
 
 		rdev = devm_regulator_register(dev, &info->desc, &config);
-- 
2.25.1

Re: [PATCH] regulator: hi6421v600: Fix setting wrong driver_data

[Mark Brown]

On Tue, 22 Jun 2021 12:33:29 +0800, Axel Lin wrote:
> Current code set "config.driver_data = sreg" but sreg only init the mutex,
> the othere fields are just zero. Fix it by pass *info to config.driver_data
> so each regulator can get corresponding data by rdev_get_drvdata().
> 
> Separate enable_mutex from struct hi6421_spmi_reg_info since only need one
> mutex for the driver.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: hi6421v600: Fix setting wrong driver_data
      commit: 9bc146acc33125cd9f365b92f1c02ec89f639977

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] regulator: hi6421v600: Fix setting wrong driver_data

[Mauro Carvalho Chehab]

Em Tue, 22 Jun 2021 12:33:29 +0800
Axel Lin <axel.lin@ingics.com> escreveu:

> Current code set "config.driver_data = sreg" but sreg only init the mutex,
> the othere fields are just zero. Fix it by pass *info to config.driver_data
> so each regulator can get corresponding data by rdev_get_drvdata().
> 
> Separate enable_mutex from struct hi6421_spmi_reg_info since only need one
> mutex for the driver.
> 
> Fixes: d2dfd50a0b57 ("staging: hikey9xx: hi6421v600-regulator: move LDO config from DT")
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  drivers/regulator/hi6421v600-regulator.c | 26 ++++++++++++++----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/regulator/hi6421v600-regulator.c b/drivers/regulator/hi6421v600-regulator.c
> index b5a19938fd3a..21153ee03a3f 100644
> --- a/drivers/regulator/hi6421v600-regulator.c
> +++ b/drivers/regulator/hi6421v600-regulator.c
> @@ -16,13 +16,15 @@
>  #include <linux/regulator/driver.h>
>  #include <linux/spmi.h>
>  
> +struct hi6421_spmi_reg_priv {
> +	/* Serialize regulator enable logic */
> +	struct mutex enable_mutex;
> +};
> +
>  struct hi6421_spmi_reg_info {
>  	struct regulator_desc	desc;
>  	u8			eco_mode_mask;
>  	u32			eco_uA;
> -
> -	/* Serialize regulator enable logic */
> -	struct mutex enable_mutex;
>  };
>  
>  static const unsigned int ldo3_voltages[] = {
> @@ -96,11 +98,12 @@ static const unsigned int ldo34_voltages[] = {
>  
>  static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
>  {
> -	struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev);
> +	struct hi6421_spmi_reg_priv *priv;
>  	int ret;
>  
> +	priv = dev_get_drvdata(rdev->dev.parent);
>  	/* cannot enable more than one regulator at one time */
> -	mutex_lock(&sreg->enable_mutex);
> +	mutex_lock(&priv->enable_mutex);
>  
>  	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>  				 rdev->desc->enable_mask,
> @@ -109,7 +112,7 @@ static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
>  	/* Avoid powering up multiple devices at the same time */
>  	usleep_range(rdev->desc->off_on_delay, rdev->desc->off_on_delay + 60);
>  
> -	mutex_unlock(&sreg->enable_mutex);
> +	mutex_unlock(&priv->enable_mutex);
>  
>  	return ret;
>  }
> @@ -225,7 +228,7 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
>  {
>  	struct device *pmic_dev = pdev->dev.parent;
>  	struct regulator_config config = { };
> -	struct hi6421_spmi_reg_info *sreg;
> +	struct hi6421_spmi_reg_priv *priv;
>  	struct hi6421_spmi_reg_info *info;
>  	struct device *dev = &pdev->dev;
>  	struct hi6421_spmi_pmic *pmic;
> @@ -241,17 +244,18 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
>  	if (WARN_ON(!pmic))
>  		return -ENODEV;
>  
> -	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
> -	if (!sreg)
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
>  
> -	mutex_init(&sreg->enable_mutex);
> +	mutex_init(&priv->enable_mutex);


> +	platform_set_drvdata(pdev, priv);

This won't work, as platform data was already set, as it is already used
as part of the MFD/SPMI binding logic. With the current 
struct hi6421_spmi_pmic, this is not a big problem, as, due to some
code churns, there are enough space for a mutex at the beginning of
the struct, but the code breaks when those are removed.

I'll send a separate patch addressing this issue.

Thanks,
Mauro

Re: [PATCH] regulator: hi6421v600: Fix setting wrong driver_data

[Axel Lin]

> > -     mutex_init(&sreg->enable_mutex);
> > +     mutex_init(&priv->enable_mutex);
>
>
> > +     platform_set_drvdata(pdev, priv);
>
> This won't work, as platform data was already set, as it is already used
> as part of the MFD/SPMI binding logic. With the current

I think you mixed dev->platform_data with dev->driver_data.
Can you check again?

Regards,
Axel

Re: [PATCH] regulator: hi6421v600: Fix setting wrong driver_data

[Mauro Carvalho Chehab]

Em Tue, 22 Jun 2021 12:33:29 +0800
Axel Lin <axel.lin@ingics.com> escreveu:

> Current code set "config.driver_data = sreg" but sreg only init the mutex,
> the othere fields are just zero. Fix it by pass *info to config.driver_data
> so each regulator can get corresponding data by rdev_get_drvdata().
> 
> Separate enable_mutex from struct hi6421_spmi_reg_info since only need one
> mutex for the driver.
> 
> Fixes: d2dfd50a0b57 ("staging: hikey9xx: hi6421v600-regulator: move LDO config from DT")
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

As discussed on a separate thread, this patch is broken. See below.

> ---
>  drivers/regulator/hi6421v600-regulator.c | 26 ++++++++++++++----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/regulator/hi6421v600-regulator.c b/drivers/regulator/hi6421v600-regulator.c
> index b5a19938fd3a..21153ee03a3f 100644
> --- a/drivers/regulator/hi6421v600-regulator.c
> +++ b/drivers/regulator/hi6421v600-regulator.c
> @@ -16,13 +16,15 @@
>  #include <linux/regulator/driver.h>
>  #include <linux/spmi.h>
>  
> +struct hi6421_spmi_reg_priv {
> +	/* Serialize regulator enable logic */
> +	struct mutex enable_mutex;
> +};
> +
>  struct hi6421_spmi_reg_info {
>  	struct regulator_desc	desc;
>  	u8			eco_mode_mask;
>  	u32			eco_uA;
> -
> -	/* Serialize regulator enable logic */
> -	struct mutex enable_mutex;
>  };
>  
>  static const unsigned int ldo3_voltages[] = {
> @@ -96,11 +98,12 @@ static const unsigned int ldo34_voltages[] = {
>  
>  static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
>  {
> -	struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev);
> +	struct hi6421_spmi_reg_priv *priv;
>  	int ret;
>  
> +	priv = dev_get_drvdata(rdev->dev.parent);
>  	/* cannot enable more than one regulator at one time */
> -	mutex_lock(&sreg->enable_mutex);
> +	mutex_lock(&priv->enable_mutex);
>  
>  	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>  				 rdev->desc->enable_mask,
> @@ -109,7 +112,7 @@ static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
>  	/* Avoid powering up multiple devices at the same time */
>  	usleep_range(rdev->desc->off_on_delay, rdev->desc->off_on_delay + 60);
>  
> -	mutex_unlock(&sreg->enable_mutex);
> +	mutex_unlock(&priv->enable_mutex);
>  
>  	return ret;
>  }
> @@ -225,7 +228,7 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
>  {
>  	struct device *pmic_dev = pdev->dev.parent;
>  	struct regulator_config config = { };
> -	struct hi6421_spmi_reg_info *sreg;
> +	struct hi6421_spmi_reg_priv *priv;
>  	struct hi6421_spmi_reg_info *info;
>  	struct device *dev = &pdev->dev;
>  	struct hi6421_spmi_pmic *pmic;
> @@ -241,17 +244,18 @@ static int hi6421_spmi_regulator_probe(struct platform_device *pdev)
>  	if (WARN_ON(!pmic))
>  		return -ENODEV;
>  
> -	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
> -	if (!sreg)
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
>  
> -	mutex_init(&sreg->enable_mutex);
> +	mutex_init(&priv->enable_mutex);
> +	platform_set_drvdata(pdev, priv);
>  
>  	for (i = 0; i < ARRAY_SIZE(regulator_info); i++) {
>  		info = &regulator_info[i];
>  
>  		config.dev = pdev->dev.parent;

This is the main problem of this patch. See, pdev->dev.parent is
actually the device's parent (e. g. the SPMI controller's device).

Looking at (devm_)regulator_register() implementation, it uses it to
store the rdev->dev.parent:

	struct regulator_dev *
	regulator_register(const struct regulator_desc *regulator_desc,
			   const struct regulator_config *cfg)
	{
...

	dev = cfg->dev;
...
	rdev->dev.parent = dev;

So, while you used platform_set_drvdata() to set the data for the
regulator's platform driver, when the driver tries to access the
mutex, the code does, instead:

	static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
	{
		struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev);
		struct hi6421_spmi_reg_priv *priv;
	 	int ret;
	 
		priv = dev_get_drvdata(rdev->dev.parent);

		mutex_lock(&priv->enable_mutex);

At this point, priv will be set to the value of the SPMI controller
dev data, instead of pointing to the area stored via
platform_set_drvdata().

As the data stored there is not the enable_mutex, a call to
mutex_lock() makes the device to hang at boot time (or to cause some
other random issue, as it is using a different memory than what
it was expected).

Thanks,
Mauro