soundwire: use bra if msg size > threshold in sdw_nread/write_no_pm#5811
soundwire: use bra if msg size > threshold in sdw_nread/write_no_pm#5811bardliao wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces configurable thresholds for switching SoundWire register transfers to a BPT-based path and adds an early-boot guard for SOF BPT open.
Changes:
- Add
bra_w_threshold/bra_r_thresholdfields tostruct sdw_busand initialize defaults on master add. - Route
sdw_nread_no_pm()/sdw_nwrite_no_pm()through a new BPT transfer helper for larger transfers. - Gate
hda_sdw_bpt_open()when SOF firmware boot is not complete (non-DSPless, first boot).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sound/soc/sof/intel/hda-sdw-bpt.c | Adds a boot-completion guard before preparing BPT DMA. |
| include/linux/soundwire/sdw.h | Extends sdw_bus with BRA threshold configuration fields and updates kernel-doc. |
| drivers/soundwire/bus.c | Adds default BRA thresholds and implements BPT-based nread/nwrite fast path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1bad5a1 to
f490794
Compare
bffe82b to
2315a97
Compare
| * Note that if the message crosses a page boundary each page will be | ||
| * transferred under a separate invocation of the msg_lock. | ||
| */ |
| * Note that if the message crosses a page boundary each page will be | ||
| * transferred under a separate invocation of the msg_lock. | ||
| */ |
0da207e to
b73e001
Compare
b73e001 to
3a35af3
Compare
On Intel ACE2+ platforms, each SoundWire link uses a single pair of
PDIs (PDI0 for TX, PDI1 for RX) and associated DMA resources for
Bulk Port Transfers. When two codecs on the same link initiate BRA
transfers concurrently (e.g. during firmware download), the second
sdw_bpt_send_async() call races with the first:
- intel_ace2x_bpt_open_stream() has a TOCTOU on the bpt_stream
pointer: both callers see it as NULL and proceed
- The second open overwrites the first's stream allocation, leaking
the original sdw_stream_runtime
- PCMSyCM mappings for the first transfer get overwritten, causing
chain DMA to operate with wrong PDI assignments
- IOC timeouts, use-after-free, and double-free follow on close
Add a per-bus bpt_lock mutex held across the send_async/wait span to
serialize BPT transfers. The lock is acquired in sdw_bpt_send_async()
before calling the master ops and released in sdw_bpt_wait() after the
transfer completes. On send_async failure, the lock is released
immediately.
Cross-link transfers (different sdw_bus instances) remain concurrent
since each bus has its own lock.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The BPT stream needs SOF DSP's support. We can only open a BPT stream after the SOF DSP FW is downloaded and booted completed. It also doesn't make sense to wait for SOF FW boot instead of starting common read/write immediately. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
3a35af3 to
c8a1f52
Compare
| int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val) | ||
| { | ||
| struct sdw_bus *bus = slave->bus; | ||
| int ret; | ||
|
|
||
| if (!bus->ops->bpt_send_async || !bus->ops->bpt_wait || | ||
| count < bus->bra_w_threshold) | ||
| goto fallback; | ||
|
|
||
| ret = sdw_ntransfer_no_pm_bpt(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val); | ||
| if (!ret) | ||
| return 0; | ||
|
|
||
| dev_dbg(&slave->dev, | ||
| "BPT write failed for addr %x, count %zu, ret %d fallback to normal write\n", | ||
| addr, count, ret); | ||
| fallback: | ||
| return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val); | ||
| } |
There was a problem hiding this comment.
@charleskeepax @rfvirgil @simontrimmer @shumingfan Will it cause any issue?
| int sdw_bpt_send_async(struct sdw_bus *bus, struct sdw_slave *slave, struct sdw_bpt_msg *msg) | ||
| { | ||
| int len = 0; | ||
| int ret; | ||
| int i; |
There was a problem hiding this comment.
We unlock the mutex when sdw_bpt_send_async, too. It should be fine. @ujfalusi What do you think?
| /* Serialize BPT/BRA transfers per bus: PDIs and DMA resources are shared */ | ||
| mutex_lock(&bus->bpt_lock); | ||
|
|
||
| ret = bus->ops->bpt_send_async(bus, slave, msg); | ||
| if (ret < 0) | ||
| mutex_unlock(&bus->bpt_lock); | ||
|
|
||
| /* on success the lock is held until sdw_bpt_wait() */ | ||
| return ret; | ||
| } | ||
| EXPORT_SYMBOL(sdw_bpt_send_async); | ||
|
|
||
| int sdw_bpt_wait(struct sdw_bus *bus, struct sdw_slave *slave, struct sdw_bpt_msg *msg) | ||
| { | ||
| return bus->ops->bpt_wait(bus, slave, msg); | ||
| int ret; | ||
|
|
||
| ret = bus->ops->bpt_wait(bus, slave, msg); | ||
| mutex_unlock(&bus->bpt_lock); | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
Same as the previous comment
| #define DEFAULT_BRA_WRITE_THRESHOLD 800 | ||
| #define DEFAULT_BRA_READ_THRESHOLD 400 |
| if (!bus->bra_w_threshold) | ||
| bus->bra_w_threshold = DEFAULT_BRA_WRITE_THRESHOLD; | ||
| if (!bus->bra_r_threshold) | ||
| bus->bra_r_threshold = DEFAULT_BRA_READ_THRESHOLD; |
We need a threshold to use BRA. The threshold can be set in the bus struct when sdw_bus_master_add() is called. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
It is more efficient to read/write with bra if the message size is larger than a predefined threshold. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
c8a1f52 to
4e63a29
Compare
| * @bra_w_threshold: Message-size threshold (bytes) above which BPT write is used. | ||
| * If set to 0, a default is used. | ||
| * @bra_r_threshold: Message-size threshold (bytes) above which BPT read is used. | ||
| * If set to 0, a default is used. |
| fallback: | ||
| return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val); |
It is more efficient to read/write with bra if the message size is
larger than a predefined threshold. The threshold can be set by
the caller of sdw_bus_master_add().