Re: [PATCH v3 2/2] ASoC: codecs: Add support for Fortemedia FM34NE DSP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] ASoC: fm34ne-dsp: mixing guard() and goto cleanup in fm34ne_dsp_set_config()
--

commit 49a97b167b8daf22a65a613318793960938a84f4
Author: Svyatoslav Ryhel <clamor95@gmail.com>

ASoC: codecs: Add support for Fortemedia FM34NE DSP

This commit adds an ASoC codec driver for the Fortemedia FM34NE digital sound
processing chip used for active noise suppression on ASUS Transformer
devices.

> diff --git a/sound/soc/codecs/fm34ne-dsp.c b/sound/soc/codecs/fm34ne-dsp.c
> new file mode 100644
> index 0000000000000..07f22507a53df
> --- /dev/null
> +++ b/sound/soc/codecs/fm34ne-dsp.c
> @@ -0,0 +1,386 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <sound/core.h>
> +#include <sound/soc.h>
> +
> +#include "fm34ne-dsp.h"
> +
> +struct fm34ne_dsp {
> +	struct i2c_client *client;
> +
> +	struct gpio_desc *bypass_gpio;
> +	struct gpio_desc *reset_gpio;
> +
> +	struct clk *dap_mclk;
> +	struct regulator *vdd_supply;
> +	struct mutex lock; /* prevent simultaneous parameter writing */
> +
> +	const struct fm34ne_dsp_info *info;
> +};
> +
> +static int fm34ne_dsp_set_config(struct fm34ne_dsp *dsp, enum dsp_state state)
> +{
> +	struct device *dev = &dsp->client->dev;
> +	int ret;
> +
> +	guard(mutex)(&dsp->lock);
> +
> +	gpiod_set_value_cansleep(dsp->bypass_gpio, 1);
> +	msleep(20);
> +
> +	switch (state) {
> +	case FM34NE_NS_ENABLE:
> +		ret = i2c_master_send(dsp->client, dsp->info->enable_parameter,
> +				      dsp->info->enable_parameter_length);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set DSP enable %d\n", ret);
> +			goto exit;
> +		}
> +
> +		ret = i2c_master_send(dsp->client, dsp->info->enable_noise_suppression,
> +				      dsp->info->enable_ns_length);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable DSP noise suppression %d\n", ret);
> +			goto exit;
> +		}
> +
> +		break;
> +
> +	case FM34NE_NS_DISABLE:
> +		ret = i2c_master_send(dsp->client, dsp->info->enable_parameter,
> +				      dsp->info->enable_parameter_length);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set DSP enable with %d\n", ret);
> +			goto exit;
> +		}
> +
> +		ret = i2c_master_send(dsp->client, dsp->info->disable_noise_suppression,
> +				      dsp->info->disable_ns_length);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to disable DSP noise suppression with %d\n", ret);
> +			goto exit;
> +		}
> +
> +		break;
> +
> +	case FM34NE_BYPASS:
> +	default:
> +		ret = i2c_master_send(dsp->client, bypass_parameter, sizeof(bypass_parameter));
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set DSP bypass with %d\n", ret);
> +			goto exit;
> +		}
> +
> +		break;
> +	}
> +
> +exit:
> +	gpiod_set_value_cansleep(dsp->bypass_gpio, 0);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

[Severity: Low]
In fm34ne_dsp_set_config(), dsp->lock is acquired via guard(mutex)(&dsp->lock)
while i2c_master_send() failure paths jump to the exit label using goto exit
to deassert dsp->bypass_gpio.

The cleanup rules in include/linux/cleanup.h state that goto-based cleanup
and scope-based cleanup helpers like guard() should not be mixed in the same
function:

 * Lastly, given that the benefit of cleanup helpers is removal of
 * "goto", and that the "goto" statement can jump between scopes, the
 * expectation is that usage of "goto" and cleanup helpers is never
 * mixed in the same function. I.e. for a given routine, convert all
 * resources that need a "goto" cleanup to scope-based cleanup, or
 * convert none of them.

Also, sound/soc/codecs/fm34ne-dsp.c uses guard(mutex) and struct mutex
without explicitly including linux/cleanup.h and linux/mutex.h.

Could fm34ne_dsp_set_config() avoid mixing guard(mutex) with goto exit
(for example by using break from the switch statement or a helper function
for the I2C transfers), and include linux/cleanup.h and linux/mutex.h
explicitly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027113929.65102-1-clamor95@gmail.com?part=2