-----Original Message----- From: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org Sent: Tuesday, September 3, 2024 8:50 PM To: Liao, Bard yung-chuan.liao@linux.intel.com; Vinod Koul vkoul@kernel.org; Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com> Cc: Kale, Sanyog R sanyog.r.kale@intel.com; Shreyas NC shreyas.nc@intel.com; alsa-devel@alsa-project.org; linux- kernel@vger.kernel.org; stable@vger.kernel.org; Liao, Bard bard.liao@intel.com Subject: Re: [PATCH] soundwire: stream: fix programming slave ports for non- continous port maps
On 03/09/2024 09:34, Liao, Bard wrote:
On 7/31/2024 2:56 PM, Vinod Koul wrote:
On 29-07-24, 16:25, Pierre-Louis Bossart wrote:
On 7/29/24 16:01, Krzysztof Kozlowski wrote:
Two bitmasks in 'struct sdw_slave_prop' - 'source_ports' and 'sink_ports' - define which ports to program in sdw_program_slave_port_params(). The masks are used to get the appropriate data port properties ('struct sdw_get_slave_dpn_prop')
from
an array.
Bitmasks can be non-continuous or can start from index different than 0, thus when looking for matching port property for given port, we must iterate over mask bits, not from 0 up to number of ports.
This fixes allocation and programming slave ports, when a source or sink masks start from further index.
Fixes: f8101c74aa54 ("soundwire: Add Master and Slave port
programming")
Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
This is a valid change to optimize how the port are accessed.
But the commit message is not completely clear, the allocation in mipi_disco.c is not modified and I don't think there's anything that would crash. If there are non-contiguous ports, we will still allocate space that will not be initialized/used.
/* Allocate memory for set bits in port lists */ nval = hweight32(prop->source_ports); prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval, sizeof(*prop->src_dpn_prop), GFP_KERNEL); if (!prop->src_dpn_prop) return -ENOMEM;
/* Read dpn properties for source port(s) */ sdw_slave_read_dpn(slave, prop->src_dpn_prop, nval, prop->source_ports, "source");
IOW, this is a valid change, but it's an optimization, not a fix in the usual sense of 'kernel oops otherwise'.
Am I missing something?
BTW, the notion of DPn is that n > 0. DP0 is a special case with different properties, BIT(0) cannot be set for either of the sink/source port bitmask.
The fix seems right to me, we cannot have assumption that ports are contagious, so we need to iterate over all valid ports and not to N ports which code does now!
Sorry to jump in after the commit was applied. But, it breaks my test.
The point is that dpn_prop[i].num where the i is the array index, and
num is the port number. So, `for (i = 0; i < num_ports; i++)` will iterate
Please fix your email client so it will write proper paragraphs. Inserting blank lines after each sentence reduces the readability.
over all valid ports.
We can see in below drivers/soundwire/mipi_disco.c
nval = hweight32(prop->sink_ports);
prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
sizeof(*prop->sink_dpn_prop),
GFP_KERNEL);
And sdw_slave_read_dpn() set data port properties one by one.
`for_each_set_bit(i, &mask, 32)` will break the system when port numbers
The entire point of the commit is to fix it for non-continuous masks. And I tested it with non-continuous masks.
are not continuous. For example, a codec has source port number = 1 and 3,
Which codec? Can you give a link to exact line in *UPSTREAM* kernel.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sou... prop->source_ports = BIT(6) | BIT(2); /* BITMAP: 01000100 */ prop->sink_ports = BIT(3) | BIT(1); /* BITMAP: 00001010 */
then dpn_prop[0].num = 1 and dpn_prop[1].num = 3. And we need to go
throuth dpn_prop[0] and dpn_prop[1] instead of dpn_prop[1] and
dpn_prop[3].
What are the source or sink ports in your case? Maybe you just generate wrong mask?
I checked my mask is 0xa when I do aplay and it matches the sink_ports of the rt722 codec.
It's not only my patch which uses for_each_set_bit(). sysfs_slave_dpn does the same.
What sysfs_slave_dpn does is i = 0; for_each_set_bit(bit, &mask, 32) { if (bit == N) { return sprintf(buf, format_string, dpn[i].field); } i++; } It uses a variable "i" to represent the array index of dpn[i]. But, it is for_each_set_bit(i, &mask, 32) in your patch and the variable "i" which represents each bit of the mask is used as the index of dpn_prop[i].
Again, the point is that the bits of mask is not the index of the dpn_prop[] array.
Best regards, Krzysztof