Re: [PATCH 5/7] add imported "FDT" module for flattened device tree operations

[Francesco Lavra <francescolavra.fl@gmail.com>]

On 05/17/2013 01:49 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> I applied this to ARM branch after fixing several grave problems
> including attempts to free statically allocated memory. The files
> originally didn't even compile.

You seem to have missed the follow-up e-mail I sent on April 7th (see 
http://lists.gnu.org/archive/html/grub-devel/2013-04/msg00086.html), 
where I sent an updated version of the fdt driver; in fact, my first 
version of fdt.c had several issues which have been addressed in the 
second version. So please replace the current fdt.c file in the arm 
branch with the updated file.

In the mail I mentioned above, I also acknowledged my mistake in using 
grub_free() to free memory allocated by 
grub_efi_allocate_loader_memory(). But the memory used for the Linux 
kernel, the initrd and the fdt is statically allocated only in the 
u-boot case, which as I explained when I first posted my code wasn't 
supported by my code. In the EFI case the memory is not statically 
allocated and IMHO should be freed when not needed anymore. I also 
proposed to add a function grub_efi_free_memory() to 
kern/arm/efi/misc.c and to use that function to free the memory. If you 
want to add this stuff, just say so and I will happily send a patch.

Regarding the current Linux loader file, I noticed a few issues:
- The defines added at the beginning of the file are not necessary in 
the current code, because they are already in include/grub/arm/linux.h. 
When I sent my loader code, I added those defines to the .c file 
because with support for EFI only it seemed overkill to have a new 
header file.
- In linux_unload(), the initrd data is discarded: why? This doesn't 
seem right, and looking for example at the i386 linux loader, I see 
no such thing there either.
- In grub_cmd_initrd() there is a leftover call to grub_free() on 
initrd_start, which should be removed (and IMHO replaced in the EFI 
case by the right function call to free the memory).
- In grub_cmd_devicetree(), if the supplied file name doesn't 
correspond to an existing file, grub_file_close() is called with a NULL 
argument, which results in a fatal crash.

The patch below addresses these issues.
Thanks,
Francesco

=== modified file 'grub-core/loader/arm/linux.c'
--- grub-core/loader/arm/linux.c	2013-05-17 11:45:22 +0000
+++ grub-core/loader/arm/linux.c	2013-05-19 17:00:13 +0000
@@ -43,15 +43,6 @@
 static grub_uint32_t machine_type;
 static void *fdt_addr;
 
-#define LINUX_ZIMAGE_OFFSET	0x24
-#define LINUX_ZIMAGE_MAGIC	0x016f2818
-
-#define ARM_FDT_MACHINE_TYPE 0xFFFFFFFF
-
-#define LINUX_PHYS_OFFSET        (0x00008000)
-#define LINUX_INITRD_PHYS_OFFSET (LINUX_PHYS_OFFSET + 0x02000000)
-#define LINUX_FDT_PHYS_OFFSET    (LINUX_INITRD_PHYS_OFFSET - 0x10000)
-
 /*
  * linux_prepare_fdt():
  *   Prepares a loaded FDT for being passed to Linux.
@@ -212,8 +203,6 @@
   grub_free (linux_args);
   linux_args = NULL;
 
-  initrd_start = initrd_end = 0;
-
   return GRUB_ERR_NONE;
 }
 
@@ -278,8 +267,6 @@
   if (size == 0)
     goto fail;
 
-  if (initrd_start)
-    grub_free ((void *) initrd_start);
 #ifdef GRUB_MACHINE_EFI
   initrd_start = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_INITRD_PHYS_OFFSET, size);
 
@@ -337,7 +324,7 @@
 
   dtb = grub_file_open (argv[0]);
   if (!dtb)
-    goto out;
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file"));
 
   size = grub_file_size (dtb);
   if (size == 0)