Add GDB support #5411

lyakh · 2025-05-09T15:24:38Z

Taking over nklayman@930457f . Respective Zephyr and SOF PRs to follow next week.

To match thesofproject/linux#5411 this commit updates the gdb ring buffer format as follows: 1. Larger ring space: there can be gdb commands longer than 128 bytes 2. Use 32-bit head and tail pointers 3. Use the reserved space in slot 0 of the debug window, since there are no free slots on Tiger Lake 4. Add a check to make sure our ring buffer isn't colliding with debug window slot descriptors Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lgirdwood

Stopped reviewing as it looks like some new files can be squashed.

sound/soc/sof/sof-client-fw-gdb.c

lgirdwood · 2025-05-26T16:20:14Z

sound/soc/sof/sof-client-fw-gdb.c

If we are debugging do we really want to support suspend/resume. If we are debugging a SOF module we should force device into D0.

@lgirdwood This is fixed in the fourth patch. I tried to keep the first commit as close to the original as possible, at least functionally. But I think I'll address at least all the comment style and lack of comment comments in it directly, instead of adding follow-up patches

lgirdwood · 2025-05-26T16:20:46Z

sound/soc/sof/sof-client-fw-gdb.c

unlikely is overkill here, not a high frequency call.

lgirdwood · 2025-05-26T16:20:56Z

sound/soc/sof/sof-client-fw-gdb.c

lgirdwood · 2025-05-26T16:21:35Z

sound/soc/sof/sof-client-fw-gdb.c

@ranj063 don we have some API or core context that has this ?

lgirdwood · 2025-05-26T16:25:32Z

sound/soc/sof/sof-client-fw-gdb.c

can be added by ipc3 users.

lgirdwood · 2025-05-26T16:26:37Z

sound/soc/sof/sof-client-fw-gdb.c

lets disable runtime PM

lgirdwood · 2025-05-26T16:27:04Z

sound/soc/sof/sof-client-fw-gdb.c

dual license with BSD.

lgirdwood · 2025-05-26T16:30:07Z

sound/soc/sof/Kconfig

In general I think this file needs some refinement around the tunnel protocol handling.

@lgirdwood sorry, which tunnel protocol do you mean?

I mean the copying/handling of GDB data to/from IPC layer, but I think you've already done this in the later patches in this PR.

@lgirdwood gdb communication isn't done via IPCs, it's done via a memory window. Only the first "switch to gdb" command is an IPC, and that isn't a part of the actual gdb messaging yet.

ok, but then how would stop/break work if there is no IRQ ?

@lgirdwood that's what the added polling periodic delayed work does in 551692f#diff-fdf6d136ee82a240596bbe084669793c8f1ca4499e85a85e92dba039fd81461aR310 . And it isn't only needed for breaks, it's needed for any replies to gdb commands

lgirdwood · 2025-05-26T16:30:49Z

sound/soc/sof/sof-client-fw-gdb.c

looks like this can be squashed.

@lgirdwood You mean as

struct sof_ipc_gdb_message { union { struct sof_ipc_cmd_hdr hdr; struct sof_ipc4_msg msg; }; char cmd[]; } __packed;

? Yeah, I'll double check if this works and if yes, will use it, thanks

sorry, I meant you are adding a new file to the kernel in a series of patches. You should just do it a 1 patch for the new file.

@lgirdwood well, I tried to keep the first patch as close to the author's original as possible. But if we decide that that isn't needed, sure, can squash

sound/soc/sof/sof-client-fw-gdb.c

ujfalusi · 2025-06-04T08:56:42Z

sound/soc/sof/sof-client-fw-gdb.c

we usually don't do this in kernel C code...

ujfalusi · 2025-06-04T08:57:27Z

sound/soc/sof/sof-client-fw-gdb.c

you can drop () from the prints, it is not aligning with the style used in kernel and SOF.

ujfalusi · 2025-06-04T08:58:08Z

sound/soc/sof/sof-client-fw-gdb.c

what does 'debug send' means?

To match thesofproject/linux#5411 this commit updates the gdb ring buffer format as follows: 1. Larger ring space: there can be gdb commands longer than 128 bytes 2. Use 32-bit head and tail pointers 3. Use the reserved space in slot 0 of the debug window, since there are no free slots on Tiger Lake 4. Add a check to make sure our ring buffer isn't colliding with debug window slot descriptors Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ujfalusi

There are two big issues I can see:

client drivers are restricted from using sdev directly.
the debug slot assumption is dangerous, what happens if you don't even have GDB support in fw and thus no GDB slot?

ujfalusi · 2025-06-06T07:46:29Z

sound/soc/sof/sof-client-fw-gdb.c

nitpick: you can align the start of the lines here

I've looked through a couple of SOF files and haven't found any similar broken lines, so I'd rather just leave it at my editor's choice :-)

Right, because they are implemented this way always:

msg->primary = SOF_IPC4_MSG_TYPE_SET(SOF_IPC4_MOD_LARGE_CONFIG_SET); msg->primary |= SOF_IPC4_MSG_DIR(SOF_IPC4_MSG_REQUEST); msg->primary |= SOF_IPC4_MSG_TARGET(SOF_IPC4_MODULE_MSG);

@ujfalusi exactly, which I also dislike :-) To me it looks like

a = 2; a = a + 3;

instead of writing

a = 2 + 3;

but let's say it's subjective :-)

I cannot say why it is like that, but clearly it is followed throughout sof kernel code...

I cannot say why it is like that, but clearly it is followed throughout sof kernel code...

@ujfalusi the entire kernel? Not only SOF / ASoC? I'm sure the kernel has enough examples of both, but counting them wouldn't be very simple...

I cannot say why it is like that, but clearly it is followed throughout sof kernel code...

@ujfalusi the entire kernel? Not only SOF / ASoC? I'm sure the kernel has enough examples of both, but counting them wouldn't be very simple...

I cannot say why it is like that, but clearly it is followed throughout sof kernel code...

sound/soc/sof/sof-client-fw-gdb.c

ujfalusi · 2025-06-06T07:58:48Z

sound/soc/sof/sof-client-fw-gdb.c

extra line is not needed.

ujfalusi · 2025-06-06T08:01:51Z

sound/soc/sof/sof-client-fw-gdb.c

sof-clients are restricted to use sdev directly.

I think I will rework the sof-client.h to make this more evident (header for client driver use only and rest which is needed by the core).

sof-clients are restricted to use sdev directly.

@ujfalusi do you mean clients should only call accessor functions to access well defined sdev functionality? For GDB we need this:

int bar = snd_sof_dsp_get_bar_index(sdev, SOF_FW_BLK_TYPE_SRAM); void __iomem *ring_buffer = sdev->bar[bar] + sdev->debug_box.offset + SOF_GDB_WINDOW_OFFSET + offset;

Can we get this via those accessors? But the client API has sof_client_dev_to_sof_dev() - should it not be available at all?

...or do you mean that clients just shouldn't save the pointer locally and should always use sof_client_dev_to_sof_dev() instead and save the client pointer?

What I mean is that clients must not have any knowledge of sdev, in your case you should ask for a debug slot by index as you have re-purposing the descriptor slot with second functionality.
We support maximum of 15 slots in the debug windows:

------------------------ | Page0 - descriptors | ------------------------ | Page1 - slot0 | ------------------------ | Page2 - slot1 | ------------------------ | ... | ------------------------ | Page14 - slot13 | ------------------------ | Page15 - slot14 | ------------------------

we have maximum of 14 descriptors for the slots, but there is no descriptor for self, so you cannot say that page0 is descriptor and GDB, which is a bummer. The slots themself does not store their magic ID either.

But what if we introduce a descriptor15 which is a special one describing what is using the page0's free part?

You could then ask for the slot by ID 0x42444700 and get the offset, but hrm, the size then would not be less that the page size... But you can define that the GDB slot size is always this not full slot, even if it is occupying a dedicated one.

...or do you mean that clients just shouldn't save the pointer locally and should always use sof_client_dev_to_sof_dev() instead and save the client pointer?

Client drivers in their code must not use anything sdev, they must use wrappers or APIs and yes, I will try to hide this from the client drivers somehow. I have it in the header so we can have light inline code as API, but it is prone to abuse.

@lyakh, another possibility is to have a way to request a slot by page index, but I think the issue is: how can you know that the GDB is using that slot and not another, is the GDB enabled in the first place? You need some way to tell that you can go ahead and use the are and not just assume that it will be always there

@ujfalusi yes, we just need to pick up something... @kv2019i @lgirdwood what do you think?

sound/soc/sof/sof-client-fw-gdb.c

ujfalusi · 2025-06-06T08:08:18Z

sound/soc/sof/sof-client-fw-gdb.c

while sdev is not allowed to be used by clients directly, you have saved this to priv->sdev in sof_fw_gdb_dfs_open()

AFAIU clients do use sdev, it does seem to be allowed and a part of the SOF client API. You seem to just prefer not to let clients access internal structure of sdev, which I'm doing in the updated version. I could of course save cdev instead of sdev in private data, but I anyway need sdev in the end.

ujfalusi · 2025-06-06T08:20:08Z

sound/soc/sof/sof-client-fw-gdb.c

The GDB is part of the debug window? How does it co-exists with other debug window users? How the slot is allocated? Is it true that this magical offset is always GDB slot and never can be anything else?

We have this for IPC4:

include/sound/sof/ipc4/header.h:#define SOF_IPC4_DEBUG_SLOT_GDB_STUB 0x42444700

and we have (in core) this to look up a slot: sof_ipc4_find_debug_slot_offset_by_type(), you would need a client wrapper for this and use it to make sure that the assumption is firing back.

@ujfalusi the problem is that we don't have many free debug slots, in fact we have none. Debug slots are page-size parts of the second memory window. The first page (slot 0) ATM is used for slot descriptors... Whereas those descriptors use a few hundreds of bytes - less than 1kB. Slot 1 is used for logging. On TGL that's it - window 2 is only 8kB (2 page) large. On other platforms like MTL we have generous 12kB there, i.e. 3 pages / 3 slots. Slot 2 is reserved for the shell. So, we can either say, that shell and GDB are mutually exclusive, but that still wouldn't solve the problem for TGL. And on PTL it's 2 pages again.
So, what I've done - I grabbed the remaining 3kB of slot 0. Nobody needs them, nobody uses them, but we need to guard that well...

ujfalusi · 2025-06-06T08:25:41Z

sound/soc/sof/sof-client-fw-gdb.c

is this right?
this is the size of the union sof_ipc_gdb_message struct. You are never sending/receiving additonal data on top of a headers?

@ujfalusi ATM we only have 1 IPC - switch to GDB stub. And it doesn't have any data. So in principle we could remove those data pointers. If we need data in the future - we can add those and it won't break the ABI, because there's anyway data size included with IPCs, right?

not quite, in ipc4 the data size is what you have placing into the mailbox, probably you want to use sof_client_ipc_tx_message_no_reply() here and have not reply specified at all.

not quite, in ipc4 the data size is what you have placing into the mailbox, probably you want to use sof_client_ipc_tx_message_no_reply() here and have not reply specified at all.

that's much better, thanks!

To match thesofproject/linux#5411 this commit updates the gdb ring buffer format as follows: 1. Larger ring space: there can be gdb commands longer than 128 bytes 2. Use 32-bit head and tail pointers 3. Use the reserved space in slot 0 of the debug window, since there are no free slots on Tiger Lake 4. Add a check to make sure our ring buffer isn't colliding with debug window slot descriptors Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ujfalusi

@lyakh, I think the code looks mostly good, but I still think that we should look up the GDB slot by type, see my comments.

ujfalusi · 2025-06-17T06:41:31Z

sound/soc/sof/ipc4.c

Keep this list ordered by index, so the GDB goes to the end of the list, after SOF_IPC4_GLB_NOTIFICATION

ujfalusi · 2025-06-17T07:11:07Z

sound/soc/sof/ipc4.c

How do we know that GDM is in slot0?

I still think it would be better to look up the offset by type for GDM as well.
descriptor 15 (a new one since atm we have 14 of them) would be used to describe the remaining of slot0:

desc0 -> slot1 desc1 -> slot2 .. desc13 -> slot14 desc14 -> slot15 desc15 -> slot0+1024

in sof_ipc4_find_debug_slot_offset_by_type() after the for loop, check if the desc15 has the match

sof_mailbox_read(sdev, slot_desc_type_offset, &type, sizeof(type)); if (type == slot_type) return sdev->debug_box.offset + 1024; dev_dbg(sdev->dev, "Slot type %#x is not available in debug window\n", slot_type); return 0;

And in GDB client driver:

offset = sof_client_ipc4_debug_slot_offset_by_type(cdev, SOF_IPC4_DEBUG_SLOT_GDB_STUB); size = SOF_IPC4_DEBUG_SLOT_SIZE - (offset & (SOF_IPC4_DEBUG_SLOT_SIZE - 1));

this would make things forward compatible in regards where GDB is and we will also be able to detect that it is available at all via the debug window.

ujfalusi · 2025-06-17T07:14:49Z

sound/soc/sof/sof-client.c

See my previous comment on using the lookup by type.

ujfalusi · 2025-06-17T07:21:55Z

sound/soc/sof/sof-client-fw-gdb.c

These are 0 after the allocation and are not changed during runtime, no need to clear them?

ujfalusi · 2025-06-17T07:23:16Z

sound/soc/sof/sof-client-fw-gdb.c

offset == 0 is an error.

sound/soc/sof/sof-client-fw-gdb.c

Add an IPC4 message type to start a firmware GDB debugging session. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ujfalusi

@lyakh, I think this looks great now, just few things I would like to ask to be changed and improved.

ujfalusi · 2025-06-18T08:06:21Z

include/sound/sof/ipc4/header.h

These were aligned with the debug slot type defines and while we are here I think we should correct the definitions and terms:

/* * debug info window is organized in 16 (equal sized) pages: * * ------------------------ * | Page0 - descriptors | * ------------------------ * | Page1 - slot0 | * ------------------------ * | Page2 - slot1 | * ------------------------ * | ... | * ------------------------ * | Page14 - slot13 | * ------------------------ * | Page15 - slot14 | * ------------------------ * * The slot size == page size */ #define SOF_IPC4_DEBUG_PAGE_SIZE 0x1000 #define SOF_IPC4_DEBUG_SLOT_SIZE SOF_IPC4_DEBUG_PAGE_SIZE #define SOF_IPC4_DEBUG_PAGE0_SLOT_OFFSET 1024 /* usable slots 0..14 (page 1..15) */ #define SOF_IPC4_MAX_DEBUG_SLOTS 15 /* * Descriptor 0-14: slot 0-14 (page 1-15) * Descriptor 15: partial slot at page 0 + 1024 */ #define SOF_IPC4_MAX_DEBUG_DESCS (SOF_IPC4_MAX_DEBUG_SLOTS + 1) #define SOF_IPC4_DEBUG_DESCRIPTOR_SIZE 12 /* 3 x u32 */ /* debug log slot types */

sound/soc/sof/ipc4.c

ujfalusi · 2025-06-18T08:22:55Z

sound/soc/sof/sof-client-fw-gdb.c

this define is no longer needed

ujfalusi

@lyakh , looks all good except the obfuscation of the SOF_IPC4_DEBUG_PAGE_SIZE value, with that reverted to 0x1000 I'm ready to approve this PR.

ujfalusi · 2025-06-18T11:51:35Z

include/sound/sof/ipc4/header.h

what was wrong with 0x1000, which is btw used also in firmware.

@ujfalusi having 0x1000 used in one location and 1024 in another was weird. So I decided that (4 * 1024) is easier than 0x400

I prefer 0x1000 for the PAGE and SLOT size and I'm indifferent on the offset representation within PAGE0

sound/soc/sof/ipc4.c

The debug memory window is split in 4KiB large pages. Page zero is used for slot descriptors, which leaves one or two more free slots for actual useful work. One of them is usually used for mtrace logging, which leaves no free slots on some systems and only one such slot on others. This commit repurposes the free space in page zero after slot descriptors to be used as slot number 16, one higher than the currently highest possible slot number. To improve documentation we move the tabular representation of the window layout from ipc4-mtrace.c to header.h. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Add wrappers for mailbox reading and writing and for debug slot offset extraction. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Adds a DebugFS file to communicate with Zephyr's GDB stub. Communication is via ringbuffers in shared SRAM. Both IPC3 and IPC4 are supported. Signed-off-by: Noah Klayman <noah.klayman@intel.com> Co-developed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2025-06-24T07:17:37Z

3 latest minor changes:

https://github.com/thesofproject/linux/compare/38e32d0487092a0ea58674fe57731e7d36ab9ba6..3b29d400135085399712be77424fbb803b2f9ab1 - changed back to 0x1000
https://github.com/thesofproject/linux/compare/3b29d400135085399712be77424fbb803b2f9ab1..865fdf77e5e95af18e5f36dd6fc8667efbd767d5 - moved sof_client_ipc4_find_debug_slot_offset_by_type() under #if IPC4 to fix IPC3 builds
https://github.com/thesofproject/linux/compare/3b29d400135085399712be77424fbb803b2f9ab1..865fdf77e5e95af18e5f36dd6fc8667efbd767d5 - added checks for sof_fw_gdb_get_tx_ptrs() return code to make sure not to use head and tail if it fails

bardliao · 2025-06-25T10:23:25Z

@lyakh Could you take a look at the spare check issue below?

CHECK   sound/soc/sof/sof-client-fw-gdb.c
sound/soc/sof/sof-client-fw-gdb.c:365:22: error: invalid assignment: |=
sound/soc/sof/sof-client-fw-gdb.c:365:22:    left side has type restricted __poll_t
sound/soc/sof/sof-client-fw-gdb.c:365:22:    right side has type int
sound/soc/sof/sof-client-fw-gdb.c:369:22: error: invalid assignment: |=
sound/soc/sof/sof-client-fw-gdb.c:369:22:    left side has type restricted __poll_t
sound/soc/sof/sof-client-fw-gdb.c:369:22:    right side has type int
make[5]: *** [scripts/Makefile.build:288: sound/soc/sof/sof-client-fw-gdb.o] Error 1

lyakh · 2025-06-25T10:45:33Z

@lyakh Could you take a look at the spare check issue below?

CHECK   sound/soc/sof/sof-client-fw-gdb.c
sound/soc/sof/sof-client-fw-gdb.c:365:22: error: invalid assignment: |=
sound/soc/sof/sof-client-fw-gdb.c:365:22:    left side has type restricted __poll_t
sound/soc/sof/sof-client-fw-gdb.c:365:22:    right side has type int
sound/soc/sof/sof-client-fw-gdb.c:369:22: error: invalid assignment: |=
sound/soc/sof/sof-client-fw-gdb.c:369:22:    left side has type restricted __poll_t
sound/soc/sof/sof-client-fw-gdb.c:369:22:    right side has type int
make[5]: *** [scripts/Makefile.build:288: sound/soc/sof/sof-client-fw-gdb.o] Error 1

@bardliao that's the code commonly used throughout the kernel, e.g.

linux/drivers/gpio/gpiolib-cdev.c

Line 1500 in 33e8c45

events = EPOLLIN | EPOLLRDNORM;

- I bet that line would cause a sparse warning too

This was referenced May 12, 2025

GDB: add support for Intel ADSP platforms zephyrproject-rtos/zephyr#89810

Merged

GDB stub support thesofproject/sof#10000

Merged

lyakh marked this pull request as ready for review May 16, 2025 06:35

lyakh requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart, ranj063 and ujfalusi as code owners May 16, 2025 06:35

lgirdwood reviewed May 26, 2025

View reviewed changes

ujfalusi reviewed Jun 4, 2025

View reviewed changes

sound/soc/sof/sof-client-fw-gdb.c Outdated Show resolved Hide resolved

ujfalusi reviewed Jun 4, 2025

View reviewed changes

sound/soc/sof/sof-client-fw-gdb.c Outdated

Copy link

Collaborator

ujfalusi Jun 4, 2025

Choose a reason for hiding this comment

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

we usually don't do this in kernel C code...

ujfalusi reviewed Jun 4, 2025

View reviewed changes

lyakh force-pushed the gdb branch 2 times, most recently from 1edfb4b to 167c481 Compare June 4, 2025 10:20

lyakh force-pushed the gdb branch 2 times, most recently from 33d4049 to 2f273b3 Compare June 4, 2025 15:06

ujfalusi requested changes Jun 6, 2025

View reviewed changes

lyakh force-pushed the gdb branch 2 times, most recently from 8d8af14 to beb38f8 Compare June 11, 2025 12:35

lyakh force-pushed the gdb branch 3 times, most recently from a9cd7c4 to 265c18f Compare June 13, 2025 14:47

ujfalusi reviewed Jun 17, 2025

View reviewed changes

ASoC: SOF: ipc4: add a command to start GDB

79a2197

Add an IPC4 message type to start a firmware GDB debugging session. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the gdb branch from 265c18f to 3214a0c Compare June 17, 2025 14:38

ujfalusi reviewed Jun 18, 2025

View reviewed changes

lyakh force-pushed the gdb branch from 3214a0c to 38e32d0 Compare June 18, 2025 11:18

ujfalusi reviewed Jun 18, 2025

View reviewed changes

lyakh force-pushed the gdb branch from 38e32d0 to 3b29d40 Compare June 18, 2025 12:39

ujfalusi previously approved these changes Jun 18, 2025

View reviewed changes

ASoC: SOF: client: add wrappers for memory window access

ec811d4

Add wrappers for mailbox reading and writing and for debug slot offset extraction. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh dismissed ujfalusi’s stale review via 865fdf7 June 23, 2025 09:54

lyakh force-pushed the gdb branch from 3b29d40 to 865fdf7 Compare June 23, 2025 09:54

lgirdwood previously approved these changes Jun 23, 2025

View reviewed changes

lyakh dismissed lgirdwood’s stale review via f4ba24f June 23, 2025 12:23

lyakh force-pushed the gdb branch from 865fdf7 to f4ba24f Compare June 23, 2025 12:23

ujfalusi approved these changes Jun 24, 2025

View reviewed changes

lgirdwood approved these changes Jun 24, 2025

View reviewed changes

bardliao merged commit 7a79ba9 into thesofproject:topic/sof-dev Jun 25, 2025
9 of 16 checks passed

lyakh deleted the gdb branch June 25, 2025 13:08

Add GDB support #5411

Add GDB support #5411

Uh oh!

Conversation

lyakh commented May 9, 2025

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!