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

[Chris Packham]

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.