linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] soc: apple: rtkit: Do not copy the reg state structure to the stack
@ 2023-02-11  9:13 Asahi Lina
  2023-02-13 17:15 ` Eric Curtin
  0 siblings, 1 reply; 3+ messages in thread
From: Asahi Lina @ 2023-02-11  9:13 UTC (permalink / raw)
  To: Arnd Bergmann, Hector Martin, Sven Peter
  Cc: Alyssa Rosenzweig, asahi, linux-arm-kernel, linux-kernel, Asahi Lina

The register state struct is 848 bytes, which ends up bloating the
apple_rtkit_crashlog_dump_regs stack frame beyond 1024 on some
32-bit platforms, triggering compile warnings.

This doesn't matter for 64BIT/ARM64, but there's also no good reason to
copy the structure to the stack in this case. We can use __packed to
avoid alignment issues, there are no double-read hazards, and this is a
fatal error path so performance does not matter.

Fixes: 22991d8d5725 ("soc: apple: rtkit: Add register dump decoding to crashlog")
Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 drivers/soc/apple/rtkit-crashlog.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/soc/apple/rtkit-crashlog.c b/drivers/soc/apple/rtkit-crashlog.c
index dfa74b32eda2..8319e365110b 100644
--- a/drivers/soc/apple/rtkit-crashlog.c
+++ b/drivers/soc/apple/rtkit-crashlog.c
@@ -57,7 +57,7 @@ struct apple_rtkit_crashlog_regs {
 	u64 unk_X;
 	u64 esr;
 	u64 unk_Z;
-};
+} __packed;
 static_assert(sizeof(struct apple_rtkit_crashlog_regs) == 0x350);
 
 static void apple_rtkit_crashlog_dump_str(struct apple_rtkit *rtk, u8 *bfr,
@@ -126,18 +126,18 @@ static void apple_rtkit_crashlog_dump_mailbox(struct apple_rtkit *rtk, u8 *bfr,
 static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr,
 					   size_t size)
 {
-	struct apple_rtkit_crashlog_regs regs;
+	struct apple_rtkit_crashlog_regs *regs;
 	const char *el;
 	int i;
 
-	if (size < sizeof(regs)) {
+	if (size < sizeof(*regs)) {
 		dev_warn(rtk->dev, "RTKit: Regs section too small: 0x%zx", size);
 		return;
 	}
 
-	memcpy(&regs, bfr, sizeof(regs));
+	regs = (struct apple_rtkit_crashlog_regs *)bfr;
 
-	switch (regs.psr & PSR_MODE_MASK) {
+	switch (regs->psr & PSR_MODE_MASK) {
 	case PSR_MODE_EL0t:
 		el = "EL0t";
 		break;
@@ -160,11 +160,11 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr,
 
 	dev_warn(rtk->dev, "RTKit: Exception dump:");
 	dev_warn(rtk->dev, "  == Exception taken from %s ==", el);
-	dev_warn(rtk->dev, "  PSR    = 0x%llx", regs.psr);
-	dev_warn(rtk->dev, "  PC     = 0x%llx\n", regs.pc);
-	dev_warn(rtk->dev, "  ESR    = 0x%llx\n", regs.esr);
-	dev_warn(rtk->dev, "  FAR    = 0x%llx\n", regs.far);
-	dev_warn(rtk->dev, "  SP     = 0x%llx\n", regs.sp);
+	dev_warn(rtk->dev, "  PSR    = 0x%llx", regs->psr);
+	dev_warn(rtk->dev, "  PC     = 0x%llx\n", regs->pc);
+	dev_warn(rtk->dev, "  ESR    = 0x%llx\n", regs->esr);
+	dev_warn(rtk->dev, "  FAR    = 0x%llx\n", regs->far);
+	dev_warn(rtk->dev, "  SP     = 0x%llx\n", regs->sp);
 	dev_warn(rtk->dev, "\n");
 
 	for (i = 0; i < 31; i += 4) {
@@ -172,12 +172,12 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr,
 			dev_warn(rtk->dev,
 					 "  x%02d-x%02d = %016llx %016llx %016llx %016llx\n",
 					 i, i + 3,
-					 regs.regs[i], regs.regs[i + 1],
-					 regs.regs[i + 2], regs.regs[i + 3]);
+					 regs->regs[i], regs->regs[i + 1],
+					 regs->regs[i + 2], regs->regs[i + 3]);
 		else
 			dev_warn(rtk->dev,
 					 "  x%02d-x%02d = %016llx %016llx %016llx\n", i, i + 3,
-					 regs.regs[i], regs.regs[i + 1], regs.regs[i + 2]);
+					 regs->regs[i], regs->regs[i + 1], regs->regs[i + 2]);
 	}
 
 	dev_warn(rtk->dev, "\n");
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] soc: apple: rtkit: Do not copy the reg state structure to the stack
  2023-02-11  9:13 [PATCH] soc: apple: rtkit: Do not copy the reg state structure to the stack Asahi Lina
@ 2023-02-13 17:15 ` Eric Curtin
  2023-02-13 19:22   ` Arnd Bergmann
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Curtin @ 2023-02-13 17:15 UTC (permalink / raw)
  To: Asahi Lina
  Cc: Arnd Bergmann, Hector Martin, Sven Peter, Alyssa Rosenzweig,
	asahi, linux-arm-kernel, linux-kernel

On Sat, 11 Feb 2023 at 09:21, Asahi Lina <lina@asahilina.net> wrote:
>
> The register state struct is 848 bytes, which ends up bloating the
> apple_rtkit_crashlog_dump_regs stack frame beyond 1024 on some
> 32-bit platforms, triggering compile warnings.
>
> This doesn't matter for 64BIT/ARM64, but there's also no good reason to
> copy the structure to the stack in this case. We can use __packed to
> avoid alignment issues, there are no double-read hazards, and this is a
> fatal error path so performance does not matter.
>
> Fixes: 22991d8d5725 ("soc: apple: rtkit: Add register dump decoding to crashlog")
> Signed-off-by: Asahi Lina <lina@asahilina.net>

LGTM, less copies too.

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

Is mise le meas/Regards,

Eric Curtin

> ---
>  drivers/soc/apple/rtkit-crashlog.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/soc/apple/rtkit-crashlog.c b/drivers/soc/apple/rtkit-crashlog.c
> index dfa74b32eda2..8319e365110b 100644
> --- a/drivers/soc/apple/rtkit-crashlog.c
> +++ b/drivers/soc/apple/rtkit-crashlog.c
> @@ -57,7 +57,7 @@ struct apple_rtkit_crashlog_regs {
>         u64 unk_X;
>         u64 esr;
>         u64 unk_Z;
> -};
> +} __packed;
>  static_assert(sizeof(struct apple_rtkit_crashlog_regs) == 0x350);
>
>  static void apple_rtkit_crashlog_dump_str(struct apple_rtkit *rtk, u8 *bfr,
> @@ -126,18 +126,18 @@ static void apple_rtkit_crashlog_dump_mailbox(struct apple_rtkit *rtk, u8 *bfr,
>  static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr,
>                                            size_t size)
>  {
> -       struct apple_rtkit_crashlog_regs regs;
> +       struct apple_rtkit_crashlog_regs *regs;
>         const char *el;
>         int i;
>
> -       if (size < sizeof(regs)) {
> +       if (size < sizeof(*regs)) {
>                 dev_warn(rtk->dev, "RTKit: Regs section too small: 0x%zx", size);
>                 return;
>         }
>
> -       memcpy(&regs, bfr, sizeof(regs));
> +       regs = (struct apple_rtkit_crashlog_regs *)bfr;
>
> -       switch (regs.psr & PSR_MODE_MASK) {
> +       switch (regs->psr & PSR_MODE_MASK) {
>         case PSR_MODE_EL0t:
>                 el = "EL0t";
>                 break;
> @@ -160,11 +160,11 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr,
>
>         dev_warn(rtk->dev, "RTKit: Exception dump:");
>         dev_warn(rtk->dev, "  == Exception taken from %s ==", el);
> -       dev_warn(rtk->dev, "  PSR    = 0x%llx", regs.psr);
> -       dev_warn(rtk->dev, "  PC     = 0x%llx\n", regs.pc);
> -       dev_warn(rtk->dev, "  ESR    = 0x%llx\n", regs.esr);
> -       dev_warn(rtk->dev, "  FAR    = 0x%llx\n", regs.far);
> -       dev_warn(rtk->dev, "  SP     = 0x%llx\n", regs.sp);
> +       dev_warn(rtk->dev, "  PSR    = 0x%llx", regs->psr);
> +       dev_warn(rtk->dev, "  PC     = 0x%llx\n", regs->pc);
> +       dev_warn(rtk->dev, "  ESR    = 0x%llx\n", regs->esr);
> +       dev_warn(rtk->dev, "  FAR    = 0x%llx\n", regs->far);
> +       dev_warn(rtk->dev, "  SP     = 0x%llx\n", regs->sp);
>         dev_warn(rtk->dev, "\n");
>
>         for (i = 0; i < 31; i += 4) {
> @@ -172,12 +172,12 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr,
>                         dev_warn(rtk->dev,
>                                          "  x%02d-x%02d = %016llx %016llx %016llx %016llx\n",
>                                          i, i + 3,
> -                                        regs.regs[i], regs.regs[i + 1],
> -                                        regs.regs[i + 2], regs.regs[i + 3]);
> +                                        regs->regs[i], regs->regs[i + 1],
> +                                        regs->regs[i + 2], regs->regs[i + 3]);
>                 else
>                         dev_warn(rtk->dev,
>                                          "  x%02d-x%02d = %016llx %016llx %016llx\n", i, i + 3,
> -                                        regs.regs[i], regs.regs[i + 1], regs.regs[i + 2]);
> +                                        regs->regs[i], regs->regs[i + 1], regs->regs[i + 2]);
>         }
>
>         dev_warn(rtk->dev, "\n");
> --
> 2.35.1
>
>


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] soc: apple: rtkit: Do not copy the reg state structure to the stack
  2023-02-13 17:15 ` Eric Curtin
@ 2023-02-13 19:22   ` Arnd Bergmann
  0 siblings, 0 replies; 3+ messages in thread
From: Arnd Bergmann @ 2023-02-13 19:22 UTC (permalink / raw)
  To: Eric Curtin, Asahi Lina
  Cc: Hector Martin, Sven Peter, Alyssa Rosenzweig, asahi,
	linux-arm-kernel, linux-kernel

On Mon, Feb 13, 2023, at 18:15, Eric Curtin wrote:
> On Sat, 11 Feb 2023 at 09:21, Asahi Lina <lina@asahilina.net> wrote:
>>
>> The register state struct is 848 bytes, which ends up bloating the
>> apple_rtkit_crashlog_dump_regs stack frame beyond 1024 on some
>> 32-bit platforms, triggering compile warnings.
>>
>> This doesn't matter for 64BIT/ARM64, but there's also no good reason to
>> copy the structure to the stack in this case. We can use __packed to
>> avoid alignment issues, there are no double-read hazards, and this is a
>> fatal error path so performance does not matter.
>>
>> Fixes: 22991d8d5725 ("soc: apple: rtkit: Add register dump decoding to crashlog")
>> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Reviewed-by: Eric Curtin <ecurtin@redhat.com>

Merged now, thanks a lot for the fix!

     Arnd

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-13 19:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-11  9:13 [PATCH] soc: apple: rtkit: Do not copy the reg state structure to the stack Asahi Lina
2023-02-13 17:15 ` Eric Curtin
2023-02-13 19:22   ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).