[PATCH] soundwire: intel: Initialize clock stop timeout

[PATCH] soundwire: intel: Initialize clock stop timeout

[Bard Liao]

From: Sjoerd Simons <sjoerd@collabora.com>

The bus->clk_stop_timeout member is only initialized to a non-zero value
during the codec driver probe. This can lead to corner cases where this
value remains pegged at zero when the bus suspends, which results in an
endless loop in sdw_bus_wait_for_clk_prep_deprep().

Corner cases include configurations with no codecs described in the
firmware, or delays in probing codec drivers.

Initializing the default timeout to the smallest non-zero value avoid this
problem and allows for the existing logic to be preserved: the
bus->clk_stop_timeout is set as the maximum required by all codecs
connected on the bus.

Fixes: 1f2dcf3a154ac ("soundwire: intel: set dev_num_ida_min")
Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Chao Song <chao.song@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 25ec9c272239..78d35bb4852c 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1311,6 +1311,7 @@ static int intel_link_probe(struct auxiliary_device *auxdev,
 
 	bus->link_id = auxdev->id;
 	bus->dev_num_ida_min = INTEL_DEV_NUM_IDA_MIN;
+	bus->clk_stop_timeout = 1;
 
 	sdw_cdns_probe(cdns);
 
-- 
2.25.1

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
Subject: [PATCH] soundwire: intel: Initialize clock stop timeout
Link: https://lore.kernel.org/stable/20221020015624.1703950-1-yung-chuan.liao%40linux.intel.com

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[Pierre-Louis Bossart]

On 10/19/22 20:56, Bard Liao wrote:
> From: Sjoerd Simons <sjoerd@collabora.com>
> 
> The bus->clk_stop_timeout member is only initialized to a non-zero value
> during the codec driver probe. This can lead to corner cases where this
> value remains pegged at zero when the bus suspends, which results in an
> endless loop in sdw_bus_wait_for_clk_prep_deprep().
> 
> Corner cases include configurations with no codecs described in the
> firmware, or delays in probing codec drivers.
> 
> Initializing the default timeout to the smallest non-zero value avoid this
> problem and allows for the existing logic to be preserved: the
> bus->clk_stop_timeout is set as the maximum required by all codecs
> connected on the bus.
> 
> Fixes: 1f2dcf3a154ac ("soundwire: intel: set dev_num_ida_min")
> Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Chao Song <chao.song@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

this patch should be sent to GregKH/Linus as a 6.1-rcx fix, it does seem
to make the life of Arch/Debian users less miserable - for some reason
very large delays on driver probe seem to trigger this corner case and
make things even worse.

see https://github.com/thesofproject/linux/issues/3777 for details.

Thanks Vinod.

> ---
>  drivers/soundwire/intel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 25ec9c272239..78d35bb4852c 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1311,6 +1311,7 @@ static int intel_link_probe(struct auxiliary_device *auxdev,
>  
>  	bus->link_id = auxdev->id;
>  	bus->dev_num_ida_min = INTEL_DEV_NUM_IDA_MIN;
> +	bus->clk_stop_timeout = 1;
>  
>  	sdw_cdns_probe(cdns);
>  

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[Vinod Koul]

On 20-10-22, 09:56, Bard Liao wrote:
> From: Sjoerd Simons <sjoerd@collabora.com>
> 
> The bus->clk_stop_timeout member is only initialized to a non-zero value
> during the codec driver probe. This can lead to corner cases where this
> value remains pegged at zero when the bus suspends, which results in an
> endless loop in sdw_bus_wait_for_clk_prep_deprep().
> 
> Corner cases include configurations with no codecs described in the
> firmware, or delays in probing codec drivers.
> 
> Initializing the default timeout to the smallest non-zero value avoid this
> problem and allows for the existing logic to be preserved: the
> bus->clk_stop_timeout is set as the maximum required by all codecs
> connected on the bus.

Applied to fixes, thanks

-- 
~Vinod

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[Pierre-Louis Bossart]

On 10/28/22 06:28, Vinod Koul wrote:
> On 20-10-22, 09:56, Bard Liao wrote:
>> From: Sjoerd Simons <sjoerd@collabora.com>
>>
>> The bus->clk_stop_timeout member is only initialized to a non-zero value
>> during the codec driver probe. This can lead to corner cases where this
>> value remains pegged at zero when the bus suspends, which results in an
>> endless loop in sdw_bus_wait_for_clk_prep_deprep().
>>
>> Corner cases include configurations with no codecs described in the
>> firmware, or delays in probing codec drivers.
>>
>> Initializing the default timeout to the smallest non-zero value avoid this
>> problem and allows for the existing logic to be preserved: the
>> bus->clk_stop_timeout is set as the maximum required by all codecs
>> connected on the bus.
> 
> Applied to fixes, thanks

Thanks Vinod, was this sent to Greg/Linus? the last pull request I see
was for 6.1-rc1.
Arch Linux cherry-picked this patch but other distros did not, so quite
a few users are left with no audio card.

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[Vinod Koul]

On 09-11-22, 10:05, Pierre-Louis Bossart wrote:
> 
> 
> On 10/28/22 06:28, Vinod Koul wrote:
> > On 20-10-22, 09:56, Bard Liao wrote:
> >> From: Sjoerd Simons <sjoerd@collabora.com>
> >>
> >> The bus->clk_stop_timeout member is only initialized to a non-zero value
> >> during the codec driver probe. This can lead to corner cases where this
> >> value remains pegged at zero when the bus suspends, which results in an
> >> endless loop in sdw_bus_wait_for_clk_prep_deprep().
> >>
> >> Corner cases include configurations with no codecs described in the
> >> firmware, or delays in probing codec drivers.
> >>
> >> Initializing the default timeout to the smallest non-zero value avoid this
> >> problem and allows for the existing logic to be preserved: the
> >> bus->clk_stop_timeout is set as the maximum required by all codecs
> >> connected on the bus.
> > 
> > Applied to fixes, thanks
> 
> Thanks Vinod, was this sent to Greg/Linus? the last pull request I see
> was for 6.1-rc1.
> Arch Linux cherry-picked this patch but other distros did not, so quite
> a few users are left with no audio card.

https://git.kernel.org/torvalds/c/f014699cca9a9a28fbdc06a9225b54562154fc20


-- 
~Vinod

[PATCH] soundwire: intel: Initialize clock stop timeout

[Pierre-Louis Bossart]

From: Sjoerd Simons <sjoerd@collabora.com>

commit 13c30a755847c7e804e1bf755e66e3ff7b7f9367 upstream

The bus->clk_stop_timeout member is only initialized to a non-zero value
during the codec driver probe. This can lead to corner cases where this
value remains pegged at zero when the bus suspends, which results in an
endless loop in sdw_bus_wait_for_clk_prep_deprep().

Corner cases include configurations with no codecs described in the
firmware, or delays in probing codec drivers.

Initializing the default timeout to the smallest non-zero value avoid this
problem and allows for the existing logic to be preserved: the
bus->clk_stop_timeout is set as the maximum required by all codecs
connected on the bus.

Fixes: 1f2dcf3a154ac ("soundwire: intel: set dev_num_ida_min")
Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Chao Song <chao.song@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Link: https://lore.kernel.org/r/20221020015624.1703950-1-yung-chuan.liao@linux.intel.com
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---

This should be applied to all stable kernels between 5.13.x and
6.0.x. Backporting to avoid a conflict with 1f2dcf3a154ac ("soundwire:
intel: set dev_num_ida_min") already added for 6.1


 drivers/soundwire/intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index af6c1a93372d9..002bc26b525e8 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1307,6 +1307,7 @@ static int intel_link_probe(struct auxiliary_device *auxdev,
 	cdns->msg_count = 0;
 
 	bus->link_id = auxdev->id;
+	bus->clk_stop_timeout = 1;
 
 	sdw_cdns_probe(cdns);
 
-- 
2.34.1

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[Greg KH]

On Mon, Dec 05, 2022 at 11:06:00AM -0600, Pierre-Louis Bossart wrote:
> From: Sjoerd Simons <sjoerd@collabora.com>
> 
> commit 13c30a755847c7e804e1bf755e66e3ff7b7f9367 upstream
> 
> The bus->clk_stop_timeout member is only initialized to a non-zero value
> during the codec driver probe. This can lead to corner cases where this
> value remains pegged at zero when the bus suspends, which results in an
> endless loop in sdw_bus_wait_for_clk_prep_deprep().
> 
> Corner cases include configurations with no codecs described in the
> firmware, or delays in probing codec drivers.
> 
> Initializing the default timeout to the smallest non-zero value avoid this
> problem and allows for the existing logic to be preserved: the
> bus->clk_stop_timeout is set as the maximum required by all codecs
> connected on the bus.
> 
> Fixes: 1f2dcf3a154ac ("soundwire: intel: set dev_num_ida_min")

This commit is is only in 6.1-rc1, so why does it need to go to any
older kernels?  Is this tag not correct?

thanks,

greg k-h

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[Pierre-Louis Bossart]

On 12/5/22 12:33, Greg KH wrote:
> On Mon, Dec 05, 2022 at 11:06:00AM -0600, Pierre-Louis Bossart wrote:
>> From: Sjoerd Simons <sjoerd@collabora.com>
>>
>> commit 13c30a755847c7e804e1bf755e66e3ff7b7f9367 upstream
>>
>> The bus->clk_stop_timeout member is only initialized to a non-zero value
>> during the codec driver probe. This can lead to corner cases where this
>> value remains pegged at zero when the bus suspends, which results in an
>> endless loop in sdw_bus_wait_for_clk_prep_deprep().
>>
>> Corner cases include configurations with no codecs described in the
>> firmware, or delays in probing codec drivers.
>>
>> Initializing the default timeout to the smallest non-zero value avoid this
>> problem and allows for the existing logic to be preserved: the
>> bus->clk_stop_timeout is set as the maximum required by all codecs
>> connected on the bus.
>>
>> Fixes: 1f2dcf3a154ac ("soundwire: intel: set dev_num_ida_min")
> 
> This commit is is only in 6.1-rc1, so why does it need to go to any
> older kernels?  Is this tag not correct?

I don't recall why this tag was selected, it's clearly not related
functionality-wise. I vaguely recall a discussion with Bard Liao on
this... And yes sure enough here it is [1], it was to indicate a
conflict but that was confusing in hindsight.

At any rate, this one-line change is really needed, some distributions
such as Arch back-ported this change but most did not, and users don't
have a working setup.

[1] https://github.com/thesofproject/linux/pull/3911#issuecomment-1284750003

Re: [PATCH] soundwire: intel: Initialize clock stop timeout

[Greg KH]

On Mon, Dec 05, 2022 at 01:33:06PM -0600, Pierre-Louis Bossart wrote:
> 
> 
> On 12/5/22 12:33, Greg KH wrote:
> > On Mon, Dec 05, 2022 at 11:06:00AM -0600, Pierre-Louis Bossart wrote:
> >> From: Sjoerd Simons <sjoerd@collabora.com>
> >>
> >> commit 13c30a755847c7e804e1bf755e66e3ff7b7f9367 upstream
> >>
> >> The bus->clk_stop_timeout member is only initialized to a non-zero value
> >> during the codec driver probe. This can lead to corner cases where this
> >> value remains pegged at zero when the bus suspends, which results in an
> >> endless loop in sdw_bus_wait_for_clk_prep_deprep().
> >>
> >> Corner cases include configurations with no codecs described in the
> >> firmware, or delays in probing codec drivers.
> >>
> >> Initializing the default timeout to the smallest non-zero value avoid this
> >> problem and allows for the existing logic to be preserved: the
> >> bus->clk_stop_timeout is set as the maximum required by all codecs
> >> connected on the bus.
> >>
> >> Fixes: 1f2dcf3a154ac ("soundwire: intel: set dev_num_ida_min")
> > 
> > This commit is is only in 6.1-rc1, so why does it need to go to any
> > older kernels?  Is this tag not correct?
> 
> I don't recall why this tag was selected, it's clearly not related
> functionality-wise. I vaguely recall a discussion with Bard Liao on
> this... And yes sure enough here it is [1], it was to indicate a
> conflict but that was confusing in hindsight.
> 
> At any rate, this one-line change is really needed, some distributions
> such as Arch back-ported this change but most did not, and users don't
> have a working setup.

Ok, now queued up, thanks.

greg k-h