`AESND_Reset` hangs when called

[muff1n1634]

Was doing some testing with aesndlib a few days ago and found that calling AESND_Reset on exit just hangs my Wii. Wondered why, and looked into it a little bit.

Apparently, I'm not the first one to find out about this - it was known about when adding support for aesndlib in Dolphin in July of 2022, and even gives a warning about it not working with an explanation of why:

// The relevant code looks like this:
//
// lrs $acc1.m,@CMBL
// ...
// cmpi $acc1.m,#0xdead
// jeq task_terminate
//
// The cmpi instruction always sign-extends, so it will compare $acc1 with 0xff'dead'0000.
// However, recv_cmd runs in set16 mode, so the load to $acc1 will produce 0x00'dead'0000.
// This means that the comparison never succeeds, and no mail is sent in response to
// MAIL_TERMINATE. This means that __dsp_donecallback is never called (since that's
// normally called in response to DSP_DONE), so __aesnddspinit is never cleared, so
// AESND_Reset never returns, resulting in a hang. We always send the mail to avoid this
// hang. (It's possible to exit without calling AESND_Reset, so most homebrew probably
// isn't affected by this bug in the first place.)
//
// A fix exists, but has not yet been added to mainline libogc:
// https://github.com/extremscorner/libogc2/commit/38edc9db93232faa612f680c91be1eb4d95dd1c6
WARN_LOG_FMT(DSPHLE, "AESndUCode - MAIL_TERMINATE is broken in this version of the "
                     "uCode; this will hang on real hardware or with DSP LLE");

From the linked commit, it looks like the fix is small:

diff --git a/libaesnd/dspcode/dspmixer.s b/libaesnd/dspcode/dspmixer.s
index 6880225..9a66211 100644
--- a/libaesnd/dspcode/dspmixer.s
+++ b/libaesnd/dspcode/dspmixer.s
@@ -157,7 +157,8 @@ recv_cmd:
    cmpi    $acc1.m,#0x0100
    jeq     send_samples

-   cmpi    $acc1.m,#0xdead
+   lri     $acc0.m,#0xdead
+   cmp
    jeq     task_terminate

 wait_commands:

I write this as an issue instead of a pull request because this fix is already in a commit (albeit in another remote) and so should be able to be cherry-picked from there. (If doing so would complicate history or other things, though, making another PR would be trivial.)

[DacoTaco]

Hey muff1n1634,

thanks for pointing out the bug. we were not made aware this was in there. sadly there are many forks of libogc that have fixes like this, but never upstream them (due to not knowing/thinking about it, not caring or even not wanting to). we won't be cherry picking from said forks, as it would make the git history and even greater mess then it already is.

i will, however, look at fixing this. is there any code you can supply to test this with? i have no knowledge of how this all works haha

[muff1n1634]

basic example that just inits and deinits aesndlib

#include <stdio.h>

#include <gccore.h>
#include <wiiuse/wpad.h>
#include <aesndlib.h>

GXRModeObj * rmode;
void * xfb;
WPADData * wm0;
AESNDPB * voice;

int main(void)
{
    // standard setup stuff

    WPAD_Init();
    wm0 = WPAD_Data(WPAD_CHAN_0);

    VIDEO_Init();
    rmode = VIDEO_GetPreferredMode(NULL);
    xfb = MEM_K0_TO_K1(SYS_AllocateFramebuffer(rmode));

    VIDEO_Configure(rmode);
    VIDEO_SetNextFramebuffer(xfb);
    VIDEO_SetBlack(false);
    VIDEO_Flush();
    VIDEO_WaitVSync();
    if (rmode->viTVMode & VI_NON_INTERLACE)
        VIDEO_WaitVSync();
    CON_Init(xfb,
         20, 20,
         rmode->fbWidth, rmode->xfbHeight,
         rmode->fbWidth * VI_DISPLAY_PIX_SZ);

    printf("\x1b[2;0H");
    printf("I am a message\n");

    // AESND_Init()

    printf("calling AESND_Init()...\n");
    AESND_Init();
    printf("AESND_Init() called\n");

[[maybe_unused]] loop:
    while (true)
    {
        WPAD_ScanPads();
        wm0 = WPAD_Data(WPAD_CHAN_0);

        if (wm0->btns_d & WPAD_BUTTON_HOME)
        {
            printf("Exiting\n");
            goto exit;
        }

        VIDEO_WaitVSync();
    }

exit:
    // AESND_Reset()

    printf("AESND_Reset()...\n");
    AESND_Reset(); // execution hangs here with an unpatched aesndlib
    printf("aesndlib was reset\n"); // this statement is never reached in that case

    return 0;
}

[muff1n1634]

while adding a comment to explain the change, i did notice that some earlier lines did this version of the comparison (lri/cmp vs cmpi):

lri $acc0.m,#0xcdd1
cmp
jeq sys_commands

lri $acc0.m,#0xface
cmp
jne wait_commands

it seems the bug was avoided with these lines; perhaps the case with 0xdead was overlooked?

[DacoTaco]

it is possible. one off cases are often a case of being overlooked

[DacoTaco]

as mentioned, this is now fixed. thanks for reporting and pointing out the fix!