ASoC: SOF: use spin_lock_irq to protect resource by RanderWang · Pull Request #434 · thesofproject/linux

RanderWang · 2018-12-13T05:23:34Z

ipc_lock is used to protect ipc message list shared by user
contexts and a workqueue. So there is no need to save interrupt
status with spin_lock_irqsave, just use spin_lock_irq.

it fixs #422

ranj063 · 2018-12-13T07:14:31Z

@RanderWang im trying to educate myself better on irq...what is the definition of a IRQ function?

RanderWang · 2018-12-13T07:34:05Z

@RanderWang im trying to educate myself better on irq...what is the definition of a IRQ function?

please refer to https://geek-university.com/linux/irq-interrupt-request/. I will refine function to handler

ranj063 · 2018-12-13T07:43:42Z

@RanderWang handler automatically clarifies what you mean here. Thanks!

ranj063 · 2018-12-13T07:45:07Z

@RanderWang btw which interrupts doe spin_lock_irq() disable? Does it disable all interrupts?

RanderWang · 2018-12-13T07:46:36Z

@RanderWang btw which interrupts doe spin_lock_irq() disable? Does it disable all interrupts?

interrupt in local cpu, not in other cpu

keyonjie

Hi Rander,
can you add more comments for those functions according to Takashi's comments, similar as:
/* ipc_lock should not held by caller, and we should not call it in interrupt context */

RanderWang · 2018-12-13T09:48:28Z

@keyonjie It is strange to add these comments before spin_lock. The usage of spin_lock should be in everyone's mind. I don't see any comment to spin_lock in HDA.

plbossart · 2018-12-13T13:35:24Z

@keyonjie It is strange to add these comments before spin_lock. The usage of spin_lock should be in everyone's mind. I don't see any comment to spin_lock in HDA.

@RanderWang [sarcasm] it's so clear we are at the 3rd PR to fix this... [/sarcasm] Please document what the assumptions are so that everyone can figure it out for themselves. It's not because others don't explain their code that we should follow their example, and the questions above show that this knowledge on spin_locks is not widely shared in the team.

plbossart

The commit message is a circular explanation that makes little sense. It should clearly explain why the ipc functions are not called in IRQ handler and thus the use of spin_lock_irqsave is not needed.
Also we need similar comments, e.g. before each function to state why the spin_lock_irq is used. No need to explain this at every line.

ranj063 · 2018-12-13T18:28:39Z

@plbossart thanks for this information: "It should clearly explain why the ipc functions are not called in IRQ handler and thus the use of spin_lock_irqsave is not needed"

I was so confused about spin_lock_irq and spin_lock_irqsave...this makes it clear as day!

keyonjie · 2018-12-14T01:21:34Z

@keyonjie It is strange to add these comments before spin_lock. The usage of spin_lock should be in everyone's mind. I don't see any comment to spin_lock in HDA.

@RanderWang I meant before the functions, not before each calling to spin_lock. similar to this one in sst-ipc.c:

/* locks held by caller */
void sst_ipc_tx_msg_reply_complete(struct sst_generic_ipc *ipc,
	struct ipc_message *msg)
{
	msg->complete = true;

	if (!msg->wait)
		list_add_tail(&msg->list, &ipc->empty_list);
	else
		wake_up(&msg->waitq);
}
EXPORT_SYMBOL_GPL(sst_ipc_tx_msg_reply_complete);

RanderWang · 2018-12-14T09:49:21Z

ok, I will update later

RanderWang · 2018-12-14T12:06:17Z

sorry, I cant connect to my work PC via VPN, so I can't update my PR today

RanderWang · 2018-12-15T14:17:38Z

update PR, thanks for review

plbossart

Please reword the comment and commit message. remember, the problem is not about being right but about convincing maintainers that you are right. The feedback on spin_locks came from Takashi Iwai, and you want a clear/logical explanation to avoid yet another round of reviews.

plbossart · 2018-12-15T17:18:27Z

Sorry @RanderWang, this does not cut it. What is needed is a clear explanation on why the conditions to use spin_lock_irq are met in the SOF IPC, and why irqsave is not needed. Your comments are unclear and raise doubts for reviewers.

Your commit message is also totally unclear. It's a circular explanation with no logical line of thought. Please reword it by explaining how the IPC is used, and based on this why spin_lock_irq is sufficient. We don't want a general lecture on spin lock, but rather an explanation of how the IPC works.

I may focus on lecture and miss the target. Ok, I will update PR later.

RanderWang · 2018-12-17T09:30:39Z

update PR, thanks!

plbossart

minor comment issue, thanks

plbossart · 2018-12-17T16:10:34Z

grammar: There is no need

yes, I will refine it

RanderWang · 2018-12-18T04:15:30Z

update PR, thanks!

Ipc_lock is used to protect ipc message list shared by user contexts and a workqueue. So there is no need to save interrupt status with spin_lock_irqsave, just use spin_lock_irq. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

plbossart

Thanks @RanderWang

With support of private stack, relevant tests must pass on powerpc64. #./test_progs -t struct_ops_private_stack thesofproject#434/1 struct_ops_private_stack/private_stack:OK thesofproject#434/2 struct_ops_private_stack/private_stack_fail:OK thesofproject#434/3 struct_ops_private_stack/private_stack_recur:OK thesofproject#434 struct_ops_private_stack:OK Summary: 1/3 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Abhishek Dubey <adubey@linux.ibm.com> Tested-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com> Reviewed-by: Hari Bathini <hbathini@linux.ibm.com> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com> Link: https://patch.msgid.link/20260401103215.104438-2-adubey@linux.ibm.com

Puranjay Mohan says: ==================== bpf,arm64: Add support for BPF Arena Changes in V4 V3: https://lore.kernel.org/bpf/20240323103057.26499-1-puranjay12@gmail.com/ - Use more descriptive variable names. - Use insn_is_cast_user() helper. Changes in V3 V2: https://lore.kernel.org/bpf/20240321153102.103832-1-puranjay12@gmail.com/ - Optimize bpf_addr_space_cast as suggested by Xu Kuohai Changes in V2 V1: https://lore.kernel.org/bpf/20240314150003.123020-1-puranjay12@gmail.com/ - Fix build warnings by using 5 in place of 32 as DONT_CLEAR marker. R5 is not mapped to any BPF register so it can safely be used here. This series adds the support for PROBE_MEM32 and bpf_addr_space_cast instructions to the ARM64 BPF JIT. These two instructions allow the enablement of BPF Arena. All arena related selftests are passing. [root@ip-172-31-6-62 bpf]# ./test_progs -a "*arena*" thesofproject#3/1 arena_htab/arena_htab_llvm:OK thesofproject#3/2 arena_htab/arena_htab_asm:OK thesofproject#3 arena_htab:OK thesofproject#4/1 arena_list/arena_list_1:OK thesofproject#4/2 arena_list/arena_list_1000:OK thesofproject#4 arena_list:OK thesofproject#434/1 verifier_arena/basic_alloc1:OK thesofproject#434/2 verifier_arena/basic_alloc2:OK thesofproject#434/3 verifier_arena/basic_alloc3:OK thesofproject#434/4 verifier_arena/iter_maps1:OK thesofproject#434/5 verifier_arena/iter_maps2:OK thesofproject#434/6 verifier_arena/iter_maps3:OK thesofproject#434 verifier_arena:OK Summary: 3/10 PASSED, 0 SKIPPED, 0 FAILED This will need the patch [1] that introduced insn_is_cast_user() helper to build. The verifier_arena selftest could fail in the CI because the following commit[2] is missing from bpf-next: [1] https://lore.kernel.org/bpf/20240324183226.29674-1-puranjay12@gmail.com/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=fa3550dca8f02ec312727653a94115ef3ab68445 Here is a CI run with all dependencies added: kernel-patches/bpf#6641 ==================== Link: https://lore.kernel.org/r/20240325150716.4387-1-puranjay12@gmail.com Signed-off-by: Alexei Starovoitov <ast@kernel.org>

RanderWang mentioned this pull request Dec 13, 2018

Upstream: Patch 5/14: check if we need to use spin_lock_irqsave #422

Closed

RanderWang requested a review from keyonjie December 13, 2018 05:32

RanderWang force-pushed the irq_save branch from ef7fd10 to 25936e5 Compare December 13, 2018 07:37

keyonjie reviewed Dec 13, 2018

View reviewed changes

plbossart requested changes Dec 13, 2018

View reviewed changes

RanderWang force-pushed the irq_save branch 3 times, most recently from e1816f2 to 94b4e12 Compare December 15, 2018 02:49

plbossart requested changes Dec 15, 2018

View reviewed changes

RanderWang force-pushed the irq_save branch 4 times, most recently from 289575e to 38e79ce Compare December 17, 2018 08:35

plbossart requested changes Dec 17, 2018

View reviewed changes

RanderWang force-pushed the irq_save branch from 38e79ce to 0c1b486 Compare December 18, 2018 04:13

RanderWang force-pushed the irq_save branch from 0c1b486 to 5dcd156 Compare December 18, 2018 04:16

ASoC: SOF: use spin_lock_irq to protect resource

5dcd156

Ipc_lock is used to protect ipc message list shared by user contexts and a workqueue. So there is no need to save interrupt status with spin_lock_irqsave, just use spin_lock_irq. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

plbossart approved these changes Dec 18, 2018

View reviewed changes

plbossart merged commit a6a1981 into thesofproject:topic/sof-dev Dec 18, 2018

Conversation

RanderWang commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented Dec 13, 2018

Uh oh!

RanderWang commented Dec 13, 2018

Uh oh!

ranj063 commented Dec 13, 2018

Uh oh!

ranj063 commented Dec 13, 2018

Uh oh!

RanderWang commented Dec 13, 2018

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Dec 13, 2018

Uh oh!

plbossart commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Dec 13, 2018

Uh oh!

keyonjie commented Dec 14, 2018

Uh oh!

RanderWang commented Dec 14, 2018

Uh oh!

RanderWang commented Dec 14, 2018

Uh oh!

RanderWang commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Dec 15, 2018

Choose a reason for hiding this comment

Uh oh!

RanderWang Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Dec 17, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

RanderWang Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Dec 18, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RanderWang commented Dec 13, 2018 •

edited

Loading

plbossart commented Dec 13, 2018 •

edited

Loading

RanderWang commented Dec 15, 2018 •

edited

Loading