On 10/17/2014 07:00 PM, Leif Lindholm wrote:

Same comment as previous patch - not big enough to keep separate. Other comments inline.

Yes, got you

On Wed, Oct 15, 2014 at 10:49:05PM +0800, Fu Wei wrote:

...

arm64: export some accessor functions of dtb and the "loaded" flag for sharing code to multiboot.c

Signed-off-by: Fu Wei fu.wei@linaro.org

grub-core/loader/arm64/linux.c | 19 +++++++++++++++++++ include/grub/arm64/linux.h | 7 +++++++ 2 files changed, 26 insertions(+)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c index 5050a82..44f6e3d 100644 --- a/grub-core/loader/arm64/linux.c +++ b/grub-core/loader/arm64/linux.c @@ -50,6 +50,25 @@ static grub_addr_t initrd_end; static void *loaded_fdt; static void *fdt; +/* The accessor functions for dtb and the "loaded" flag */

Do we really need accessors for these? You could just rename and export the variables themselves.

In the V2 patch I have sent you at 8th April, I did something like this:

...

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c index 65129c2..dccd104 100644 --- a/grub-core/loader/arm64/linux.c +++ b/grub-core/loader/arm64/linux.c @@ -37,10 +37,11 @@ GRUB_MOD_LICENSE ("GPLv3+"); #define BYTES_TO_PAGES(bytes) (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT) #define GRUB_EFI_PE_MAGIC 0x5A4D -static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;

-static grub_dl_t my_mod; -static int loaded; +grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID; +grub_dl_t my_mod; +int loaded; +void *fdt; +void get_fdt (void); static void *kernel_addr; static grub_uint64_t kernel_size; @@ -52,7 +53,6 @@ static grub_addr_t initrd_start; static grub_addr_t initrd_end; static void *loaded_fdt; -static void *fdt; static void * get_firmware_fdt (void) @@ -75,7 +75,7 @@ get_firmware_fdt (void) return firmware_fdt; } -static void +void get_fdt (void) { void *raw_fdt; @@ -464,8 +464,12 @@ fail: return grub_errno; }

Your comment is : " I would much prefer to keep these variables static (where possible), and add accessor functions to get to them. The GRUB coding style otherwise mandates that they are given global namespace (i.e., they would need to be called grub_arm64_...). "

So I added these three accessor functions to access "loaded" and "get_fdt"

Did I misunderstand your suggestion??

...

+static void get_fdt (void);

+int grub_arm64_linux_get_loaded (void) +{

• return (loaded);

+}

+void grub_arm64_linux_set_loaded (int loaded_flag) +{

• loaded = loaded_flag;

+}

+void *grub_arm64_linux_get_fdt (void) +{

• get_fdt ();
• return (fdt);

+}

static void * get_firmware_fdt (void) { diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h index 22ffbaa..a438840 100644 --- a/include/grub/arm64/linux.h +++ b/include/grub/arm64/linux.h @@ -42,4 +42,11 @@ struct grub_arm64_linux_kernel_header grub_uint32_t hdr_offset; /* Offset of PE/COFF header */ }; +/* Export the accessor functions for dtb and the "loaded" flag */ +void EXPORT_FUNC (*grub_arm64_linux_get_fdt) (void);

+int EXPORT_FUNC (grub_arm64_linux_get_loaded) (void);

+void EXPORT_FUNC (grub_arm64_linux_set_loaded) (int loaded_flag);

#endif /* ! GRUB_LINUX_CPU_HEADER */

Are these EXPORT statements really necessary? This is all part of the same module.

Yes, you are right, we don't need it. ACTION : delete "EXPORT_FUNC ()" for these three accessor functions

/ Leif