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

[Bjorn Andersson]

On Thu, Mar 19, 2026 at 03:11:54PM +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.
> > 
> > Fixes: 6ac7e4d7ad70 ("mfd: wcd934x: Add support to wcd9340/wcd9341 codec")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxxxx>
> > ---
> >  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.
> 

The intent is that slim_device_update_status() will only be called
between a successful probe() and the invocation of remove().

The case where this I'm still trying to convince myself about is if
we have a race between removal of the grandparent controller and the
restart of the audio DSP; but that should be addressed in the slimbus
framework and not here.

Regards,
Bjorn

> > +	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 [李琼斯]