Skip to content

SoundWire: fix port_ready[] allocation and remove it when unnecessary - #2269

Merged
plbossart merged 2 commits into
thesofproject:topic/sof-devfrom
plbossart:sdw/fix-port-ready
Jul 23, 2020
Merged

SoundWire: fix port_ready[] allocation and remove it when unnecessary#2269
plbossart merged 2 commits into
thesofproject:topic/sof-devfrom
plbossart:sdw/fix-port-ready

Conversation

@plbossart

Copy link
Copy Markdown
Member

The use of port_ready[] is broken for ports that are not contiguous, and none of the existing codec devices use this functionality anyways. Let's fix the core and remove unnecessary allocations in codec drivers.

No obvious regressions seen on CML-U w/ SoundWire, but additional checks might not hurt.

@shumingfan@ryans-lee@oder-chiou@jack-cy-yu Can you do me a favor and double-check that the use of the simplified channel prepare state machine is indeed a feature of your hardware, not a copy/paste error or omission. If we do need to enable the more complicated state machine then the last patch needs to be redone. Thanks!

@bardliao

Copy link
Copy Markdown
Collaborator

@plbossart Now slave->port_ready is only allocated and initialized at sdw_slave_read_prop(). But I don't see sdw_slave_read_prop() is called anywhere. Will it cause problem? Can we move the allocation and initialization to sdw_slave_add() since it is a fixed array now?

@RanderWang

Copy link
Copy Markdown

@plbossart Now slave->port_ready is only allocated and initialized at sdw_slave_read_prop(). But I don't see sdw_slave_read_prop() is called anywhere. Will it cause problem? Can we move the allocation and initialization to sdw_slave_add() since it is a fixed array now?

I also found this. I added log in sdw driver but found port_ready was not checked by complete(&slave->port_ready[port]) on TGL RVP for no port irq was triggered.

@plbossart

Copy link
Copy Markdown
MemberAuthor

@plbossart Now slave->port_ready is only allocated and initialized at sdw_slave_read_prop(). But I don't see sdw_slave_read_prop() is called anywhere. Will it cause problem? Can we move the allocation and initialization to sdw_slave_add() since it is a fixed array now?

I also found this. I added log in sdw driver but found port_ready was not checked by complete(&slave->port_ready[port]) on TGL RVP for no port irq was triggered.

Sorry I don't get your point @RanderWang. If the codecs uses the simplified channel prepare state machine, there will not be any port_ready interrupts.

@plbossart

Copy link
Copy Markdown
MemberAuthor

@plbossart Now slave->port_ready is only allocated and initialized at sdw_slave_read_prop(). But I don't see sdw_slave_read_prop() is called anywhere. Will it cause problem? Can we move the allocation and initialization to sdw_slave_add() since it is a fixed array now?

we could indeed move to a common place, good suggestion, so that codec drivers don't need to worry about this.

@plbossart
plbossartforce-pushed the sdw/fix-port-ready branch 2 times, most recently from 430919b to 8ce7e5fCompareJuly 13, 2020 14:20
@plbossartplbossart changed the title [TEST] SoundWire: fix port_ready[] allocation and remove it when unnecessarySoundWire: fix port_ready[] allocation and remove it when unnecessaryJul 13, 2020
@RanderWang

Copy link
Copy Markdown

@plbossart Now slave->port_ready is only allocated and initialized at sdw_slave_read_prop(). But I don't see sdw_slave_read_prop() is called anywhere. Will it cause problem? Can we move the allocation and initialization to sdw_slave_add() since it is a fixed array now?

I also found this. I added log in sdw driver but found port_ready was not checked by complete(&slave->port_ready[port]) on TGL RVP for no port irq was triggered.

Sorry I don't get your point @RanderWang. If the codecs uses the simplified channel prepare state machine, there will not be any port_ready interrupts.

I meant not error in original PR for the port_ready was not checked by slave, or it would lead to kernel panic.
That was why Bard said it would cause problem.

Comment threaddrivers/soundwire/mipi_disco.c Outdated
@RanderWang

Copy link
Copy Markdown

Tested on TGL RVP. no regression issue was found.

Comment threaddrivers/soundwire/slave.c Outdated
Comment threadsound/soc/codecs/max98373-sdw.c
A Device may have at most 15 physical ports (DP0, DP1..DP14).
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart

Copy link
Copy Markdown
MemberAuthor

@bardliao@RanderWang@lyakh can you take a look?

@bardliao this would need to be squashed with another patch 13e6f3d ('ASoC: codecs: rt*-sdw.c: fix off-by-one port count') since it's really the same issue.

bardliao
bardliao previously approved these changes Jul 22, 2020
@bardliao

Copy link
Copy Markdown
Collaborator

@bardliao this would need to be squashed with another patch 13e6f3d ('ASoC: codecs: rt*-sdw.c: fix off-by-one port count') since it's really the same issue.

acked.

@RanderWang

Copy link
Copy Markdown

@plbossart a CI compiling error

drivers/soundwire/mipi_disco.c: Infunctionsdw_slave_read_prop’:
drivers/soundwire/mipi_disco.c:292:12: error: unusedvariablei’ [-Werror=unused-variable]
292 | intnval, i, dp0=0;
| ^
cc1: allwarningsbeingtreatedaserrorsscripts/Makefile.build:280: recipefortarget'drivers/soundwire/mipi_disco.o'failed

lyakh
lyakh previously approved these changes Jul 22, 2020
… codecs
The existing code allocates memory for the total number of ports.
This only works if the ports are contiguous, but will break if e.g. a
Devices uses port0, 1, and 14. The port_ready[] array would contain 3
elements, which would lead to an out-of-bounds access. Conversely in
other cases, the wrong port index would be used leading to timeouts on
prepare.
This can be fixed by allocating for the worst-case of 15
ports (DP0..DP14). In addition since the number is now fixed, we can
use an array instead of a dynamic allocation.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart

Copy link
Copy Markdown
MemberAuthor

@plbossart a CI compiling error

drivers/soundwire/mipi_disco.c: Infunctionsdw_slave_read_prop’:
drivers/soundwire/mipi_disco.c:292:12: error: unusedvariablei’ [-Werror=unused-variable]
292 | intnval, i, dp0=0;
| ^
cc1: allwarningsbeingtreatedaserrorsscripts/Makefile.build:280: recipefortarget'drivers/soundwire/mipi_disco.o'failed

fixed now

@plbossart
plbossart dismissed stale reviews from lyakh and bardliao via c98c5feJuly 22, 2020 16:03
@plbossart
plbossart requested review from bardliao and lyakhJuly 22, 2020 16:03
@plbossart
plbossart merged commit e1a2507 into thesofproject:topic/sof-devJul 23, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@plbossart@bardliao@RanderWang@lyakh