Re: [PATCH v3 2/2] ASoC: codecs: add TAS2557 mono smart amplifier driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unreserved secondary I2C addresses and data race on `client->addr` via single shared regmap.
--

--- Patch [2]: [PATCH v3 2/2] ASoC: codecs: add TAS2557 mono smart amplifier driver ---
commit 6ad2a6c10e4ed5ed98d85d212f482c1f0987454d
Author: Gianluca Boiano <morf3089@gmail.com>

ASoC: codecs: add TAS2557 mono smart amplifier driver

This commit adds support for the TAS2557 mono smart amplifier. It uses a
shared regmap architecture to manage up to two physical chips by dynamically
retargeting the I2C client address before each transfer.

> diff --git a/sound/soc/codecs/tas2557.c b/sound/soc/codecs/tas2557.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/codecs/tas2557.c
> @@ -331,12 +331,12 @@ static int tas2557_change_book_page(struct tas2557_priv *tas2557,
>  				    unsigned int chn,
>  				    unsigned char book, unsigned char page)
>  {
>  	struct i2c_client *client = to_i2c_client(tas2557->dev);
>  	struct tas2557_dev *dv = &tas2557->devs[chn];
>  	int ret;
>  
>  	if (client->addr != dv->addr) {
>  		client->addr = dv->addr;
>  
>  		ret = regmap_write(tas2557->regmap, TAS2557_PAGE_REG, 0);

[Severity: High]
Can this mutation of client->addr cause a data race with concurrent regmap
accesses? 

If userspace accesses regmap debugfs concurrently, it acquires the regmap's
internal mutex to perform I2C transfers. Because client->addr is modified
here under dev_lock but before acquiring the regmap lock, could this result
in debugfs I2C transactions inadvertently hitting the wrong physical device?

[ ... ]

> @@ -2875,13 +2875,13 @@ static int tas2557_parse_dt(struct tas2557_priv *tas2557, struct i2c_client *client)
>  {
>  	struct device *dev = tas2557->dev;
>  	struct device_node *np = dev->of_node;
>  	unsigned int imon_slots[TAS2557_MAX_DEV] = { 0, 0 };
>  	unsigned int vmon_slots[TAS2557_MAX_DEV] = { 2, 2 };
>  	unsigned int i;
>  	int ndev = 0;
>  
>  	if (np) {
>  		u64 addr;
>  
>  		for (i = 0; i < TAS2557_MAX_DEV; i++) {
>  			if (of_property_read_reg(np, i, &addr, NULL))
>  				break;
>  			tas2557->devs[ndev++].addr = addr;
>  		}
>  	}

[Severity: High]
Are these secondary I2C addresses left unreserved with the I2C subsystem?

Since there is no call to i2c_new_dummy_device() or a similar API to claim
the additional addresses parsed from the reg array, is it possible that
another driver or a userspace process could accidentally bind to them and
cause bus collisions?

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