Re: [PATCH] mfd: wcd934x: set DMA mask on parent to silence "DMA mask not set"

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] NULL pointer dereference in error path when of_irq_get() fails.
- [High] Memory and resource leak during SLIMbus status transitions.
- [High] Regulator references are permanently leaked on unbind and probe errors.
- [High] Regulators are disabled before child devices are removed, risking bus timeouts and crashes.
--

--- Patch [1]: [PATCH] mfd: wcd934x: set DMA mask on parent to silence "DMA mask not set" ---
commit 37f997595db7b92153ec4714307685274e9c64de
Author: David Heidelberg <[email protected]>

mfd: wcd934x: set DMA mask on parent to silence "DMA mask not set"

The wcd934x MFD parent (a slim_device) never initializes its
dma_mask/coherent_dma_mask. When the DT-aware children pass through
of_dma_configure() this triggers the "DMA mask not set" warning for
each child.

Cc: [email protected]
Fixes: f959dcd6ddfd ("dma-direct: Fix potential NULL pointer dereference")
Assisted-by: tencent:hy3

> diff --git a/drivers/mfd/wcd934x.c b/drivers/mfd/wcd934x.c
> index 3c3080e8c8cf7..70ee13791d52b 100644
> --- a/drivers/mfd/wcd934x.c
> +++ b/drivers/mfd/wcd934x.c

[ ... ]

> @@ -217,6 +218,11 @@ static int wcd934x_slim_probe(struct slim_device *sdev)
>  	struct gpio_desc *reset_gpio;
>  	int ret;
>  
> +	if (!dev->dma_mask) {
> +		dev->dma_mask = &dev->coherent_dma_mask;
> +		dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +	}
> +
>  	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
>  	if (!ddata)
>  		return	-ENOMEM;

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
if of_irq_get() fails?

When ddata is allocated here, ddata->dev is not yet initialized (it is
assigned later in the probe function). If the immediate next step fails:

wcd934x_slim_probe() {
    ...
    ddata->irq = of_irq_get(np, 0);
    if (ddata->irq < 0)
        return dev_err_probe(ddata->dev, ddata->irq,
                             "Failed to get IRQ\n");
    ...
}

Would passing the NULL ddata->dev to dev_err_probe() cause a crash when it
attempts to record the deferred probe reason?

[Severity: High]
This is a pre-existing issue, but does this code permanently leak regulator
references on probe failure or device unbind?

The regulators are acquired manually without a managed devres API:

wcd934x_slim_probe() {
    ...
    ret = regulator_bulk_get(dev, WCD934X_MAX_SUPPLY, ddata->supplies);
    if (ret)
        return dev_err_probe(dev, ret, "Failed to get supplies\n");
    ...
}

However, the error path and wcd934x_slim_remove() only call
regulator_bulk_disable(). Should there be a matching regulator_bulk_free()
call, or should regulator_bulk_get() be replaced with
devm_regulator_bulk_get()?

[Severity: High]
This is a pre-existing issue, but is there a risk of bus timeouts during
driver unbind?

In the remove callback, regulators are disabled before the child devices
are removed:

wcd934x_slim_remove() {
    ...
    regulator_bulk_disable(WCD934X_MAX_SUPPLY, ddata->supplies);
    mfd_remove_devices(&sdev->dev);
    ...
}

If active child devices (like the codec or SoundWire) attempt to cleanly shut
down by writing to hardware registers during their own remove callbacks,
will they hang or time out because the power has already been cut?

[Severity: High]
This is a pre-existing issue, but does the SLIMbus status handling leak memory
and stack devres allocations?

When the device transitions to SLIM_DEVICE_STATUS_UP, a regmap is manually
allocated and a managed IRQ chip is registered:

wcd934x_slim_status_up() {
    ...
    ddata->regmap = regmap_init_slimbus(sdev, &wcd934x_regmap_config);
    ...
}

On transition to SLIM_DEVICE_STATUS_DOWN, mfd_remove_devices() is called, but
regmap_exit() is never called to free the regmap:

wcd934x_slim_status() {
    ...
    case SLIM_DEVICE_STATUS_DOWN:
        mfd_remove_devices(&sdev->dev);
        break;
    ...
}

Furthermore, since devm_regmap_add_irq_chip() is used during the UP
transition, will repeated UP/DOWN toggles continually accumulate IRQ chip
allocations on the device's devres list without freeing them?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1