From: Mark Brown <broonie@kernel.org> To: "Ivan T. Ivanov" <iivanov@suse.de> Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, "Madhavan T . Venkataraman" <madvenka@linux.microsoft.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1] arm64: Add initial set of stack unwinder self tests Date: Wed, 29 Jun 2022 16:59:03 +0100 [thread overview] Message-ID: <Yrx2xwbYMfcl8Qok@sirena.org.uk> (raw) In-Reply-To: <20220624141000.88120-2-iivanov@suse.de> [-- Attachment #1: Type: text/plain, Size: 1476 bytes --] On Fri, Jun 24, 2022 at 05:10:00PM +0300, Ivan T. Ivanov wrote: > Add kunit tests for obvious cases where stack unwind could be needed. > Like these: > > * Unwind a separate task > * Unwind starting from caller > * Unwind from irq context > * Unwind from kprobe handler called via ftrace > * Unwind from ftrace handler > * Unwind through kretprobed function > * Unwind from kretprobe handler > > Tests are completely based on code used in s390 unwinder tests. > Cases which where not relevant to aarch64 where removed and > some places where adjusted to address aarch64 specifics. I think this would be a bit easier to digest if it were a series which builds things up with the test cases in individual patches, or at least things like ftrace and kprobes split out a bit more, rather than every single test all at once. I've got a few *very* superficial comments below, I think the code is fine but there's several moving pieces to check. > +/* > + * Calls test_arch_stack_walk() which is handy wrapper of aarch64 unwind > + * functionality, and verifies that the result contains unwindme_func2 > + *followed by unwindme_func1. Missing space. > + ret = register_ftrace_function(fops); > + if (!ret) { > + ret = test_unwind_ftraced_func(u); > + unregister_ftrace_function(fops); > + } else { > + kunit_err(current_test, > + "failed to register ftrace handler (%d)\n", ret); > + } Shouldn't we return an error here? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org> To: "Ivan T. Ivanov" <iivanov@suse.de> Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, "Madhavan T . Venkataraman" <madvenka@linux.microsoft.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1] arm64: Add initial set of stack unwinder self tests Date: Wed, 29 Jun 2022 16:59:03 +0100 [thread overview] Message-ID: <Yrx2xwbYMfcl8Qok@sirena.org.uk> (raw) In-Reply-To: <20220624141000.88120-2-iivanov@suse.de> [-- Attachment #1.1: Type: text/plain, Size: 1476 bytes --] On Fri, Jun 24, 2022 at 05:10:00PM +0300, Ivan T. Ivanov wrote: > Add kunit tests for obvious cases where stack unwind could be needed. > Like these: > > * Unwind a separate task > * Unwind starting from caller > * Unwind from irq context > * Unwind from kprobe handler called via ftrace > * Unwind from ftrace handler > * Unwind through kretprobed function > * Unwind from kretprobe handler > > Tests are completely based on code used in s390 unwinder tests. > Cases which where not relevant to aarch64 where removed and > some places where adjusted to address aarch64 specifics. I think this would be a bit easier to digest if it were a series which builds things up with the test cases in individual patches, or at least things like ftrace and kprobes split out a bit more, rather than every single test all at once. I've got a few *very* superficial comments below, I think the code is fine but there's several moving pieces to check. > +/* > + * Calls test_arch_stack_walk() which is handy wrapper of aarch64 unwind > + * functionality, and verifies that the result contains unwindme_func2 > + *followed by unwindme_func1. Missing space. > + ret = register_ftrace_function(fops); > + if (!ret) { > + ret = test_unwind_ftraced_func(u); > + unregister_ftrace_function(fops); > + } else { > + kunit_err(current_test, > + "failed to register ftrace handler (%d)\n", ret); > + } Shouldn't we return an error here? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-06-29 15:59 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-24 14:09 [PATCH 0/1] arm64: Add stack unwinder unit tests Ivan T. Ivanov 2022-06-24 14:09 ` Ivan T. Ivanov 2022-06-24 14:10 ` [PATCH 1/1] arm64: Add initial set of stack unwinder self tests Ivan T. Ivanov 2022-06-24 14:10 ` Ivan T. Ivanov 2022-06-29 15:59 ` Mark Brown [this message] 2022-06-29 15:59 ` Mark Brown 2022-06-30 6:55 ` Ivan T. Ivanov 2022-06-30 6:55 ` Ivan T. Ivanov 2022-07-06 12:31 ` [PATCH 0/1] arm64: Add stack unwinder unit tests Will Deacon 2022-07-06 12:31 ` Will Deacon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Yrx2xwbYMfcl8Qok@sirena.org.uk \ --to=broonie@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=iivanov@suse.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=madvenka@linux.microsoft.com \ --cc=mark.rutland@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.