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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7171817f-e8c6-4828-8423-0929644ff2df@linaro.org>
Date: Tue, 30 Jul 2024 11:19:11 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
 Vinod Koul <vkoul@...nel.org>, Bard Liao <yung-chuan.liao@...ux.intel.com>,
 Sanyog Kale <sanyog.r.kale@...el.com>, Shreyas NC <shreyas.nc@...el.com>,
 alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Cc: stable@...r.kernel.org
Subject: Re: [PATCH] soundwire: stream: fix programming slave ports for
 non-continous port maps

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