[PATCH v1] ASoC: tas2783-sdw: Add basic BPT support

[PATCH v1] ASoC: tas2783-sdw: Add basic BPT support

[Baojun Xu]

Add for BPT supported.

Signed-off-by: Baojun Xu <baojun.xu@ti.com>
---
 sound/soc/codecs/tas2783-sdw.c | 188 ++++++++++++++++++++++++++++++---
 1 file changed, 176 insertions(+), 12 deletions(-)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index a57230e01f75..c1c74707b633 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -47,6 +47,26 @@
 #define TAS2783_CALI_GUID EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, \
 				   0x09, 0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92)
 
+#define TAS2783_MAX_FW_SECTIONS	256
+
+enum {
+	SDW_MSG_FLAG_READ = 0,
+	SDW_MSG_FLAG_WRITE,
+};
+
+struct sdw_bpt_section {
+	u32 addr;
+	u32 len;
+	u8 *buf;
+};
+
+struct sdw_bpt_msg {
+	struct sdw_bpt_section *sec;
+	int sections;
+	u8 dev_num;
+	u8 flags;
+};
+
 static const u32 tas2783_cali_reg[] = {
 	TAS2783_CAL_R0,
 	TAS2783_CAL_INVR0,
@@ -738,7 +758,103 @@ static s32 tas_fw_get_next_file(const u8 *data, struct tas_fw_file *file)
 	file->dest_addr = get_unaligned_le32(&data[16]);
 	file->fw_data = (u8 *)&data[20];
 
-	return file->length + sizeof(u32) * 5;
+	return file->length + FW_FL_HDR;
+}
+
+static int tas2783_fw_download_bpt(struct tas2783_prv *tas_dev,
+				   const struct firmware *fmw)
+{
+	struct sdw_slave *slave = tas_dev->sdw_peripheral;
+	struct sdw_bpt_section *sections = NULL;
+	struct sdw_bus *bus = slave->bus;
+	struct sdw_bpt_msg msg = {0};
+	struct tas_fw_file file;
+	struct tas_fw_hdr hdr;
+	size_t offset = FW_DL_OFFSET;
+	size_t img_sz = fmw->size;
+	const u8 *buf = fmw->data;
+	int num_sections = 0;
+	int i, ret;
+
+	if (img_sz < FW_DL_OFFSET)
+		return -EINVAL;
+
+	tas_fw_read_hdr(buf, &hdr);
+	if (hdr.size != img_sz)
+		return -EINVAL;
+
+	while (offset < (img_sz - FW_FL_HDR)) {
+		size_t adv = (size_t)tas_fw_get_next_file(&buf[offset], &file);
+
+		/*
+		 * Reject a section whose payload runs past the image. Compare
+		 * file.length (not adv) against the space left after this
+		 * section's header, so the check is immune to the size_t wrap
+		 * in tas_fw_get_next_file() on 32-bit kernels.
+		 */
+		if (file.length > img_sz - offset - FW_FL_HDR) {
+			dev_err(tas_dev->dev,
+				"Firmware section at offset %zu exceeded.\n",
+				offset);
+			return -EINVAL;
+		}
+		if (num_sections >= TAS2783_MAX_FW_SECTIONS) {
+			dev_err(tas_dev->dev,
+				"BPT fw has too many sections (max %d)\n",
+				TAS2783_MAX_FW_SECTIONS);
+			return -EINVAL;
+		}
+		offset += adv;
+		num_sections++;
+	}
+
+	if (num_sections == 0)
+		return -EINVAL;
+
+	sections = kcalloc(num_sections, sizeof(*sections), GFP_KERNEL);
+	if (!sections)
+		return -ENOMEM;
+
+	offset = FW_DL_OFFSET;
+	for (i = 0; i < num_sections && offset < (img_sz - FW_FL_HDR); i++) {
+		size_t adv = (size_t)tas_fw_get_next_file(&buf[offset], &file);
+
+		sections[i].addr = file.dest_addr;
+		sections[i].len = file.length;
+		sections[i].buf = file.fw_data;
+		offset += adv;
+
+		dev_dbg(tas_dev->dev, "BPT section %d: addr=0x%x len=%d",
+			i, file.dest_addr, file.length);
+	}
+
+	msg.sec = sections;
+	msg.sections = num_sections;
+	msg.dev_num = slave->dev_num;
+	msg.flags = SDW_MSG_FLAG_WRITE;
+
+	dev_dbg(tas_dev->dev, "BPT starting: %d sections, dev_num=%d",
+		num_sections, slave->dev_num);
+
+	ret = sdw_bpt_send_sync(bus, slave, &msg);
+
+	if (ret < 0)
+		dev_err(tas_dev->dev, "BPT firmware download failed: %d", ret);
+	else
+		dev_dbg(tas_dev->dev,
+			 "BPT firmware download complete: %d sections",
+			 num_sections);
+
+	kfree(sections);
+	return ret;
+}
+
+static bool tas2783_bpt_supported(struct tas2783_prv *tas_dev)
+{
+	struct sdw_slave *slave = tas_dev->sdw_peripheral;
+	struct sdw_bus *bus = slave->bus;
+
+	return bus && bus->ops && bus->ops->bpt_send_async;
 }
 
 static void tas2783_fw_ready(const struct firmware *fmw, void *context)
@@ -761,7 +877,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 	if (!fmw || !fmw->data) {
 		if (!tas_dev->fw_use_fallback) {
 			tas_dev->fw_use_fallback = true;
-			dev_info(tas_dev->dev,
+			dev_dbg(tas_dev->dev,
 				"Failed to read preferred fw binary: %s, attempting fallback binary load\n",
 				tas_dev->rca_binaryname);
 		} else {
@@ -774,6 +890,30 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 		goto out;
 	}
 
+	/* Try BPT first if supported, fallback to legacy method */
+	dev_dbg(tas_dev->dev, "BPT check: async=%p wait=%p",
+		 tas_dev->sdw_peripheral->bus->ops->bpt_send_async,
+		 tas_dev->sdw_peripheral->bus->ops->bpt_wait);
+	if (tas2783_bpt_supported(tas_dev)) {
+		ret = tas2783_fw_download_bpt(tas_dev, fmw);
+
+		if (ret == 0) {
+			dev_dbg(tas_dev->dev, "Firmware downloaded via BPT");
+			cur_file = 1;
+			goto calibrate;
+		}
+		/*
+		 * -ESHUTDOWN means the manager is clock-stopping the bus for
+		 * system suspend and refused the transfer. The legacy fallback
+		 * would only thrash the dying bus with command timeouts, so
+		 * bail and let the next attach cycle (on resume) reissue it.
+		 */
+		if (ret == -ESHUTDOWN)
+			goto out;
+		dev_warn(tas_dev->dev,
+			 "BPT failed (%d), falling back to legacy method",
+			 ret);
+	}
 	img_sz = fmw->size;
 	buf = fmw->data;
 	offset += tas_fw_read_hdr(buf, hdr);
@@ -834,6 +974,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 		cur_file++;
 	}
 	mutex_unlock(&tas_dev->pde_lock);
+calibrate:
 	regcache_drop_region(tas_dev->regmap, 0, UINT_MAX);
 
 	if (cur_file == 0) {
@@ -841,6 +982,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 		ret = -EINVAL;
 	} else {
 		tas2783_update_calibdata(tas_dev);
+		regmap_write(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 7), 0x22);
 		ret = regmap_bulk_read(tas_dev->regmap, TAS2783_FW_VERSION,
 					&val, 4);
 		tas_dev->fw_version = (val[0] << 24) | (val[1] << 16) |
@@ -1216,25 +1358,32 @@ static s32 tas_fw_load(struct tas2783_prv *tas_dev, struct sdw_slave *slave)
 static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
 {
 	struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
+	uint val;
 	s32 ret;
 
 	if (tas_dev->hw_init)
 		return 0;
 
-	tas_dev->fw_dl_success = false;
-
-	ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
+	regmap_read(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 7), &val);
+	dev_dbg(dev, "Got B0P0R7 = %02X with %d.", val, ret);
 	if (ret) {
-		dev_err(dev, "sw reset failed, err=%d", ret);
+		dev_err(dev, "Get FW download status failed, err=%d", ret);
 		return ret;
 	}
-	usleep_range(2000, 2200);
-
-	tas_dev->fw_use_fallback = false;
-	ret = tas_fw_load(tas_dev, slave);
-	if (!ret && tas_dev->fw_use_fallback)
+	if (val != 0x22) {
+		tas_dev->fw_dl_success = false;
+		ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
+		if (ret) {
+			dev_err(dev, "sw reset failed, err=%d", ret);
+			return ret;
+		}
+		usleep_range(2000, 2200);
+		regcache_cache_only(tas_dev->regmap, false);
+		tas_dev->fw_use_fallback = false;
 		ret = tas_fw_load(tas_dev, slave);
-
+		if (!ret && tas_dev->fw_use_fallback)
+			ret = tas_fw_load(tas_dev, slave);
+	}
 	if (!ret) {
 		if (tas_dev->sa_func_data)
 			ret = sdca_regmap_write_init(dev, tas_dev->regmap,
@@ -1382,6 +1531,21 @@ static s32 tas_sdw_probe(struct sdw_slave *peripheral,
 	if (ret)
 		return dev_err_probe(dev, ret,
 				     "slave property read failed");
+	/*
+	 * BPT firmware download uses DP0 and relies on the SoundWire stream
+	 * framework to prepare/deprepare DP0. Ensure DP0 properties exist for
+	 * the framework prepare/deprepare sequence.
+	 */
+	if (!peripheral->prop.dp0_prop) {
+		peripheral->prop.dp0_prop = devm_kzalloc(dev,
+			sizeof(*peripheral->prop.dp0_prop), GFP_KERNEL);
+		if (!peripheral->prop.dp0_prop)
+			return dev_err_probe(dev, -ENOMEM,
+					     "dp0_prop alloc failed");
+
+		peripheral->prop.dp0_prop->simple_ch_prep_sm = true;
+		peripheral->prop.dp0_prop->ch_prep_timeout = 25;
+	}
 
 	tas_dev = devm_kzalloc(dev, sizeof(*tas_dev), GFP_KERNEL);
 	if (!tas_dev)
-- 
2.25.1

Re: [PATCH v1] ASoC: tas2783-sdw: Add basic BPT support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] ASoC: tas2783-sdw: uninitialized ret in tas_io_init() after regmap_read()
- [High] ASoC: tas2783-sdw: B0P0R7 status marker clobbered and fw_dl_success unset
- [High] ASoC: tas2783-sdw: sdw_dev_lock deadlock in tas2783_fw_download_bpt()
- [Medium] ASoC: tas2783-sdw: duplicate SoundWire BPT struct and flag definitions
- [Medium] ASoC: tas2783-sdw: missing error propagation from tas2783_fw_ready()
- [Medium] ASoC: tas2783-sdw: legacy fallback on malformed firmware in tas2783_fw_ready()
- [Medium] ASoC: tas2783-sdw: dangling dp0_prop pointer after probe failure or unbind
- [Medium] ASoC: tas2783-sdw: DP0 BPT prep in tas_port_prep() powers on PDE23 early
- [Medium] ASoC: tas2783-sdw: uninitialized val read at calibrate in tas2783_fw_ready()
- [Low] ASoC: tas2783-sdw: unchecked bus->ops dereference and missing bpt_wait check
--

commit 34619ca19ed4f9d3a26eb6716aaef0ab7b07df84
Author: Baojun Xu <baojun.xu@ti.com>

ASoC: tas2783-sdw: Add basic BPT support

This commit adds SoundWire Bulk Peripherals Transport (BPT) firmware
download support to the tas2783 codec driver, with a fallback to the
existing sdw_nwrite_no_pm() download path.

> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
> index a57230e01f755..c1c74707b633a 100644
> --- a/sound/soc/codecs/tas2783-sdw.c
> +++ b/sound/soc/codecs/tas2783-sdw.c
> @@ -47,6 +47,26 @@
>  #define TAS2783_CALI_GUID EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, \
>  				   0x09, 0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92)
>  
> +#define TAS2783_MAX_FW_SECTIONS	256
> +
> +enum {
> +	SDW_MSG_FLAG_READ = 0,
> +	SDW_MSG_FLAG_WRITE,
> +};
> +
> +struct sdw_bpt_section {
> +	u32 addr;
> +	u32 len;
> +	u8 *buf;
> +};
> +
> +struct sdw_bpt_msg {
> +	struct sdw_bpt_section *sec;
> +	int sections;
> +	u8 dev_num;
> +	u8 flags;
> +};

[Severity: Medium]
Should struct sdw_bpt_section, struct sdw_bpt_msg, and the
SDW_MSG_FLAG_READ / SDW_MSG_FLAG_WRITE enum be moved from
drivers/soundwire/bus.h to include/linux/soundwire/sdw.h instead of
being duplicated in sound/soc/codecs/tas2783-sdw.c?

Because include/linux/soundwire/sdw.h currently only forward-declares
struct sdw_bpt_msg for sdw_bpt_send_sync(), redefining these types locally
in the codec driver bypasses compiler type checking across the subsystem
boundary and risks layout mismatches or build breakage if the SoundWire
core headers are updated.

> +
>  static const u32 tas2783_cali_reg[] = {
>  	TAS2783_CAL_R0,
>  	TAS2783_CAL_INVR0,
> @@ -738,7 +758,103 @@ static s32 tas_fw_get_next_file(const u8 *data, struct tas_fw_file *file)
>  	file->dest_addr = get_unaligned_le32(&data[16]);
>  	file->fw_data = (u8 *)&data[20];
>  
> -	return file->length + sizeof(u32) * 5;
> +	return file->length + FW_FL_HDR;
> +}
> +
> +static int tas2783_fw_download_bpt(struct tas2783_prv *tas_dev,
> +				   const struct firmware *fmw)
> +{

[ ... ]

> +	msg.sec = sections;
> +	msg.sections = num_sections;
> +	msg.dev_num = slave->dev_num;
> +	msg.flags = SDW_MSG_FLAG_WRITE;
> +
> +	dev_dbg(tas_dev->dev, "BPT starting: %d sections, dev_num=%d",
> +		num_sections, slave->dev_num);
> +
> +	ret = sdw_bpt_send_sync(bus, slave, &msg);

[Severity: High]
Can calling sdw_bpt_send_sync() here deadlock on slave->sdw_dev_lock
until wait_event_timeout() expires in tas_fw_load()?

When the SoundWire core invokes drv->ops->update_status() from
sdw_update_slave_status() or sdw_drv_probe(), it holds
slave->sdw_dev_lock across the entire callback:

  sdw_update_slave_status()
    mutex_lock(&slave->sdw_dev_lock)
    tas_update_status()
      tas_io_init()
        tas_fw_load()
          request_firmware_nowait(..., tas2783_fw_ready)
          wait_event_timeout(tas_dev->fw_wait, tas_dev->fw_dl_task_done, ...)

Meanwhile, in the firmware workqueue thread, sdw_bpt_send_sync() prepares
DP0 for the BPT stream and tries to acquire the same slave->sdw_dev_lock:

  tas2783_fw_ready()
    tas2783_fw_download_bpt()
      sdw_bpt_send_sync()
        sdw_bpt_send_async()
          intel_ace2x_bpt_send_async()
            intel_ace2x_bpt_open_stream()
              sdw_prepare_stream()
                _sdw_prepare_stream()
                  sdw_prep_deprep_ports()
                    sdw_prep_deprep_slave_ports()
                      sdw_do_port_prep()
                        mutex_lock(&slave->sdw_dev_lock)

Because tas_fw_load() is blocked waiting for tas2783_fw_ready() while
holding slave->sdw_dev_lock, sdw_do_port_prep() blocks until
wait_event_timeout() times out and tas_update_status() returns, causing
tas_io_init() to fail with -EAGAIN while tas2783_fw_ready() continues
running in the background.

> +
> +	if (ret < 0)
> +		dev_err(tas_dev->dev, "BPT firmware download failed: %d", ret);
> +	else
> +		dev_dbg(tas_dev->dev,
> +			 "BPT firmware download complete: %d sections",
> +			 num_sections);
> +
> +	kfree(sections);
> +	return ret;
> +}
> +
> +static bool tas2783_bpt_supported(struct tas2783_prv *tas_dev)
> +{
> +	struct sdw_slave *slave = tas_dev->sdw_peripheral;
> +	struct sdw_bus *bus = slave->bus;
> +
> +	return bus && bus->ops && bus->ops->bpt_send_async;
> +}

[ ... ]

> @@ -774,6 +890,30 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
>  		goto out;
>  	}
>  
> +	/* Try BPT first if supported, fallback to legacy method */
> +	dev_dbg(tas_dev->dev, "BPT check: async=%p wait=%p",
> +		 tas_dev->sdw_peripheral->bus->ops->bpt_send_async,
> +		 tas_dev->sdw_peripheral->bus->ops->bpt_wait);
> +	if (tas2783_bpt_supported(tas_dev)) {

[Severity: Low]
Does the dev_dbg() call above dereference tas_dev->sdw_peripheral->bus
and bus->ops before tas2783_bpt_supported() checks whether they are
non-NULL?

Also, since sdw_bpt_send_async() in drivers/soundwire/bus.c requires both
bus->ops->bpt_send_async and bus->ops->bpt_wait to be implemented, should
tas2783_bpt_supported() check bus->ops->bpt_wait as well?

> +		ret = tas2783_fw_download_bpt(tas_dev, fmw);
> +
> +		if (ret == 0) {
> +			dev_dbg(tas_dev->dev, "Firmware downloaded via BPT");
> +			cur_file = 1;
> +			goto calibrate;
> +		}
> +		/*
> +		 * -ESHUTDOWN means the manager is clock-stopping the bus for
> +		 * system suspend and refused the transfer. The legacy fallback
> +		 * would only thrash the dying bus with command timeouts, so
> +		 * bail and let the next attach cycle (on resume) reissue it.
> +		 */
> +		if (ret == -ESHUTDOWN)
> +			goto out;

[Severity: Medium]
When ret == -ESHUTDOWN jumps to out:, tas_dev->fw_dl_success remains
false and tas_dev->fw_dl_task_done is set to true, waking tas_fw_load().
However, in tas_fw_load():

	ret = wait_event_timeout(tas_dev->fw_wait, tas_dev->fw_dl_task_done,
				 msecs_to_jiffies(TIMEOUT_FW_DL_MS));
	if (!ret) {
		dev_err(tas_dev->dev, "fw request, wait_event timeout\n");
		return -EAGAIN;
	}

	return 0;

tas_fw_load() returns 0 without checking tas_dev->fw_dl_success. Doesn't
that cause tas_io_init() to continue issuing sdca_regmap_write_init() or
regmap_multi_reg_write() over the shutting-down bus and set
tas_dev->hw_init = true even though firmware download was aborted?

> +		dev_warn(tas_dev->dev,
> +			 "BPT failed (%d), falling back to legacy method",
> +			 ret);
> +	}

[Severity: Medium]
If tas2783_fw_download_bpt() returns -EINVAL because the firmware image
failed validation (for example, img_sz < FW_DL_OFFSET or a section's
file.length exceeds img_sz - offset - FW_FL_HDR), should
tas2783_fw_ready() abort instead of falling back to the legacy loader?

In the legacy path right below, tas_fw_read_hdr(buf, hdr) reads 84 bytes
before checking if (img_sz < FW_DL_OFFSET), and the while loop passes
file->fw_data and file->length to sdw_nwrite_no_pm() without checking
whether file->length runs past the end of fmw->data.

>  	img_sz = fmw->size;
>  	buf = fmw->data;
>  	offset += tas_fw_read_hdr(buf, hdr);
> @@ -834,6 +974,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
>  		cur_file++;
>  	}
>  	mutex_unlock(&tas_dev->pde_lock);
> +calibrate:
>  	regcache_drop_region(tas_dev->regmap, 0, UINT_MAX);
>  
>  	if (cur_file == 0) {
> @@ -841,6 +982,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
>  		ret = -EINVAL;
>  	} else {
>  		tas2783_update_calibdata(tas_dev);
> +		regmap_write(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 7), 0x22);
>  		ret = regmap_bulk_read(tas_dev->regmap, TAS2783_FW_VERSION,
>  					&val, 4);
>  		tas_dev->fw_version = (val[0] << 24) | (val[1] << 16) |

[Severity: Medium]
When jumping to calibrate: from the BPT path, the earlier
regmap_bulk_read() before the legacy loop is skipped, so val[0..3] has
not been initialized yet. If regmap_bulk_read() here fails, will
tas_dev->fw_version be populated from uninitialized stack memory in val?

Also, if the legacy download loop fails and breaks out with ret < 0 after
downloading at least one section (cur_file >= 1), won't this else branch
still write 0x22 to TASDEV_REG_SDW(0, 0, 7) and overwrite the error in
ret with the return value of regmap_bulk_read()?

> @@ -1216,25 +1358,32 @@ static s32 tas_fw_load(struct tas2783_prv *tas_dev, struct sdw_slave *slave)
>  static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
>  {
>  	struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
> +	uint val;
>  	s32 ret;
>  
>  	if (tas_dev->hw_init)
>  		return 0;
>  
> -	tas_dev->fw_dl_success = false;
> -
> -	ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
> +	regmap_read(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 7), &val);
> +	dev_dbg(dev, "Got B0P0R7 = %02X with %d.", val, ret);
>  	if (ret) {
> -		dev_err(dev, "sw reset failed, err=%d", ret);
> +		dev_err(dev, "Get FW download status failed, err=%d", ret);
>  		return ret;
>  	}

[Severity: High]
Isn't ret used uninitialized here because the return value of
regmap_read() is not assigned to ret?

> +	if (val != 0x22) {
> +		tas_dev->fw_dl_success = false;
> +		ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
> +		if (ret) {
> +			dev_err(dev, "sw reset failed, err=%d", ret);
> +			return ret;
> +		}
> +		usleep_range(2000, 2200);
> +		regcache_cache_only(tas_dev->regmap, false);
> +		tas_dev->fw_use_fallback = false;
>  		ret = tas_fw_load(tas_dev, slave);
> +		if (!ret && tas_dev->fw_use_fallback)
> +			ret = tas_fw_load(tas_dev, slave);
> +	}

[Severity: High]
There appear to be two issues with the TASDEV_REG_SDW(0, 0, 7) (address
0x00800007) firmware status check in tas_io_init():

1. When val == 0x22 and tas_dev->fw_dl_success is false (for example,
   when the driver is unbound and rebound while the amplifier retained
   state), skipping the if (val != 0x22) block leaves
   tas_dev->fw_dl_success as false. Subsequent calls to
   tas_sdw_hw_params() check:

	if (!tas_dev->fw_dl_success) {
		dev_err(tas_dev->dev, "error playback without fw download");
		return -EINVAL;
	}

   which will reject all audio playback and capture streams with -EINVAL.

2. When val != 0x22, tas2783_fw_ready() writes 0x22 to
   TASDEV_REG_SDW(0, 0, 7), but immediately afterwards in tas_io_init(),
   if tas_dev->sa_func_data is NULL, regmap_multi_reg_write() writes
   tas2783_init_seq, which contains:

	REG_SEQ0(0x00800007, 0x20),

   Won't that overwrite register 0x00800007 back to 0x20 in both the
   hardware and regcache right after firmware download completes?

>  	if (!ret) {
>  		if (tas_dev->sa_func_data)
>  			ret = sdca_regmap_write_init(dev, tas_dev->regmap,
> @@ -1382,6 +1531,21 @@ static s32 tas_sdw_probe(struct sdw_slave *peripheral,
>  	if (ret)
>  		return dev_err_probe(dev, ret,
>  				     "slave property read failed");
> +	/*
> +	 * BPT firmware download uses DP0 and relies on the SoundWire stream
> +	 * framework to prepare/deprepare DP0. Ensure DP0 properties exist for
> +	 * the framework prepare/deprepare sequence.
> +	 */
> +	if (!peripheral->prop.dp0_prop) {
> +		peripheral->prop.dp0_prop = devm_kzalloc(dev,
> +			sizeof(*peripheral->prop.dp0_prop), GFP_KERNEL);
> +		if (!peripheral->prop.dp0_prop)
> +			return dev_err_probe(dev, -ENOMEM,
> +					     "dp0_prop alloc failed");
> +
> +		peripheral->prop.dp0_prop->simple_ch_prep_sm = true;
> +		peripheral->prop.dp0_prop->ch_prep_timeout = 25;
> +	}

[Severity: Medium]
Can allocating peripheral->prop.dp0_prop with devm_kzalloc() leave a
dangling pointer on struct sdw_slave if tas_sdw_probe() fails later or if
the driver is unbound?

In drivers/soundwire/mipi_disco.c, sdw_slave_read_prop() does not clear
prop->dp0_prop when the mipi-sdw-dp-0-subproperties firmware node is
absent. If devres frees this allocation on probe failure or unbind,
peripheral->prop.dp0_prop remains non-NULL and will not be reallocated on
re-probe, causing sdw_initialize_slave() and DP0 port preparation to
dereference freed memory.

[Severity: Medium]
Also, what happens during DP0 preparation for BPT when
peripheral->prop.dp0_prop->simple_ch_prep_sm is set to true?

In sdw_prep_deprep_slave_ports() (drivers/soundwire/stream.c), setting
simple_ch_prep_sm = true causes the SoundWire core to skip writing
SDW_DPN_PREPARECTRL(0) and instead rely on sdw_do_port_prep() calling
tas_port_prep() with prep_ch->num == 0. However, tas_port_prep() checks
slave->prop.sink_dpn_prop instead of dp0_prop, and on
SDW_OPS_PORT_PRE_PREP it powers on TAS2783_SDCA_ENT_PDE23 before firmware
has been downloaded (and never powers PDE23 off on
SDW_OPS_PORT_PRE_DEPREP). Should dp0_prop->simple_ch_prep_sm be false for
DP0, or should tas_port_prep() handle port 0 separately?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103133853.1612-1-baojun.xu@ti.com?part=1

Re: [PATCH v1] ASoC: tas2783-sdw: Add basic BPT support

[Pierre-Louis Bossart]

> +static int tas2783_fw_download_bpt(struct tas2783_prv *tas_dev,
> +				   const struct firmware *fmw)
> +{
> +	struct sdw_slave *slave = tas_dev->sdw_peripheral;
> +	struct sdw_bpt_section *sections = NULL;
> +	struct sdw_bus *bus = slave->bus;
> +	struct sdw_bpt_msg msg = {0};
> +	struct tas_fw_file file;
> +	struct tas_fw_hdr hdr;
> +	size_t offset = FW_DL_OFFSET;
> +	size_t img_sz = fmw->size;
> +	const u8 *buf = fmw->data;
> +	int num_sections = 0;
> +	int i, ret;
> +
> +	if (img_sz < FW_DL_OFFSET)
> +		return -EINVAL;
> +
> +	tas_fw_read_hdr(buf, &hdr);
> +	if (hdr.size != img_sz)
> +		return -EINVAL;
> +
> +	while (offset < (img_sz - FW_FL_HDR)) {
> +		size_t adv = (size_t)tas_fw_get_next_file(&buf[offset], &file);
> +
> +		/*
> +		 * Reject a section whose payload runs past the image. Compare
> +		 * file.length (not adv) against the space left after this
> +		 * section's header, so the check is immune to the size_t wrap
> +		 * in tas_fw_get_next_file() on 32-bit kernels.
> +		 */
> +		if (file.length > img_sz - offset - FW_FL_HDR) {
> +			dev_err(tas_dev->dev,
> +				"Firmware section at offset %zu exceeded.\n",
> +				offset);
> +			return -EINVAL;
> +		}
> +		if (num_sections >= TAS2783_MAX_FW_SECTIONS) {
> +			dev_err(tas_dev->dev,
> +				"BPT fw has too many sections (max %d)\n",
> +				TAS2783_MAX_FW_SECTIONS);
> +			return -EINVAL;
> +		}
> +		offset += adv;
> +		num_sections++;
> +	}
> +
> +	if (num_sections == 0)
> +		return -EINVAL;
> +
> +	sections = kcalloc(num_sections, sizeof(*sections), GFP_KERNEL);
> +	if (!sections)
> +		return -ENOMEM;
> +
> +	offset = FW_DL_OFFSET;
> +	for (i = 0; i < num_sections && offset < (img_sz - FW_FL_HDR); i++) {
> +		size_t adv = (size_t)tas_fw_get_next_file(&buf[offset], &file);
> +
> +		sections[i].addr = file.dest_addr;
> +		sections[i].len = file.length;
> +		sections[i].buf = file.fw_data;
> +		offset += adv;
> +
> +		dev_dbg(tas_dev->dev, "BPT section %d: addr=0x%x len=%d",
> +			i, file.dest_addr, file.length);
> +	}
> +
> +	msg.sec = sections;
> +	msg.sections = num_sections;
> +	msg.dev_num = slave->dev_num;
> +	msg.flags = SDW_MSG_FLAG_WRITE;
> +
> +	dev_dbg(tas_dev->dev, "BPT starting: %d sections, dev_num=%d",
> +		num_sections, slave->dev_num);
> +
> +	ret = sdw_bpt_send_sync(bus, slave, &msg);
> +
> +	if (ret < 0)
> +		dev_err(tas_dev->dev, "BPT firmware download failed: %d", ret);
> +	else
> +		dev_dbg(tas_dev->dev,
> +			 "BPT firmware download complete: %d sections",
> +			 num_sections);
> +
> +	kfree(sections);
> +	return ret;
> +}
Before adding BPT-based optimizations to speed-up firmware download,
it'd be good to address existing problems with firmware download, e.g.
when a firmware contains volume configuration information that bypasses
regmap, see the thread in "ASoC: tas2783-sdw: keep the volume settings
across resets". Using BPT will only make things worse I am afraid,
unless there's a check that this is only used for resources not managed
by regmap?

Re: [PATCH v1] ASoC: tas2783-sdw: Add basic BPT support

[Andy Shevchenko]

On Tue, Nov 03, 2026 at 09:38:53PM +0800, Baojun Xu wrote:
> Add for BPT supported.

...

> +enum {
> +	SDW_MSG_FLAG_READ = 0,
> +	SDW_MSG_FLAG_WRITE,

Is it going to HW? If not, drop duplicate assignment. Otherwise assign *all* of
the enum to make code robust against modifications.

> +};

...

> +static int tas2783_fw_download_bpt(struct tas2783_prv *tas_dev,
> +				   const struct firmware *fmw)
> +{
> +	struct sdw_slave *slave = tas_dev->sdw_peripheral;
> +	struct sdw_bpt_section *sections = NULL;
> +	struct sdw_bus *bus = slave->bus;
> +	struct sdw_bpt_msg msg = {0};

'0' is not required.

> +	struct tas_fw_file file;
> +	struct tas_fw_hdr hdr;
> +	size_t offset = FW_DL_OFFSET;
> +	size_t img_sz = fmw->size;
> +	const u8 *buf = fmw->data;
> +	int num_sections = 0;
> +	int i, ret;
> +
> +	if (img_sz < FW_DL_OFFSET)
> +		return -EINVAL;
> +
> +	tas_fw_read_hdr(buf, &hdr);
> +	if (hdr.size != img_sz)
> +		return -EINVAL;
> +
> +	while (offset < (img_sz - FW_FL_HDR)) {
> +		size_t adv = (size_t)tas_fw_get_next_file(&buf[offset], &file);
> +
> +		/*
> +		 * Reject a section whose payload runs past the image. Compare
> +		 * file.length (not adv) against the space left after this
> +		 * section's header, so the check is immune to the size_t wrap
> +		 * in tas_fw_get_next_file() on 32-bit kernels.
> +		 */
> +		if (file.length > img_sz - offset - FW_FL_HDR) {
> +			dev_err(tas_dev->dev,
> +				"Firmware section at offset %zu exceeded.\n",
> +				offset);
> +			return -EINVAL;
> +		}
> +		if (num_sections >= TAS2783_MAX_FW_SECTIONS) {
> +			dev_err(tas_dev->dev,
> +				"BPT fw has too many sections (max %d)\n",
> +				TAS2783_MAX_FW_SECTIONS);
> +			return -EINVAL;
> +		}
> +		offset += adv;
> +		num_sections++;
> +	}
> +
> +	if (num_sections == 0)
> +		return -EINVAL;
> +
> +	sections = kcalloc(num_sections, sizeof(*sections), GFP_KERNEL);

__free() ?

> +	if (!sections)
> +		return -ENOMEM;
> +
> +	offset = FW_DL_OFFSET;
> +	for (i = 0; i < num_sections && offset < (img_sz - FW_FL_HDR); i++) {

	for (int i = 0; i < num_sections && offset < (img_sz - FW_FL_HDR); i++) {

> +		size_t adv = (size_t)tas_fw_get_next_file(&buf[offset], &file);
> +
> +		sections[i].addr = file.dest_addr;
> +		sections[i].len = file.length;
> +		sections[i].buf = file.fw_data;
> +		offset += adv;
> +
> +		dev_dbg(tas_dev->dev, "BPT section %d: addr=0x%x len=%d",
> +			i, file.dest_addr, file.length);
> +	}
> +
> +	msg.sec = sections;
> +	msg.sections = num_sections;
> +	msg.dev_num = slave->dev_num;
> +	msg.flags = SDW_MSG_FLAG_WRITE;
> +
> +	dev_dbg(tas_dev->dev, "BPT starting: %d sections, dev_num=%d",
> +		num_sections, slave->dev_num);
> +
> +	ret = sdw_bpt_send_sync(bus, slave, &msg);

> +

Drop blank line.

> +	if (ret < 0)
> +		dev_err(tas_dev->dev, "BPT firmware download failed: %d", ret);
> +	else
> +		dev_dbg(tas_dev->dev,
> +			 "BPT firmware download complete: %d sections",
> +			 num_sections);
> +
> +	kfree(sections);
> +	return ret;
> +}

...

>  static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
>  {
>  	struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
> +	uint val;

What is this? Use unsigned int or u32 or other better fit.

>  	s32 ret;
>  
>  	if (tas_dev->hw_init)
>  		return 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] ASoC: tas2783-sdw: Add basic BPT support

[kernel test robot]

Hi Baojun,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on linus/master next-20261030]
[cannot apply to v7.3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Baojun-Xu/ASoC-tas2783-sdw-Add-basic-BPT-support/20261103-213853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20261103133853.1612-1-baojun.xu%40ti.com
patch subject: [PATCH v1] ASoC: tas2783-sdw: Add basic BPT support
config: riscv-allyesconfig (https://download.01.org/0day-ci/archive/20261104/202611041243.Bn9AJATC-lkp@intel.com/config)
compiler: clang version 24.0.0git (https://github.com/llvm/llvm-project 65303e81f481bbfe386b8596423f94c43198ba94)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261104/202611041243.Bn9AJATC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202611041243.Bn9AJATC-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> sound/soc/codecs/tas2783-sdw.c:1369:6: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
    1369 |         if (ret) {
         |             ^~~
   sound/soc/codecs/tas2783-sdw.c:1362:9: note: initialize the variable 'ret' to silence this warning
    1362 |         s32 ret;
         |                ^
         |                 = 0
   1 warning generated.


vim +/ret +1369 sound/soc/codecs/tas2783-sdw.c

  1357	
  1358	static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
  1359	{
  1360		struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
  1361		uint val;
  1362		s32 ret;
  1363	
  1364		if (tas_dev->hw_init)
  1365			return 0;
  1366	
  1367		regmap_read(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 7), &val);
  1368		dev_dbg(dev, "Got B0P0R7 = %02X with %d.", val, ret);
> 1369		if (ret) {
  1370			dev_err(dev, "Get FW download status failed, err=%d", ret);
  1371			return ret;
  1372		}
  1373		if (val != 0x22) {
  1374			tas_dev->fw_dl_success = false;
  1375			ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
  1376			if (ret) {
  1377				dev_err(dev, "sw reset failed, err=%d", ret);
  1378				return ret;
  1379			}
  1380			usleep_range(2000, 2200);
  1381			regcache_cache_only(tas_dev->regmap, false);
  1382			tas_dev->fw_use_fallback = false;
  1383			ret = tas_fw_load(tas_dev, slave);
  1384			if (!ret && tas_dev->fw_use_fallback)
  1385				ret = tas_fw_load(tas_dev, slave);
  1386		}
  1387		if (!ret) {
  1388			if (tas_dev->sa_func_data)
  1389				ret = sdca_regmap_write_init(dev, tas_dev->regmap,
  1390							     tas_dev->sa_func_data);
  1391			else
  1392				ret = regmap_multi_reg_write(tas_dev->regmap, tas2783_init_seq,
  1393							     ARRAY_SIZE(tas2783_init_seq));
  1394	
  1395			/* Re-active AMP after resume. */
  1396			regmap_write(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 2), 0);
  1397			if (ret)
  1398				dev_err(tas_dev->dev,
  1399					"init writes failed, err=%d", ret);
  1400			else
  1401				tas_dev->hw_init = true;
  1402		}
  1403	
  1404		return ret;
  1405	}
  1406	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki