From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Ue7Vq-0007nf-C1 for mharc-grub-devel@gnu.org; Sun, 19 May 2013 13:35:14 -0400 Received: from eggs.gnu.org ([208.118.235.92]:60817) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ue7Vk-0007lj-Lh for grub-devel@gnu.org; Sun, 19 May 2013 13:35:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ue7Vh-0005df-Fa for grub-devel@gnu.org; Sun, 19 May 2013 13:35:08 -0400 Received: from mail-ea0-x22a.google.com ([2a00:1450:4013:c01::22a]:51123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ue7Vh-0005ag-75 for grub-devel@gnu.org; Sun, 19 May 2013 13:35:05 -0400 Received: by mail-ea0-f170.google.com with SMTP id f15so3612879eak.29 for ; Sun, 19 May 2013 10:35:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=cMWOBU8ib/hAO0xehTht7bbUoHTj5S3CfAnkRspyuZg=; b=ZHSDmrqHdQj61G1ZiahvUQ8h09Vsatd2Wo1J/UUbIMUzvVfH5rusWx/hb9zInHzTKS Zz96rmSXAAAcgdnIP2vTYnLJBzLwDF8avcGJ6rUCSM7OOPZ8qOofZJ50NinqDkxo5wqV pMnU0r2U6BI1IWKXerLGmAz6MDaHosEjGMb5zWZFBaAiKQOv8Hj0wtWwcEpKfvC4xzW0 ettei/AvQEA8/ceKFIICjQS7AEI11hJbDnTV6zyEf7Wxsh3UJeiJlRdfG9YUa5WW0pC3 dFxYCFcu7WmqdcctMjvJggr0FooFh507H0FcRTzsyMGbjgtiTSN3srJmA/UTRCgT25b1 DDvQ== X-Received: by 10.15.26.6 with SMTP id m6mr156925501eeu.4.1368984904083; Sun, 19 May 2013 10:35:04 -0700 (PDT) Received: from [192.168.56.2] (na-19-76-226.service.infuturo.it. [151.19.76.226]) by mx.google.com with ESMTPSA id w43sm32562409eeg.14.2013.05.19.10.34.58 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sun, 19 May 2013 10:35:03 -0700 (PDT) Message-ID: <51990D90.6010103@gmail.com> Date: Sun, 19 May 2013 19:36:16 +0200 From: Francesco Lavra User-Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20110929 Thunderbird/7.0.1 MIME-Version: 1.0 To: The development of GNU GRUB Subject: Re: [PATCH 5/7] add imported "FDT" module for flattened device tree operations References: <5158EDEC.1030606@gmail.com> <5159B905.9040601@gmail.com> <5196194E.4020600@gmail.com> In-Reply-To: <5196194E.4020600@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4013:c01::22a Cc: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 19 May 2013 17:35:12 -0000 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)