Skip to content

Conversation

@slawblauciak
Copy link
Collaborator

In some situations the domain clear routine might encounter
a case, when the number of clients is decremented and 0,
as such, a subsequent subdivision will result in an integer underflow.

Signed-off-by: Slawomir Blauciak slawomir.blauciak@linux.intel.com

Fixes #3950
Fixes #3949
Fixes #3947

In some situations the domain clear routine might encounter
a case, when the number of clients is decremented and 0,
as such, a subsequent subdivision will result in an integer underflow.

Signed-off-by: Slawomir Blauciak <slawomir.blauciak@linux.intel.com>
@slawblauciak slawblauciak requested a review from keyonjie April 23, 2021 15:28
@slawblauciak slawblauciak requested a review from mrajwa as a code owner April 23, 2021 15:28
Comment on lines +334 to +335
if (atomic_read(&sch->domain->num_clients)) {
count = atomic_sub(&sch->domain->num_clients, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if num_clients changes here between reading and subtracting. @lgirdwood @plbossart what do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if num_clients changes here between reading and subtracting. @lgirdwood @plbossart what do you think ?

We hold the domain->lock during these checks so that should not happen.

BTW, we are kind of mess in this client/task management, I am working on refining them, please see the PR here: #4089

Copy link
Member

Choose a reason for hiding this comment

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

ok, this is fine with the locks held.

Copy link
Contributor

@keyonjie keyonjie left a comment

Choose a reason for hiding this comment

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

Nice observation @slawblauciak , we are somewhat mess in this client/task management part, better to do a thorough cleanup here, please see my PR #4089.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Good find @slawblauciak !

@lgirdwood
Copy link
Member

@keyonjie @slawblauciak can we use this in TGL 12 and integrate this alongside the cleanup in v1.8 ?

@slawblauciak
Copy link
Collaborator Author

@keyonjie @slawblauciak can we use this in TGL 12 and integrate this alongside the cleanup in v1.8 ?

Yeah, the plan is to use it with TGL 12 for sure. Not certain about Keyon's change yet.

@lgirdwood
Copy link
Member

@keyonjie lets clean up for v1.8 if things are messy today.

@lgirdwood lgirdwood merged commit b500999 into thesofproject:main Apr 26, 2021
@keyonjie
Copy link
Contributor

@keyonjie lets clean up for v1.8 if things are messy today.

sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants