[U-Boot] crash in usb_stor_get_info using pre-relocation address for ss->transport

[Chris Packham]

On Fri, Jun 14, 2013 at 10:48 AM, Chris Packham <judge.packham at gmail.com> wrote:
> On Thu, Jun 13, 2013 at 11:24 PM, Albert ARIBAUD
> <albert.u.boot at aribaud.net> wrote:
>> Hi Chris,
>>
>> On Thu, 13 Jun 2013 22:19:54 +1200, Chris Packham
>> <judge.packham at gmail.com> wrote:
>>
>>> Hi Albert,
>>>
>>> On 13/06/13 17:43, Albert ARIBAUD wrote:
>>> > Hi Chris,
>>> >
>>> > On Thu, 13 Jun 2013 13:16:17 +1200, Chris Packham
>>> > <judge.packham at gmail.com> wrote:
>>> >
>>> >> On Thu, Jun 13, 2013 at 12:02 PM, Chris Packham <judge.packham at gmail.com> wrote:
>>> >>> Hi,
>>> >>>
>>> >>> I've just found a crash in usb_stor_get_info (actually usb_inquiry
>>> >>> which gets auto-inlined). The cause seems to be that ss->transport is
>>> >>> set to the pre-relocation address of usb_stor_BBB_transport. Yet
>>> >>> ss->transport_reset is set to the correct relocated address of.
>>> >>>
>>> >>> The difference between the two is that usb_stor_BBB_reset is declared
>>> >>> static and usb_stor_BBB_transport is not. Changing
>>> >>> usb_stor_BBB_transport to a static makes things work but I notice that
>>> >>> none of the other transport functions are static either so I'm
>>> >>> thinking I haven't actually fixed the problem rather just masked it.
>>> >>
>>> >> Actually I see commit 199adb60 (common/misc: sparse fixes) does change
>>> >> the transport functions to static. Which is the change I was looking
>>> >> at. I still don't know if it is fixing a problem or masking a
>>> >> different one but this is probably why no-one else is complaining that
>>> >> their usb mass storage devices are causing crashes. I'll cherry-pick
>>> >> this to fix my problem.
>>> >>
>>> >>>
>>> >>> I did  some poking with a lauterbach and from the disassembly it looks
>>> >>> like there is a translation table being used when the function
>>> >>> pointers are setup by usb_storage_probe and when declared normally
>>> >>> usb_stor_BBB_transport ends up at the end. Everything else has the
>>> >>> correct relocated address so I wonder if there is an off-by-one error
>>> >>> in whatever creates that table.
>>> >
>>> > Can you elaborate? The only relocation-related table that I know of is
>>> > the one used in relocate_code(), and no other relocation-fix table
>>> > exists or is used anywhere else.
>>> >
>>> >>> Does this sound familiar to anyone.
>>> >
>>> > Familiar, no, but it does set in my mind, if not a blaring alarm with
>>> > flashing beacons, at least a blinking red light with a beep, so let's
>>> > analyize this.
>>> >
>>> > Amicalement,
>>> >
>>>
>>> I'm at home right now so I don't have the board in front of me. Here's
>>> some disassembly that gdb gives me
>>>
>>> int usb_stor_BBB_transport(); (without 199adb60)
>>>
>>> 1272            case US_PR_BULK:
>>> 1273                    USB_STOR_PRINTF("Bulk/Bulk/Bulk\n");
>>> 1274                    ss->transport = usb_stor_BBB_transport;
>>>    0xfffa9780 <+208>:   lwz     r0,-4(r30)
>>>    0xfffa9784 <+212>:   stw     r0,48(r31)
>>>
>>> 1275                    ss->transport_reset = usb_stor_BBB_reset;
>>>    0xfffa9788 <+216>:   lwz     r0,-4268(r30)
>>>    0xfffa978c <+220>:   b       0xfffa9770 <usb_storage_probe+192>
>>>
>>> 1276                    break;
>>>
>>> static int usb_stor_BBB_transport(); (with 199adb60)
>>>
>>> 1261            case US_PR_CB:
>>> 1262                    USB_STOR_PRINTF("Control/Bulk\n");
>>> 1263                    ss->transport = usb_stor_CB_transport;
>>>    0xfffa9608 <+180>:   lwz     r0,-4240(r30)
>>>    0xfffa960c <+184>:   stw     r0,48(r31)
>>>
>>> 1264                    ss->transport_reset = usb_stor_CB_reset;
>>>    0xfffa9610 <+188>:   lwz     r0,-4248(r30)
>>>    0xfffa9614 <+192>:   stw     r0,44(r31)
>>>
>>> 1265                    break;
>>>
>>> So r30 is the table thing I was talking about. I'm assuming it's
>>> something maintained by the compiler/linker. From memory -4(r30) was
>>> 0xfffaabcd everything else (including -4268(r30)) seemed to be the
>>> relocated address for various symbols, hence my comment about a possible
>>> off-by-one in whatever maintains that table.
>>>
>>> Because it's probably relevant here are my compiler details
>>>   $ powerpc-e500-linux-gnu-gcc --version
>>>   powerpc-e500-linux-gnu-gcc (Gentoo 4.6.3-r1 p1.9, pie-0.5.2) 4.6.3
>>>
>>> When I get back to work tomorrow I can post a dump of r30 from a running
>>> system.
>>
>> So that's PPC. Maybe PPC does manual fixing -- I was being ARM-centric
>> there.
>>
>> Amicalement,
>> --
>> Albert.
>
> Another relevant detail that I left out is that I'm booting via the
> CPC (from SPI or NOR). So the pre-relocation address is definitely
> invalid for me, but it might be valid for others (problems with
> re-flashing the board aside).
>
> Here's some info I was able to extract using the lauterbach.
>
> R(R30) = 7FF9AE00
> R(R30) - 0x4    7FF9ADFC = FFFA94A8
> R(R30) - 0x10AC 7FF99D54 = 7FF586E8
> .u-boot\usb_storage\usb_stor_BBB_reset
> R(R30) - 0x8    7FF9ADF8 = 7FFB017C  .u-boot\Global\NetArpWaitTimerStart
>
>
> And fom u-boot.map
>  .got           0x00000000fffe8024     0x2de8 arch/powerpc/cpu/mpc85xx/start.o
>                 0x00000000fffeae00                _GLOBAL_OFFSET_TABLE_
>                 0x00000000fffeae10                PROVIDE
> (_GLOBAL_OFFSET_TABLE_, (. + 0x4))
>                 0x00000000fffeae0c                _FIXUP_TABLE_ = .
>
> so 7FF9AE00 is the relocated address of _GLOBAL_OFFSET_TABLE_

More digging.

Albert, this is all looking PowerPC specific so I'm not sure how much
you care. I'll dump what I've found here in this thread in case
someone else wants to look at it in the future. For now 199adb60 fixes
my problem.

The following code snippet from arch/powerpc/cpu/mpc85xx/start.S
handles the relocation for the global offset table.

in_ram:

    /*
     * Relocation Function, r12 point to got2+0x8000
     *
     * Adjust got2 pointers, no need to check for 0, this code
     * already puts a few entries in the table.
     */
    li    r0,__got2_entries at sectoff@l
    la    r3,GOT(_GOT2_TABLE_)
    lwz    r11,GOT(_GOT2_TABLE_)
    mtctr    r0
    sub    r11,r3,r11
    addi    r3,r3,-4
1:    lwzu    r0,4(r3)
    cmpwi    r0,0
    beq-    2f
    add    r0,r0,r11
    stw    r0,0(r3)
2:    bdnz    1b

Walking through this code I can see all the entries get changed to
their relocated addresses, except for the last one which happens to be
usb_stor_BBB_transport. So as I suspected there is probably an off by
one error in the code snippet above, nothing leaps out at me as
obviously wrong but my PowerPC assembly is a bit rusty.