linux-kernel - Re: [PATCH v2] soundwire: qcom: adjust autoenumeration timeout

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]

Date:   Sat, 7 May 2022 07:52:46 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        vkoul@...nel.org
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        bard.liao@...el.com,
        Srinivasa Rao Mandadapu <quic_srivasam@...cinc.com>
Subject: Re: [PATCH v2] soundwire: qcom: adjust autoenumeration timeout

Thanks Pierre,

On 06/05/2022 15:13, Pierre-Louis Bossart wrote:
> 
> 
> On 5/6/22 03:47, Srinivas Kandagatla wrote:
>> Currently timeout for autoenumeration during probe and bus reset is set to
>> 2 secs which is really a big value. This can have an adverse effect on
>> boot time if the slave device is not ready/reset.
>> This was the case with wcd938x which was not reset yet but we spent 2
>> secs waiting in the soundwire controller probe. Reduce this time to
>> 1/10 of Hz which should be good enough time to finish autoenumeration
>> if any slaves are available on the bus.
> 
> Humm, now that I think of it I am not sure what reducing the timeout does.
> 
> It's clear that autoenumeration should be very fast, but if there is
> nothing to enumerate what would happen then? It seems that reducing the
> timeout value only forces an inconsistent configuration to be exposed
> earlier, but that would not result in a functional change where the
> missing device would magically appear, would it? Is this change mainly
> to make the tests fail faster? If the 'slave device is not ready/reset',
> is there a recovery mechanism to recheck later?
> 
> Would you mind clarifying what happens after the timeout, and why the
> timeout would happen in the first place?

This issue is mostly present/seen with WCD938x codec due to its Linux 
device model.
WCD938x Codec has 3 Linux component drivers
1. TX Component (A soundwire device connected to TX Soundwire Master)
2. RX Component (A soundwire device connected to RX Soundwire Master)
3. Master Component (Linux component framework master for (1) and  (2) 
and registers ASoC codec)

Also we have only one reset for (1) and (2).

reset line is handled by (3)
There are two possibilities when the WCD938x reset can happen,

1. If reset happens earlier than probing (1) and (2) which is best case.

2. if reset happens after (1) and (2) are probed then SoundWire TX and 
RX master will have spend 2 + 2 secs waiting, Which is a long time out
Hence the patch.

TBH, the 2 sec timeout value was just a random number which I added at 
the start, we had to come up with some sensible value over the time 
anyway for that.

You could say why do we need wait itself in the first place.

The reason we need wait in first place is because, there is a danger of 
codec accessing registers even before enumeration is finished. Because 
most of the ASoC codec registration happens as part of codec/component 
driver probe function rather than status callback.

I hope this answers your questions.

thanks,
--srini

> 
> Thanks!
> 
>>
>> Reported-by: Srinivasa Rao Mandadapu <quic_srivasam@...cinc.com>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>> ---
>>
>> Changes since v1:
>> 	replaced HZ/10 with 100 as suggested by Pierre
>>
>>   drivers/soundwire/qcom.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 7367aa88b8ac..d6111f69d320 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -105,7 +105,7 @@
>>   
>>   #define SWRM_SPECIAL_CMD_ID	0xF
>>   #define MAX_FREQ_NUM		1
>> -#define TIMEOUT_MS		(2 * HZ)
>> +#define TIMEOUT_MS		100
>>   #define QCOM_SWRM_MAX_RD_LEN	0x1
>>   #define QCOM_SDW_MAX_PORTS	14
>>   #define DEFAULT_CLK_FREQ	9600000