[PATCH] mfd: wcd934x: Unroll regmap and irqchip on removal

[PATCH] mfd: wcd934x: Unroll regmap and irqchip on removal

[Bjorn Andersson]

When the slimbus-up event is handled a new regmap is created, an IRQ
chip is registered on this regmap and then the MFD devices are added.

But the regmap is left dangling if either any of those operations are
failing or if the slimbus-down event ever comes. Which manifest itself
as an error print from debugfs once the next slimbus-up event happens.

Likewise, if for some reason a slimbus-down event would be followed by
a slimbus-up event without the MFD being torn down by the slimbus
controller inbetween, we're going to have a dangling irq_chip.

Add cleanup of the registered resources on failure and on removal.

Fixes: 6ac7e4d7ad70 ("mfd: wcd934x: Add support to wcd9340/wcd9341 codec")
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/mfd/wcd934x.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/wcd934x.c b/drivers/mfd/wcd934x.c
index 3c3080e8c8cf7ecaaa62e255c7e01a850e65e9ad..b03cc91cc3a6a114a34efdb278420ae3dfa016eb 100644
--- a/drivers/mfd/wcd934x.c
+++ b/drivers/mfd/wcd934x.c
@@ -170,29 +170,56 @@ static int wcd934x_slim_status_up(struct slim_device *sdev)
 	ret = wcd934x_bring_up(ddata);
 	if (ret) {
 		dev_err(dev, "Failed to bring up WCD934X: err = %d\n", ret);
-		return ret;
+		goto err_regmap_exit;
 	}
 
-	ret = devm_regmap_add_irq_chip(dev, ddata->regmap, ddata->irq,
-				       IRQF_TRIGGER_HIGH, 0,
-				       &wcd934x_regmap_irq_chip,
-				       &ddata->irq_data);
+	ret = regmap_add_irq_chip(ddata->regmap, ddata->irq,
+				  IRQF_TRIGGER_HIGH, 0,
+				  &wcd934x_regmap_irq_chip,
+				  &ddata->irq_data);
 	if (ret) {
 		dev_err(dev, "Failed to add IRQ chip: err = %d\n", ret);
-		return ret;
+		goto err_regmap_exit;
 	}
 
 	ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, wcd934x_devices,
 			      ARRAY_SIZE(wcd934x_devices), NULL, 0, NULL);
 	if (ret) {
-		dev_err(dev, "Failed to add child devices: err = %d\n",
-			ret);
-		return ret;
+		dev_err(dev, "Failed to add child devices: err = %d\n", ret);
+		goto err_del_irq_chip;
 	}
 
+	return 0;
+
+err_del_irq_chip:
+	regmap_del_irq_chip(ddata->irq, ddata->irq_data);
+	ddata->irq_data = NULL;
+err_regmap_exit:
+	regmap_exit(ddata->regmap);
+	ddata->regmap = NULL;
 	return ret;
 }
 
+static void wcd934x_slim_status_down(struct slim_device *sdev)
+{
+	struct device *dev = &sdev->dev;
+	struct wcd934x_ddata *ddata;
+
+	ddata = dev_get_drvdata(dev);
+
+	mfd_remove_devices(&sdev->dev);
+
+	if (ddata->irq_data) {
+		regmap_del_irq_chip(ddata->irq, ddata->irq_data);
+		ddata->irq_data = NULL;
+	}
+
+	if (ddata->regmap) {
+		regmap_exit(ddata->regmap);
+		ddata->regmap = NULL;
+	}
+}
+
 static int wcd934x_slim_status(struct slim_device *sdev,
 			       enum slim_device_status status)
 {
@@ -200,7 +227,7 @@ static int wcd934x_slim_status(struct slim_device *sdev,
 	case SLIM_DEVICE_STATUS_UP:
 		return wcd934x_slim_status_up(sdev);
 	case SLIM_DEVICE_STATUS_DOWN:
-		mfd_remove_devices(&sdev->dev);
+		wcd934x_slim_status_down(sdev);
 		break;
 	default:
 		return -EINVAL;
@@ -276,7 +303,7 @@ static void wcd934x_slim_remove(struct slim_device *sdev)
 	struct wcd934x_ddata *ddata = dev_get_drvdata(&sdev->dev);
 
 	regulator_bulk_disable(WCD934X_MAX_SUPPLY, ddata->supplies);
-	mfd_remove_devices(&sdev->dev);
+	wcd934x_slim_status_down(sdev);
 }
 
 static const struct slim_device_id wcd934x_slim_id[] = {

---
base-commit: a0ae2a256046c0c5d3778d1a194ff2e171f16e5f
change-id: 20260309-wcd934x-unroll-regmap-f9e08834f9d7

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Re: [PATCH] mfd: wcd934x: Unroll regmap and irqchip on removal

[Lee Jones]

On Mon, 09 Mar 2026, Bjorn Andersson wrote:

> When the slimbus-up event is handled a new regmap is created, an IRQ
> chip is registered on this regmap and then the MFD devices are added.
> 
> But the regmap is left dangling if either any of those operations are
> failing or if the slimbus-down event ever comes. Which manifest itself
> as an error print from debugfs once the next slimbus-up event happens.
> 
> Likewise, if for some reason a slimbus-down event would be followed by
> a slimbus-up event without the MFD being torn down by the slimbus
> controller inbetween, we're going to have a dangling irq_chip.
> 
> Add cleanup of the registered resources on failure and on removal.

I don't understand.  Why isn't devm_* working here?

And if you want to force-remove, why not devm_regmap_del_irq_chip() in
the error path instead?  If you end up using this, then you could go the
other way (the preferred method) and use devm_mfd_add_devices().

> Fixes: 6ac7e4d7ad70 ("mfd: wcd934x: Add support to wcd9340/wcd9341 codec")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/mfd/wcd934x.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mfd/wcd934x.c b/drivers/mfd/wcd934x.c
> index 3c3080e8c8cf7ecaaa62e255c7e01a850e65e9ad..b03cc91cc3a6a114a34efdb278420ae3dfa016eb 100644
> --- a/drivers/mfd/wcd934x.c
> +++ b/drivers/mfd/wcd934x.c
> @@ -170,29 +170,56 @@ static int wcd934x_slim_status_up(struct slim_device *sdev)
>  	ret = wcd934x_bring_up(ddata);
>  	if (ret) {
>  		dev_err(dev, "Failed to bring up WCD934X: err = %d\n", ret);
> -		return ret;
> +		goto err_regmap_exit;
>  	}
>  
> -	ret = devm_regmap_add_irq_chip(dev, ddata->regmap, ddata->irq,
> -				       IRQF_TRIGGER_HIGH, 0,
> -				       &wcd934x_regmap_irq_chip,
> -				       &ddata->irq_data);
> +	ret = regmap_add_irq_chip(ddata->regmap, ddata->irq,
> +				  IRQF_TRIGGER_HIGH, 0,
> +				  &wcd934x_regmap_irq_chip,
> +				  &ddata->irq_data);
>  	if (ret) {
>  		dev_err(dev, "Failed to add IRQ chip: err = %d\n", ret);
> -		return ret;
> +		goto err_regmap_exit;
>  	}
>  
>  	ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, wcd934x_devices,
>  			      ARRAY_SIZE(wcd934x_devices), NULL, 0, NULL);
>  	if (ret) {
> -		dev_err(dev, "Failed to add child devices: err = %d\n",
> -			ret);
> -		return ret;
> +		dev_err(dev, "Failed to add child devices: err = %d\n", ret);
> +		goto err_del_irq_chip;
>  	}
>  
> +	return 0;
> +
> +err_del_irq_chip:
> +	regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> +	ddata->irq_data = NULL;
> +err_regmap_exit:
> +	regmap_exit(ddata->regmap);
> +	ddata->regmap = NULL;
>  	return ret;
>  }
>  
> +static void wcd934x_slim_status_down(struct slim_device *sdev)
> +{
> +	struct device *dev = &sdev->dev;
> +	struct wcd934x_ddata *ddata;
> +
> +	ddata = dev_get_drvdata(dev);
> +
> +	mfd_remove_devices(&sdev->dev);
> +
> +	if (ddata->irq_data) {
> +		regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> +		ddata->irq_data = NULL;
> +	}
> +
> +	if (ddata->regmap) {
> +		regmap_exit(ddata->regmap);
> +		ddata->regmap = NULL;
> +	}
> +}
> +
>  static int wcd934x_slim_status(struct slim_device *sdev,
>  			       enum slim_device_status status)
>  {
> @@ -200,7 +227,7 @@ static int wcd934x_slim_status(struct slim_device *sdev,
>  	case SLIM_DEVICE_STATUS_UP:
>  		return wcd934x_slim_status_up(sdev);
>  	case SLIM_DEVICE_STATUS_DOWN:
> -		mfd_remove_devices(&sdev->dev);
> +		wcd934x_slim_status_down(sdev);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -276,7 +303,7 @@ static void wcd934x_slim_remove(struct slim_device *sdev)
>  	struct wcd934x_ddata *ddata = dev_get_drvdata(&sdev->dev);
>  
>  	regulator_bulk_disable(WCD934X_MAX_SUPPLY, ddata->supplies);
> -	mfd_remove_devices(&sdev->dev);
> +	wcd934x_slim_status_down(sdev);
>  }
>  
>  static const struct slim_device_id wcd934x_slim_id[] = {
> 
> ---
> base-commit: a0ae2a256046c0c5d3778d1a194ff2e171f16e5f
> change-id: 20260309-wcd934x-unroll-regmap-f9e08834f9d7
> 
> Best regards,
> -- 
> Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH] mfd: wcd934x: Unroll regmap and irqchip on removal

[Bjorn Andersson]

On Tue, Mar 10, 2026 at 09:13:16AM +0000, Lee Jones wrote:
> On Mon, 09 Mar 2026, Bjorn Andersson wrote:
> 
> > When the slimbus-up event is handled a new regmap is created, an IRQ
> > chip is registered on this regmap and then the MFD devices are added.
> > 
> > But the regmap is left dangling if either any of those operations are
> > failing or if the slimbus-down event ever comes. Which manifest itself
> > as an error print from debugfs once the next slimbus-up event happens.
> > 
> > Likewise, if for some reason a slimbus-down event would be followed by
> > a slimbus-up event without the MFD being torn down by the slimbus
> > controller inbetween, we're going to have a dangling irq_chip.
> > 
> > Add cleanup of the registered resources on failure and on removal.
> 
> I don't understand.  Why isn't devm_* working here?
> 

devm_ "works", but there are two different ends to the life cycle; the
primary is a call to wcd934x_slim_status() with SLIM_DEVICE_STATUS_DOWN,
which might be followed by another SLIM_DEVICE_STATUS_UP. The other is
the removal of the device while it's still in "up state" (although it
seems this shouldn't happen).

For the latter devres makes some sense - although I prefer to keep
devres to codepaths rooted in probe().

> And if you want to force-remove, why not devm_regmap_del_irq_chip() in
> the error path instead?

Because the primary end of the life cycle for these allocations aren't
the removal of the device, it's the SLIM_DEVICE_STATUS_DOWN event.

The whole purpose with devres is to simplify the code, if we need to
manually handle the resources anyway they are just a cognitive burden.

> If you end up using this, then you could go the
> other way (the preferred method) and use devm_mfd_add_devices().
> 

mfd_add_devices() and mfd_remove_devices() does not follow the device's
life cycle, so that wouldn't be helpful.

Perhaps the slimbus stack can be redesigned so that these resources
follow the device's life cycle? I don't understand it well enough to
say at this point.
I would argue that it would still be better to fix the memory and
debugfs leaks before making such changes anyways.

Regards,
Bjorn

> > Fixes: 6ac7e4d7ad70 ("mfd: wcd934x: Add support to wcd9340/wcd9341 codec")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> >  drivers/mfd/wcd934x.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 38 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mfd/wcd934x.c b/drivers/mfd/wcd934x.c
> > index 3c3080e8c8cf7ecaaa62e255c7e01a850e65e9ad..b03cc91cc3a6a114a34efdb278420ae3dfa016eb 100644
> > --- a/drivers/mfd/wcd934x.c
> > +++ b/drivers/mfd/wcd934x.c
> > @@ -170,29 +170,56 @@ static int wcd934x_slim_status_up(struct slim_device *sdev)
> >  	ret = wcd934x_bring_up(ddata);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to bring up WCD934X: err = %d\n", ret);
> > -		return ret;
> > +		goto err_regmap_exit;
> >  	}
> >  
> > -	ret = devm_regmap_add_irq_chip(dev, ddata->regmap, ddata->irq,
> > -				       IRQF_TRIGGER_HIGH, 0,
> > -				       &wcd934x_regmap_irq_chip,
> > -				       &ddata->irq_data);
> > +	ret = regmap_add_irq_chip(ddata->regmap, ddata->irq,
> > +				  IRQF_TRIGGER_HIGH, 0,
> > +				  &wcd934x_regmap_irq_chip,
> > +				  &ddata->irq_data);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to add IRQ chip: err = %d\n", ret);
> > -		return ret;
> > +		goto err_regmap_exit;
> >  	}
> >  
> >  	ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, wcd934x_devices,
> >  			      ARRAY_SIZE(wcd934x_devices), NULL, 0, NULL);
> >  	if (ret) {
> > -		dev_err(dev, "Failed to add child devices: err = %d\n",
> > -			ret);
> > -		return ret;
> > +		dev_err(dev, "Failed to add child devices: err = %d\n", ret);
> > +		goto err_del_irq_chip;
> >  	}
> >  
> > +	return 0;
> > +
> > +err_del_irq_chip:
> > +	regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> > +	ddata->irq_data = NULL;
> > +err_regmap_exit:
> > +	regmap_exit(ddata->regmap);
> > +	ddata->regmap = NULL;
> >  	return ret;
> >  }
> >  
> > +static void wcd934x_slim_status_down(struct slim_device *sdev)
> > +{
> > +	struct device *dev = &sdev->dev;
> > +	struct wcd934x_ddata *ddata;
> > +
> > +	ddata = dev_get_drvdata(dev);
> > +
> > +	mfd_remove_devices(&sdev->dev);
> > +
> > +	if (ddata->irq_data) {
> > +		regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> > +		ddata->irq_data = NULL;
> > +	}
> > +
> > +	if (ddata->regmap) {
> > +		regmap_exit(ddata->regmap);
> > +		ddata->regmap = NULL;
> > +	}
> > +}
> > +
> >  static int wcd934x_slim_status(struct slim_device *sdev,
> >  			       enum slim_device_status status)
> >  {
> > @@ -200,7 +227,7 @@ static int wcd934x_slim_status(struct slim_device *sdev,
> >  	case SLIM_DEVICE_STATUS_UP:
> >  		return wcd934x_slim_status_up(sdev);
> >  	case SLIM_DEVICE_STATUS_DOWN:
> > -		mfd_remove_devices(&sdev->dev);
> > +		wcd934x_slim_status_down(sdev);
> >  		break;
> >  	default:
> >  		return -EINVAL;
> > @@ -276,7 +303,7 @@ static void wcd934x_slim_remove(struct slim_device *sdev)
> >  	struct wcd934x_ddata *ddata = dev_get_drvdata(&sdev->dev);
> >  
> >  	regulator_bulk_disable(WCD934X_MAX_SUPPLY, ddata->supplies);
> > -	mfd_remove_devices(&sdev->dev);
> > +	wcd934x_slim_status_down(sdev);
> >  }
> >  
> >  static const struct slim_device_id wcd934x_slim_id[] = {
> > 
> > ---
> > base-commit: a0ae2a256046c0c5d3778d1a194ff2e171f16e5f
> > change-id: 20260309-wcd934x-unroll-regmap-f9e08834f9d7
> > 
> > Best regards,
> > -- 
> > Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > 
> 
> -- 
> Lee Jones [李琼斯]

Re: [PATCH] mfd: wcd934x: Unroll regmap and irqchip on removal

[Lee Jones]

On Mon, 09 Mar 2026, Bjorn Andersson wrote:

> When the slimbus-up event is handled a new regmap is created, an IRQ
> chip is registered on this regmap and then the MFD devices are added.
> 
> But the regmap is left dangling if either any of those operations are
> failing or if the slimbus-down event ever comes. Which manifest itself
> as an error print from debugfs once the next slimbus-up event happens.
> 
> Likewise, if for some reason a slimbus-down event would be followed by
> a slimbus-up event without the MFD being torn down by the slimbus
> controller inbetween, we're going to have a dangling irq_chip.
> 
> Add cleanup of the registered resources on failure and on removal.
> 
> Fixes: 6ac7e4d7ad70 ("mfd: wcd934x: Add support to wcd9340/wcd9341 codec")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/mfd/wcd934x.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mfd/wcd934x.c b/drivers/mfd/wcd934x.c
> index 3c3080e8c8cf7ecaaa62e255c7e01a850e65e9ad..b03cc91cc3a6a114a34efdb278420ae3dfa016eb 100644
> --- a/drivers/mfd/wcd934x.c
> +++ b/drivers/mfd/wcd934x.c
> @@ -170,29 +170,56 @@ static int wcd934x_slim_status_up(struct slim_device *sdev)
>  	ret = wcd934x_bring_up(ddata);
>  	if (ret) {
>  		dev_err(dev, "Failed to bring up WCD934X: err = %d\n", ret);
> -		return ret;
> +		goto err_regmap_exit;
>  	}
>  
> -	ret = devm_regmap_add_irq_chip(dev, ddata->regmap, ddata->irq,
> -				       IRQF_TRIGGER_HIGH, 0,
> -				       &wcd934x_regmap_irq_chip,
> -				       &ddata->irq_data);
> +	ret = regmap_add_irq_chip(ddata->regmap, ddata->irq,
> +				  IRQF_TRIGGER_HIGH, 0,
> +				  &wcd934x_regmap_irq_chip,
> +				  &ddata->irq_data);
>  	if (ret) {
>  		dev_err(dev, "Failed to add IRQ chip: err = %d\n", ret);
> -		return ret;
> +		goto err_regmap_exit;
>  	}
>  
>  	ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, wcd934x_devices,
>  			      ARRAY_SIZE(wcd934x_devices), NULL, 0, NULL);
>  	if (ret) {
> -		dev_err(dev, "Failed to add child devices: err = %d\n",
> -			ret);
> -		return ret;
> +		dev_err(dev, "Failed to add child devices: err = %d\n", ret);
> +		goto err_del_irq_chip;
>  	}
>  
> +	return 0;
> +
> +err_del_irq_chip:
> +	regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> +	ddata->irq_data = NULL;
> +err_regmap_exit:
> +	regmap_exit(ddata->regmap);
> +	ddata->regmap = NULL;
>  	return ret;
>  }
>  
> +static void wcd934x_slim_status_down(struct slim_device *sdev)
> +{
> +	struct device *dev = &sdev->dev;
> +	struct wcd934x_ddata *ddata;
> +
> +	ddata = dev_get_drvdata(dev);

Is this guaranteed to be populated?

If this fails for any reason, we'll nul-ptr-deref below.

> +	mfd_remove_devices(&sdev->dev);
> +
> +	if (ddata->irq_data) {
> +		regmap_del_irq_chip(ddata->irq, ddata->irq_data);
> +		ddata->irq_data = NULL;
> +	}
> +
> +	if (ddata->regmap) {
> +		regmap_exit(ddata->regmap);
> +		ddata->regmap = NULL;
> +	}
> +}

[...]

-- 
Lee Jones [李琼斯]