Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

soundwire: intel: Initialize clock stop timeout #3911

Merged
merged 1 commit into from
Oct 19, 2022
Merged

soundwire: intel: Initialize clock stop timeout #3911

merged 1 commit into from
Oct 19, 2022

Conversation

sjoerdsimons
Copy link

The clk_stop_timeout field of the bus never got initialized leading to an endless loop when trying to stop the clock if it fails.

Signed-off-by: Sjoerd Simons sjoerd@collabora.com

@sofci
Copy link
Collaborator

sofci commented Oct 8, 2022

Can one of the admins verify this patch?

reply test this please to run this test once

@sjoerdsimons sjoerdsimons mentioned this pull request Oct 8, 2022
Closed
@aiChaoSONG
Copy link
Collaborator

test this please

@aslfv aslfv mentioned this pull request Oct 9, 2022
Closed
RanderWang
RanderWang previously approved these changes Oct 10, 2022
View reviewed changes
Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

plbossart
plbossart requested changes Oct 10, 2022
View reviewed changes
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goodness, this is one nice catch @sjoerdsimons. Many thanks indeed for the patch.

I completely agree with the need to initialize this variable, but suggested two improvements below. If you don't mind re-pushing a new version that would be great.

drivers/soundwire/intel.c Outdated Show resolved Hide resolved
drivers/soundwire/intel.c Outdated Show resolved Hide resolved
plbossart
plbossart previously approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sjoerdsimons

Happy Friday!

aiChaoSONG
aiChaoSONG previously approved these changes Oct 17, 2022
View reviewed changes
RanderWang
RanderWang previously approved these changes Oct 17, 2022
View reviewed changes
@plbossart
Copy link
Member

SOFCI TEST

@sjoerdsimons
soundwire: intel: Initialize clock stop timeout
840bc2c 
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.

Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
@sjoerdsimons
Copy link
Author

Fwiw repushed my commit to have < 75 characters on the commit messages as that caused checkpatch to fail

@aiChaoSONG
Copy link
Collaborator

SOFCI TEST

@plbossart
Copy link
Member

@bardliao can you review, merge and send to Vinod ASAP? I think this should even be Cc: stable@vger.kernel.org

aiChaoSONG
aiChaoSONG approved these changes Oct 19, 2022
View reviewed changes
Copy link
Collaborator

@aiChaoSONG aiChaoSONG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout on CML_RVP_SDW is not related to this patch.

@bardliao bardliao merged commit eb3c333 into thesofproject:topic/sof-dev Oct 19, 2022
@bardliao
Copy link
Collaborator

@plbossart @sjoerdsimons Do we need a Fixes tag?

@plbossart
Copy link
Member

Maybe 29a269c ("soundwire:` intel: move to auxiliary bus")?

This problem existed since the beginning but the patch will not apply on earlier versions since we completely changed the structure of the code.

@bardliao
Copy link
Collaborator

Maybe 29a269c ("soundwire:` intel: move to auxiliary bus")?

This problem existed since the beginning but the patch will not apply on earlier versions since we completely changed the structure of the code.

I tested it and this patch doesn't apply to 29a269c ("soundwire:` intel: move to auxiliary bus")

It can apply to 1f2dcf3 ("soundwire: intel: set dev_num_ida_min"). So, I will add Fixes: 1f2dcf3a154ac ("soundwire: intel: set dev_num_ida_min")
Below is a part of git blame result.

83e129afbe5c4 (Pierre-Louis Bossart 2020-06-01 02:20:58 +0800 1512)     cdns->msg_count = 0;
83e129afbe5c4 (Pierre-Louis Bossart 2020-06-01 02:20:58 +0800 1513)
29a269c6f5482 (Pierre-Louis Bossart 2021-05-11 13:21:32 +0800 1514)     bus->link_id = auxdev->id;
1f2dcf3a154ac (Pierre-Louis Bossart 2022-08-23 12:50:04 +0800 1515)     bus->dev_num_ida_min = INTEL_DEV_NUM_IDA_MIN;
4475109a527ac (Sjoerd Simons        2022-10-08 21:57:51 +0200 1516)     bus->clk_stop_timeout = 1;
71bb8a1b059ec (Vinod Koul           2017-12-14 11:19:43 +0530 1517)
83e129afbe5c4 (Pierre-Louis Bossart 2020-06-01 02:20:58 +0800 1518)     sdw_cdns_probe(cdns);
71bb8a1b059ec (Vinod Koul           2017-12-14 11:19:43 +0530 1519)
0b59d4c947589 (Pierre-Louis Bossart 2022-09-20 01:57:18 +0800 1520)     /* Set ops */
b6109dd6dc9fb (Pierre-Louis Bossart 2020-06-01 02:20:57 +0800 1521)     bus->ops = &sdw_intel_ops;

@paulmenzel
Copy link

There is a small typo:

Initializing the default timeout to the smallest non-zero value avoid this

avoids

@plbossart
Copy link
Member

too late, it's posted upstream now...

@paulmenzel
Copy link

This probably also fixes issue #4540, the one created for the Dell XPS 13 9315.

@paulmenzel paulmenzel mentioned this pull request Dec 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plbossart plbossart plbossart approved these changes

@bardliao bardliao bardliao approved these changes

@aiChaoSONG aiChaoSONG aiChaoSONG approved these changes

@shumingfan shumingfan Awaiting requested review from shumingfan

@oder-chiou oder-chiou Awaiting requested review from oder-chiou

@jack-cy-yu jack-cy-yu Awaiting requested review from jack-cy-yu

@ryans-lee ryans-lee Awaiting requested review from ryans-lee

@RanderWang RanderWang Awaiting requested review from RanderWang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sjoerdsimons @sofci @aiChaoSONG @plbossart @bardliao @paulmenzel @RanderWang