[PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Krzysztof Kozlowski]

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>
---
 drivers/soundwire/stream.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 7aa4900dcf31..f275143d7b18 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1291,18 +1291,18 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
 					    unsigned int port_num)
 {
 	struct sdw_dpn_prop *dpn_prop;
-	u8 num_ports;
+	unsigned long mask;
 	int i;
 
 	if (direction == SDW_DATA_DIR_TX) {
-		num_ports = hweight32(slave->prop.source_ports);
+		mask = slave->prop.source_ports;
 		dpn_prop = slave->prop.src_dpn_prop;
 	} else {
-		num_ports = hweight32(slave->prop.sink_ports);
+		mask = slave->prop.sink_ports;
 		dpn_prop = slave->prop.sink_dpn_prop;
 	}
 
-	for (i = 0; i < num_ports; i++) {
+	for_each_set_bit(i, &mask, 32) {
 		if (dpn_prop[i].num == port_num)
 			return &dpn_prop[i];
 	}
-- 
2.43.0

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Pierre-Louis Bossart]

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.


> ---
>  drivers/soundwire/stream.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 7aa4900dcf31..f275143d7b18 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1291,18 +1291,18 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
>  					    unsigned int port_num)
>  {
>  	struct sdw_dpn_prop *dpn_prop;
> -	u8 num_ports;
> +	unsigned long mask;
>  	int i;
>  
>  	if (direction == SDW_DATA_DIR_TX) {
> -		num_ports = hweight32(slave->prop.source_ports);
> +		mask = slave->prop.source_ports;
>  		dpn_prop = slave->prop.src_dpn_prop;
>  	} else {
> -		num_ports = hweight32(slave->prop.sink_ports);
> +		mask = slave->prop.sink_ports;
>  		dpn_prop = slave->prop.sink_dpn_prop;
>  	}
>  
> -	for (i = 0; i < num_ports; i++) {
> +	for_each_set_bit(i, &mask, 32) {
>  		if (dpn_prop[i].num == port_num)
>  			return &dpn_prop[i];
>  	}

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Krzysztof Kozlowski]

On 29/07/2024 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.

I think we speak about two different things. port num > 1, that's
correct. But index for src_dpn_prop array is something different. Look
at mipi-disco sdw_slave_read_dpn():

173         u32 bit, i = 0;
...
178         addr = ports;
179         /* valid ports are 1 to 14 so apply mask */
180         addr &= GENMASK(14, 1);
181
182         for_each_set_bit(bit, &addr, 32) {
...
186                 dpn[i].num = bit;


so dpn[0..i] = 1..n
where i is also the bit in the mask.

Similar implementation was done in Qualcomm wsa and wcd codecs like:
array indexed from 0:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n51

genmask from 0, with a mistake:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n1255

The mistake I corrected here:
https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d7a8b56f05@linaro.org/

To summarize, the mask does not denote port numbers (1...14) but indices
of the dpn array which are from 0..whatever (usually -1 from port number).


Best regards,
Krzysztof

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Krzysztof Kozlowski]

On 30/07/2024 10:23, Krzysztof Kozlowski wrote:
> On 29/07/2024 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.
> 
> I think we speak about two different things. port num > 1, that's
> correct. But index for src_dpn_prop array is something different. Look
> at mipi-disco sdw_slave_read_dpn():
> 
> 173         u32 bit, i = 0;
> ...
> 178         addr = ports;
> 179         /* valid ports are 1 to 14 so apply mask */
> 180         addr &= GENMASK(14, 1);
> 181
> 182         for_each_set_bit(bit, &addr, 32) {
> ...
> 186                 dpn[i].num = bit;
> 
> 
> so dpn[0..i] = 1..n
> where i is also the bit in the mask.
> 
> Similar implementation was done in Qualcomm wsa and wcd codecs like:
> array indexed from 0:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n51
> 
> genmask from 0, with a mistake:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n1255
> 
> The mistake I corrected here:
> https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d7a8b56f05@linaro.org/
> 
> To summarize, the mask does not denote port numbers (1...14) but indices
> of the dpn array which are from 0..whatever (usually -1 from port number).
> 

Let me also complete this with a real life example of my work in
progress. I want to use same dpn_prop array for sink and source ports
and use different masks. The code in progress is:

https://git.codelinaro.org/krzysztof.kozlowski/linux/-/commit/ef709a0e8ab2498751305367e945df18d7a05c78#6f965d7b74e712a5cfcbc1cca407b85443a66bac_2147_2157

Without this patch, I get -EINVAL from sdw_get_slave_dpn_prop():
  soundwire sdw-master-1-0: Program transport params failed: -22

Best regards,
Krzysztof

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Pierre-Louis Bossart]

On 7/30/24 10:39, Krzysztof Kozlowski wrote:
> On 30/07/2024 10:23, Krzysztof Kozlowski wrote:
>> On 29/07/2024 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.
>>
>> I think we speak about two different things. port num > 1, that's
>> correct. But index for src_dpn_prop array is something different. Look
>> at mipi-disco sdw_slave_read_dpn():
>>
>> 173         u32 bit, i = 0;
>> ...
>> 178         addr = ports;
>> 179         /* valid ports are 1 to 14 so apply mask */
>> 180         addr &= GENMASK(14, 1);
>> 181
>> 182         for_each_set_bit(bit, &addr, 32) {
>> ...
>> 186                 dpn[i].num = bit;
>>
>>
>> so dpn[0..i] = 1..n
>> where i is also the bit in the mask.

yes, agreed on the indexing.

But are we in agreement that the case of non-contiguous ports would not
create any issues? the existing code is not efficient but it wouldn't
crash, would it?

There are multiple cases of non-contiguous ports, I am not aware of any
issues...

rt700-sdw.c:    prop->source_ports = 0x14; /* BITMAP: 00010100 */
rt711-sdca-sdw.c:       prop->source_ports = 0x14; /* BITMAP: 00010100
rt712-sdca-sdw.c:       prop->source_ports = BIT(8) | BIT(4);
rt715-sdca-sdw.c:       prop->source_ports = 0x50;/* BITMAP: 01010000 */
rt722-sdca-sdw.c:       prop->source_ports = BIT(6) | BIT(2); /* BITMAP:
01000100 */

same for sinks:

rt712-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
00001010 */
rt722-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
00001010 */

>> Similar implementation was done in Qualcomm wsa and wcd codecs like:
>> array indexed from 0:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n51
>>
>> genmask from 0, with a mistake:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n1255
>>
>> The mistake I corrected here:
>> https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d7a8b56f05@linaro.org/
>>
>> To summarize, the mask does not denote port numbers (1...14) but indices
>> of the dpn array which are from 0..whatever (usually -1 from port number).
>>
> 
> Let me also complete this with a real life example of my work in
> progress. I want to use same dpn_prop array for sink and source ports
> and use different masks. The code in progress is:
> 
> https://git.codelinaro.org/krzysztof.kozlowski/linux/-/commit/ef709a0e8ab2498751305367e945df18d7a05c78#6f965d7b74e712a5cfcbc1cca407b85443a66bac_2147_2157
> 
> Without this patch, I get -EINVAL from sdw_get_slave_dpn_prop():
>   soundwire sdw-master-1-0: Program transport params failed: -2

Not following, sorry. The sink and source masks are separate on purpose,
to allow for bi-directional ports. The SoundWire spec allows a port to
be configured at run-time either as source or sink. In practice I've
never seen this happen, all existing hardware relies on ports where the
direction is hard-coded/fixed, but still we want to follow the spec.

So if ports can be either source or sink, I am not sure how the
properties could be shared with a single array?

Those two lines aren't clear to me at all:

	pdev->prop.sink_dpn_prop = wsa884x_sink_dpn_prop;
	pdev->prop.src_dpn_prop = wsa884x_sink_dpn_prop;

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Krzysztof Kozlowski]

On 30/07/2024 10:59, Pierre-Louis Bossart wrote:
>>>>
>>>> 	/* 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.
>>>
>>> I think we speak about two different things. port num > 1, that's
>>> correct. But index for src_dpn_prop array is something different. Look
>>> at mipi-disco sdw_slave_read_dpn():
>>>
>>> 173         u32 bit, i = 0;
>>> ...
>>> 178         addr = ports;
>>> 179         /* valid ports are 1 to 14 so apply mask */
>>> 180         addr &= GENMASK(14, 1);
>>> 181
>>> 182         for_each_set_bit(bit, &addr, 32) {
>>> ...
>>> 186                 dpn[i].num = bit;
>>>
>>>
>>> so dpn[0..i] = 1..n
>>> where i is also the bit in the mask.
> 
> yes, agreed on the indexing.
> 
> But are we in agreement that the case of non-contiguous ports would not
> create any issues? the existing code is not efficient but it wouldn't
> crash, would it?
> 
> There are multiple cases of non-contiguous ports, I am not aware of any
> issues...
> 
> rt700-sdw.c:    prop->source_ports = 0x14; /* BITMAP: 00010100 */
> rt711-sdca-sdw.c:       prop->source_ports = 0x14; /* BITMAP: 00010100
> rt712-sdca-sdw.c:       prop->source_ports = BIT(8) | BIT(4);
> rt715-sdca-sdw.c:       prop->source_ports = 0x50;/* BITMAP: 01010000 */
> rt722-sdca-sdw.c:       prop->source_ports = BIT(6) | BIT(2); /* BITMAP:
> 01000100 */
> 
> same for sinks:
> 
> rt712-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
> 00001010 */
> rt722-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
> 00001010 */

All these work because they have separate source and sink dpn_prop
arrays. Separate arrays, separate number of ports, separate masks - all
this is good. Now going to my code...

> 
>>> Similar implementation was done in Qualcomm wsa and wcd codecs like:
>>> array indexed from 0:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n51
>>>
>>> genmask from 0, with a mistake:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n1255
>>>
>>> The mistake I corrected here:
>>> https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d7a8b56f05@linaro.org/
>>>
>>> To summarize, the mask does not denote port numbers (1...14) but indices
>>> of the dpn array which are from 0..whatever (usually -1 from port number).
>>>
>>
>> Let me also complete this with a real life example of my work in
>> progress. I want to use same dpn_prop array for sink and source ports
>> and use different masks. The code in progress is:
>>
>> https://git.codelinaro.org/krzysztof.kozlowski/linux/-/commit/ef709a0e8ab2498751305367e945df18d7a05c78#6f965d7b74e712a5cfcbc1cca407b85443a66bac_2147_2157
>>
>> Without this patch, I get -EINVAL from sdw_get_slave_dpn_prop():
>>   soundwire sdw-master-1-0: Program transport params failed: -2
> 
> Not following, sorry. The sink and source masks are separate on purpose,
> to allow for bi-directional ports. The SoundWire spec allows a port to
> be configured at run-time either as source or sink. In practice I've
> never seen this happen, all existing hardware relies on ports where the
> direction is hard-coded/fixed, but still we want to follow the spec.

The ports are indeed hard-coded/fixed.

> 
> So if ports can be either source or sink, I am not sure how the
> properties could be shared with a single array?

Because I could, just easier to code. :) Are you saying the code is not
correct? If I understand the concept of source/sink dpn port mask, it
should be correct. I have some array with source and sink ports. I pass
it to Soundwire with a mask saying which ports are source and which are
sink.

> 
> Those two lines aren't clear to me at all:
> 
> 	pdev->prop.sink_dpn_prop = wsa884x_sink_dpn_prop;
> 	pdev->prop.src_dpn_prop = wsa884x_sink_dpn_prop;

I could do: s/wsa884x_sink_dpn_prop/wsa884x_dpn_prop/ and expect the
code to be correct.

Best regards,
Krzysztof

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Pierre-Louis Bossart]

On 7/30/24 11:19, Krzysztof Kozlowski wrote:
> On 30/07/2024 10:59, Pierre-Louis Bossart wrote:
>>>>>
>>>>> 	/* 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.
>>>>
>>>> I think we speak about two different things. port num > 1, that's
>>>> correct. But index for src_dpn_prop array is something different. Look
>>>> at mipi-disco sdw_slave_read_dpn():
>>>>
>>>> 173         u32 bit, i = 0;
>>>> ...
>>>> 178         addr = ports;
>>>> 179         /* valid ports are 1 to 14 so apply mask */
>>>> 180         addr &= GENMASK(14, 1);
>>>> 181
>>>> 182         for_each_set_bit(bit, &addr, 32) {
>>>> ...
>>>> 186                 dpn[i].num = bit;
>>>>
>>>>
>>>> so dpn[0..i] = 1..n
>>>> where i is also the bit in the mask.
>>
>> yes, agreed on the indexing.
>>
>> But are we in agreement that the case of non-contiguous ports would not
>> create any issues? the existing code is not efficient but it wouldn't
>> crash, would it?
>>
>> There are multiple cases of non-contiguous ports, I am not aware of any
>> issues...
>>
>> rt700-sdw.c:    prop->source_ports = 0x14; /* BITMAP: 00010100 */
>> rt711-sdca-sdw.c:       prop->source_ports = 0x14; /* BITMAP: 00010100
>> rt712-sdca-sdw.c:       prop->source_ports = BIT(8) | BIT(4);
>> rt715-sdca-sdw.c:       prop->source_ports = 0x50;/* BITMAP: 01010000 */
>> rt722-sdca-sdw.c:       prop->source_ports = BIT(6) | BIT(2); /* BITMAP:
>> 01000100 */
>>
>> same for sinks:
>>
>> rt712-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
>> 00001010 */
>> rt722-sdca-sdw.c:       prop->sink_ports = BIT(3) | BIT(1); /* BITMAP:
>> 00001010 */
> 
> All these work because they have separate source and sink dpn_prop
> arrays. Separate arrays, separate number of ports, separate masks - all
> this is good. Now going to my code...
> 
>>
>>>> Similar implementation was done in Qualcomm wsa and wcd codecs like:
>>>> array indexed from 0:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n51
>>>>
>>>> genmask from 0, with a mistake:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n1255
>>>>
>>>> The mistake I corrected here:
>>>> https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d7a8b56f05@linaro.org/
>>>>
>>>> To summarize, the mask does not denote port numbers (1...14) but indices
>>>> of the dpn array which are from 0..whatever (usually -1 from port number).
>>>>
>>>
>>> Let me also complete this with a real life example of my work in
>>> progress. I want to use same dpn_prop array for sink and source ports
>>> and use different masks. The code in progress is:
>>>
>>> https://git.codelinaro.org/krzysztof.kozlowski/linux/-/commit/ef709a0e8ab2498751305367e945df18d7a05c78#6f965d7b74e712a5cfcbc1cca407b85443a66bac_2147_2157
>>>
>>> Without this patch, I get -EINVAL from sdw_get_slave_dpn_prop():
>>>   soundwire sdw-master-1-0: Program transport params failed: -2
>>
>> Not following, sorry. The sink and source masks are separate on purpose,
>> to allow for bi-directional ports. The SoundWire spec allows a port to
>> be configured at run-time either as source or sink. In practice I've
>> never seen this happen, all existing hardware relies on ports where the
>> direction is hard-coded/fixed, but still we want to follow the spec.
> 
> The ports are indeed hard-coded/fixed.
> 
>>
>> So if ports can be either source or sink, I am not sure how the
>> properties could be shared with a single array?
> 
> Because I could, just easier to code. :) Are you saying the code is not
> correct? If I understand the concept of source/sink dpn port mask, it
> should be correct. I have some array with source and sink ports. I pass
> it to Soundwire with a mask saying which ports are source and which are
> sink.
> 
>>
>> Those two lines aren't clear to me at all:
>>
>> 	pdev->prop.sink_dpn_prop = wsa884x_sink_dpn_prop;
>> 	pdev->prop.src_dpn_prop = wsa884x_sink_dpn_prop;
> 
> I could do: s/wsa884x_sink_dpn_prop/wsa884x_dpn_prop/ and expect the
> code to be correct.

Ah I think I see what you are trying to do, you have a single dpn_prop
array but each entry is valid for either sink or source depending on the
sink / source_mask which don't overlap.

Did I get this right?

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Krzysztof Kozlowski]

On 30/07/2024 11:28, Pierre-Louis Bossart wrote:
>>
>>>
>>> So if ports can be either source or sink, I am not sure how the
>>> properties could be shared with a single array?
>>
>> Because I could, just easier to code. :) Are you saying the code is not
>> correct? If I understand the concept of source/sink dpn port mask, it
>> should be correct. I have some array with source and sink ports. I pass
>> it to Soundwire with a mask saying which ports are source and which are
>> sink.
>>
>>>
>>> Those two lines aren't clear to me at all:
>>>
>>> 	pdev->prop.sink_dpn_prop = wsa884x_sink_dpn_prop;
>>> 	pdev->prop.src_dpn_prop = wsa884x_sink_dpn_prop;
>>
>> I could do: s/wsa884x_sink_dpn_prop/wsa884x_dpn_prop/ and expect the
>> code to be correct.
> 
> Ah I think I see what you are trying to do, you have a single dpn_prop
> array but each entry is valid for either sink or source depending on the
> sink / source_mask which don't overlap.
> 
> Did I get this right?

Yes, correct.

Best regards,
Krzysztof

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Pierre-Louis Bossart]

On 7/30/24 11:29, Krzysztof Kozlowski wrote:
> On 30/07/2024 11:28, Pierre-Louis Bossart wrote:
>>>
>>>>
>>>> So if ports can be either source or sink, I am not sure how the
>>>> properties could be shared with a single array?
>>>
>>> Because I could, just easier to code. :) Are you saying the code is not
>>> correct? If I understand the concept of source/sink dpn port mask, it
>>> should be correct. I have some array with source and sink ports. I pass
>>> it to Soundwire with a mask saying which ports are source and which are
>>> sink.
>>>
>>>>
>>>> Those two lines aren't clear to me at all:
>>>>
>>>> 	pdev->prop.sink_dpn_prop = wsa884x_sink_dpn_prop;
>>>> 	pdev->prop.src_dpn_prop = wsa884x_sink_dpn_prop;
>>>
>>> I could do: s/wsa884x_sink_dpn_prop/wsa884x_dpn_prop/ and expect the
>>> code to be correct.
>>
>> Ah I think I see what you are trying to do, you have a single dpn_prop
>> array but each entry is valid for either sink or source depending on the
>> sink / source_mask which don't overlap.
>>
>> Did I get this right?
> 
> Yes, correct.

Sounds good, thanks for the explanations.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Vinod Koul]

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!

> 
> 
> > ---
> >  drivers/soundwire/stream.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> > index 7aa4900dcf31..f275143d7b18 100644
> > --- a/drivers/soundwire/stream.c
> > +++ b/drivers/soundwire/stream.c
> > @@ -1291,18 +1291,18 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
> >  					    unsigned int port_num)
> >  {
> >  	struct sdw_dpn_prop *dpn_prop;
> > -	u8 num_ports;
> > +	unsigned long mask;
> >  	int i;
> >  
> >  	if (direction == SDW_DATA_DIR_TX) {
> > -		num_ports = hweight32(slave->prop.source_ports);
> > +		mask = slave->prop.source_ports;
> >  		dpn_prop = slave->prop.src_dpn_prop;
> >  	} else {
> > -		num_ports = hweight32(slave->prop.sink_ports);
> > +		mask = slave->prop.sink_ports;
> >  		dpn_prop = slave->prop.sink_dpn_prop;
> >  	}
> >  
> > -	for (i = 0; i < num_ports; i++) {
> > +	for_each_set_bit(i, &mask, 32) {
> >  		if (dpn_prop[i].num == port_num)
> >  			return &dpn_prop[i];
> >  	}

-- 
~Vinod

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Vinod Koul]

On Mon, 29 Jul 2024 16:01:57 +0200, 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.
> 
> [...]

Applied, thanks!

[1/1] soundwire: stream: fix programming slave ports for non-continous port maps
      commit: ab8d66d132bc8f1992d3eb6cab8d32dda6733c84

Best regards,
-- 
~Vinod

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Liao, Bard]

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

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

are not continuous. For example, a codec has source port number = 1 and 3,

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].


>
>>
>>> ---
>>>   drivers/soundwire/stream.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
>>> index 7aa4900dcf31..f275143d7b18 100644
>>> --- a/drivers/soundwire/stream.c
>>> +++ b/drivers/soundwire/stream.c
>>> @@ -1291,18 +1291,18 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
>>>   					    unsigned int port_num)
>>>   {
>>>   	struct sdw_dpn_prop *dpn_prop;
>>> -	u8 num_ports;
>>> +	unsigned long mask;
>>>   	int i;
>>>   
>>>   	if (direction == SDW_DATA_DIR_TX) {
>>> -		num_ports = hweight32(slave->prop.source_ports);
>>> +		mask = slave->prop.source_ports;
>>>   		dpn_prop = slave->prop.src_dpn_prop;
>>>   	} else {
>>> -		num_ports = hweight32(slave->prop.sink_ports);
>>> +		mask = slave->prop.sink_ports;
>>>   		dpn_prop = slave->prop.sink_dpn_prop;
>>>   	}
>>>   
>>> -	for (i = 0; i < num_ports; i++) {
>>> +	for_each_set_bit(i, &mask, 32) {
>>>   		if (dpn_prop[i].num == port_num)
>>>   			return &dpn_prop[i];
>>>   	}

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Krzysztof Kozlowski]

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.

> 
> 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?

It's not only my patch which uses for_each_set_bit(). sysfs_slave_dpn
does the same.

Best regards,
Krzysztof

RE: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Liao, Bard]

> -----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/sound/soc/codecs/rt722-sdca-sdw.c#n217
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

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Krzysztof Kozlowski]

On 03/09/2024 17:17, Liao, Bard wrote:

>>>
>>> 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.

Yes, you are right. I think I cannot achieve my initial goal of using
same dpn array with different indices. My patch should be reverted, I
believe.

I'll send a revert, sorry for the mess.

Best regards,
Krzysztof

Re: [PATCH] soundwire: stream: fix programming slave ports for non-continous port maps

[Charles Keepax]

On Wed, Sep 04, 2024 at 01:43:50PM +0200, Krzysztof Kozlowski wrote:
> On 03/09/2024 17:17, Liao, Bard wrote:
> 
> >>>
> >>> 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.
> 
> Yes, you are right. I think I cannot achieve my initial goal of using
> same dpn array with different indices. My patch should be reverted, I
> believe.
> 
> I'll send a revert, sorry for the mess.
> 

Hi, apologies for being late to the party (was on holiday), but yeah
this is breaking things for me as well and is clearly wrong.
Agree probably best to revert.

Thanks,
Charles