[PATCH] soundwire: stream: fix programming slave ports for non-continous port maps
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]; }
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;
num_ports = hweight32(slave->prop.sink_ports);
mask = slave->prop.sink_ports;
- 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]; }
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/soun...
genmask from 0, with a mistake: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
The mistake I corrected here: https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d...
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
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/soun...
genmask from 0, with a mistake: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
The mistake I corrected here: https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d...
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/ef709a0e8ab249...
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
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/soun...
genmask from 0, with a mistake: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
The mistake I corrected here: https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d...
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/ef709a0e8ab249...
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;
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/soun...
genmask from 0, with a mistake: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
The mistake I corrected here: https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d...
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/ef709a0e8ab249...
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
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/soun...
genmask from 0, with a mistake: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
The mistake I corrected here: https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d...
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/ef709a0e8ab249...
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?
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
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
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;
num_ports = hweight32(slave->prop.sink_ports);
mask = slave->prop.sink_ports;
- 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]; }
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;
num_ports = hweight32(slave->prop.sink_ports);
mask = slave->prop.sink_ports;
- 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]; }
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
-----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
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
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
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,
participants (6)
-
Charles Keepax -
Krzysztof Kozlowski -
Liao, Bard -
Liao, Bard -
Pierre-Louis Bossart -
Vinod Koul