| Message ID | 20240729140157.326450-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Accepted |
| Commit | ab8d66d132bc8f1992d3eb6cab8d32dda6733c84 |
| Headers | show |
| Series | soundwire: stream: fix programming slave ports for non-continous port maps | expand |
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
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
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;
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
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?
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; > > 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 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,
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]; >>> }
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/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
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
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]; }
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(-)