On Mon, 8 Sep 2014 22:18:56 +0100 Matt Fleming <matt@xxxxxxxxxxxxxxxxx> wrote: > On Sun, 07 Sep, at 02:11:33AM, Andre wrote: > > On Sat, 6 Sep 2014 23:02:51 +0200 > > Andre <andre.muller@xxxxxx> wrote: > > > With a Thinkpad T420 (pre-2.0 EFI), I do run into the failure condition for another reason: > > > > > > setup_efi_pci64 gets called, > > > the loop therein is executed nr_pci=11 (!?) times. > > > The second call run calls into __setup_efi_pci64 successfully, the memcpy takes place; the free_struct: path is not used. Back in setup_efi_pci64, the non-data path is walked. > > > > > > For all the other 10 runs of the loop, __setup_efi_pci64 fails at > > > if (!pci->romimage || !pci->romsize) > > > return EFI_INVALID_PARAMETER; > > > so that is the status here for any run but the second. > > Thanks for doing the analysis. > > > Following up with code... > > > > If it is OK to just leave the loop in setup_efi_pci64, > > this can be done simply by > > > > --- arch/x86/boot/compressed/eboot.c 2014-09-07 01:54:41.761008645 +0200 > > +++ arch/x86/boot/compressed/eboot_break.c 2014-09-07 01:52:06.321004951 +0200 > > @@ -504,6 +504,7 @@ > > params->hdr.setup_data = (unsigned long)rom; > > > > data = (struct setup_data *)rom; > > + break; > > > > } > > > > But I've got severe doubts about this. > > Right, we can't do this because we want to add all the PCI devices that > we can find with EFI, not just the first one. > Yes, I expected that to be by design, but I'm basically clueless as to the big picture. > > A solution limited to this particular failure mode > > could look like the below. It has all the charm of > > introducing a helper var. Hmpf. [Embarrassing Code] > > The code is generally fine as-is. Thanks, but no it's not, it's too ugly to live; a severe case of debugging stupor. That code should rather look like this: --- arch/x86/boot/compressed/eboot.c 2014-09-07 10:37:47.686001561 +0200 +++ arch/x86/boot/compressed/eboot.c 2014-09-07 11:31:38.233078339 +0200 @@ -474,6 +474,7 @@ unsigned long nr_pci; struct setup_data *data; int i; + int setup_pci_worked_once = 0; data = (struct setup_data *)(unsigned long)params->hdr.setup_data; @@ -498,6 +499,8 @@ if (status != EFI_SUCCESS) continue; + setup_pci_worked_once = 1; + if (data) data->next = (unsigned long)rom; else @@ -507,6 +510,8 @@ } + if (status == EFI_INVALID_PARAMETER && setup_pci_worked_once == 1) + status = EFI_SUCCESS; return status; } > Where I think we could improve things > is by adding efi_printk() message in certain error paths. Clearly, not > all error paths need such messages, e.g. the EFI_INVALID_PARAMETER path > you highlighted above, but it makes sense for memory allocation and PCI > read failures. > > Care to take a whack at that? I gave it a try, will post as a follow-up. The patch adds printks to non-success conditions for all pci.read instances and all allocate_pool instances within the scope of setup_efi_pci(). There are two calls left in the setup_graphics path, one for gop and one for uga. Does it make sense to add printks there as well? They won't display properly, I guess :-) Also, uga is called as a fallback when gop fails. So the uga failure would be the lost final straw to be noisy about? One more question: In the patch above, I added a helper to print a failure only if the __setup_efi_pci64() never succeeded. That's not really needed, is it? Because then, the check for setup_efi_pci success, the overly noisy bit of Ulf's patch, could go away again, and both your and my EFI_SUCCESS tweaks are not needed anymore. Thursday ff, I will be offline for a week. I'll happily follow up afterwards, but also feel free to run with the bits you see fit. Regards, Andre -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
