arm64: Fix the bug in fdt module

Message ID CADyBb7sJMH--hT=xifUvsRQ8U_8ipZ3N=y98Fh=Sw2hw7gj4BQ@mail.gmail.com
State New
Headers show

Commit Message

Fu Wei Nov. 5, 2015, 8:51 a.m.
Hi Vladimir

Please correct me :-)
Can I do this :


On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
> Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
> a écrit :
>>
>>
>> Le 4 nov. 2015 12:09 PM, "Fu Wei" <fu.wei@linaro.org> a écrit :
>> >
>> >  Hi Vladimir,
>> >
>> > On 4 November 2015 at 19:06, Fu Wei <fu.wei@linaro.org> wrote:
>> > > Hi Vladimir,
>> > >
>> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
>> > > <phcoder@gmail.com> wrote:
>> > >>
>> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" <fu.wei@linaro.org> a écrit :
>> > >>>
>> > >>> Hi Vladimir,
>> > >>>
>> > >>> Great thanks for your help :-)
>> > >>>
>> > >>>
>> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
>> > >>> <phcoder@gmail.com> wrote:
>> > >>> >
>> > >>> > Le 3 nov. 2015 9:56 AM, <fu.wei@linaro.org> a écrit :
>> > >>> >>
>> > >>> >> From: Fu Wei <fu.wei@linaro.org>
>> > >>> >>
>> > >>> >> This patch goes with commit:
>> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
>> > >>> >>  arm64: Move FDT functions to separate module
>> > >>> >>
>> > >>> >> linux and xen_boot modules can't work without this patch.
>> > >>> >>
>> > >>> >> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>> > >>> >> ---
>> > >>> >>  grub-core/Makefile.core.def  | 1 +
>> > >>> >>  grub-core/loader/arm64/fdt.c | 5 +++++
>> > >>> >>  2 files changed, 6 insertions(+)
>> > >>> >>
>> > >>> >> diff --git a/grub-core/Makefile.core.def
>> > >>> >> b/grub-core/Makefile.core.def
>> > >>> >> index 2ef10d1..3ea4e49 100644
>> > >>> >> --- a/grub-core/Makefile.core.def
>> > >>> >> +++ b/grub-core/Makefile.core.def
>> > >>> >> @@ -1674,6 +1674,7 @@ module = {
>> > >>> >>  module = {
>> > >>> >>    name = fdt;
>> > >>> >>    arm64 = loader/arm64/fdt.c;
>> > >>> >> +  fdt = lib/fdt.c;
>> > >>> >>    enable = arm64;
>> > >>> >>  };
>> > >>> >>
>> > >>> > Please don't add same file to 2 different modules. Remove it from
>> > >>> > Linux
>> > >>> > module
>> > >>>
>> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
>> > >>> So please allow me to separate all the fdt code from
>> > >>> loader/arm/linux.c; just like arm64.
>> > >>>
>> > >> I don't think it's necessary at this point. Is xen_boot going to be
>> > >> available for 32-bit arm as well?
>> >
>> > And I would like to help to sort out the code for your new fdt module
>> > , If you don't mind.
>> >
>> > But according to my tests, I think we need "fdt = lib/fdt.c;" in
>> > module. If we don't separate fdt code from arm, we also need "fdt =
>> > lib/fdt.c;" in linux module.
>> >
>> > Please correct me if I misunderstand something, thanks
>> >
>> We need arm = lib/fdt.c in linux if we don't restructure. There is no
>> reason to include lib/fdt.c in 2 different modules on arm64. Fdt in first
>> part just mean arm and arm64 in this context
>>
> As an alternative: enable fdt module on all fdt platforms (enable = fdt) and
> put lib/fdt.c
>
>> > >>> This patch may become a patchset :-)
>> > >>>
>> > >>> >> diff --git a/grub-core/loader/arm64/fdt.c
>> > >>> >> b/grub-core/loader/arm64/fdt.c
>> > >>> >> index 5202c14..d160ca0 100644
>> > >>> >> --- a/grub-core/loader/arm64/fdt.c
>> > >>> >> +++ b/grub-core/loader/arm64/fdt.c
>> > >>> >> @@ -25,6 +25,10 @@
>> > >>> >>  #include <grub/file.h>
>> > >>> >>  #include <grub/efi/efi.h>
>> > >>> >>
>> > >>> >> +GRUB_MOD_LICENSE ("GPLv3+");
>> > >>> >> +
>> > >>> >> +static grub_dl_t my_mod;
>> > >>> >> +
>> > >>> > What's the reason for my_mod?
>> > >>>
>> > >>> this is for grub_dl_unref and grub_dl_ref. but I forgot to check
>> > >>> this
>> > >>> again for this, sorry,
>> > >>> will add this later
>> > >>>
>> > >> Forget dl_ref and dl_unref. Linux module having a function reference
>> > >> is
>> > >> already good enough
>> > >>> >>  static void *loaded_fdt;
>> > >>> >>  static void *fdt;
>> > >>> >>
>> > >>> >> @@ -177,6 +181,7 @@ GRUB_MOD_INIT (fdt)
>> > >>> >>    cmd_devicetree =
>> > >>> >>      grub_register_command ("devicetree", grub_cmd_devicetree, 0,
>> > >>> >>                            N_("Load DTB file."));
>> > >>> >> +  my_mod = mod;
>> > >>> >>  }
>> > >>> >>
>> > >>> >>  GRUB_MOD_FINI (fdt)
>> > >>> >> --
>> > >>> >> 2.4.3
>> > >>> >>
>> > >>>
>> > >>>
>> > >>>
>> > >>> --
>> > >>> Best regards,
>> > >>>
>> > >>> Fu Wei
>> > >>> Software Engineer
>> > >>> Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> > >>> Ph: +86 21 61221326(direct)
>> > >>> Ph: +86 186 2020 4684 (mobile)
>> > >>> Room 1512, Regus One Corporate Avenue,Level 15,
>> > >>> One Corporate Avenue,222 Hubin Road,Huangpu District,
>> > >>> Shanghai,China 200021
>> > >
>> > >
>> > >
>> > > --
>> > > Best regards,
>> > >
>> > > Fu Wei
>> > > Software Engineer
>> > > Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> > > Ph: +86 21 61221326(direct)
>> > > Ph: +86 186 2020 4684 (mobile)
>> > > Room 1512, Regus One Corporate Avenue,Level 15,
>> > > One Corporate Avenue,222 Hubin Road,Huangpu District,
>> > > Shanghai,China 200021
>> >
>> >
>> >
>> > --
>> > Best regards,
>> >
>> > Fu Wei
>> > Software Engineer
>> > Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> > Ph: +86 21 61221326(direct)
>> > Ph: +86 186 2020 4684 (mobile)
>> > Room 1512, Regus One Corporate Avenue,Level 15,
>> > One Corporate Avenue,222 Hubin Road,Huangpu District,
>> > Shanghai,China 200021

Comments

Fu Wei Nov. 5, 2015, 9:48 a.m. | #1
Hi Vladimir

On 5 November 2015 at 17:45, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
> Le 5 nov. 2015 9:51 AM, "Fu Wei" <fu.wei@linaro.org> a écrit :
>>
>> Hi Vladimir
>>
>> Please correct me :-)
>> Can I do this :
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 2ef10d1..ea5831f 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1665,7 +1665,6 @@ module = {
>>    ia64_efi = loader/ia64/efi/linux.c;
>>    arm = loader/arm/linux.c;
>>    arm64 = loader/arm64/linux.c;
>> -  fdt = lib/fdt.c;
>>    common = loader/linux.c;
>>    common = lib/cmdline.c;
>>    enable = noemu;
>> @@ -1674,7 +1673,9 @@ module = {
>>  module = {
>>    name = fdt;
>>    arm64 = loader/arm64/fdt.c;
>> -  enable = arm64;
>> +  arm = loader/arm/fdt.c;
> Where does this file come from?
>

that is the file I am working on

i just tried to let you know my thought.

Is this way Ok for you? Please correct me if I misunderstand you


>
>> +  common = lib/fdt.c;
>> +  enable = fdt;
>>  };
>>
>>  module = {
>>
>> On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>> >
>> > Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko"
>> > <phcoder@gmail.com>
>> > a écrit :
>> >>
>> >>
>> >> Le 4 nov. 2015 12:09 PM, "Fu Wei" <fu.wei@linaro.org> a écrit :
>> >> >
>> >> >  Hi Vladimir,
>> >> >
>> >> > On 4 November 2015 at 19:06, Fu Wei <fu.wei@linaro.org> wrote:
>> >> > > Hi Vladimir,
>> >> > >
>> >> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
>> >> > > <phcoder@gmail.com> wrote:
>> >> > >>
>> >> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" <fu.wei@linaro.org> a écrit :
>> >> > >>>
>> >> > >>> Hi Vladimir,
>> >> > >>>
>> >> > >>> Great thanks for your help :-)
>> >> > >>>
>> >> > >>>
>> >> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
>> >> > >>> <phcoder@gmail.com> wrote:
>> >> > >>> >
>> >> > >>> > Le 3 nov. 2015 9:56 AM, <fu.wei@linaro.org> a écrit :
>> >> > >>> >>
>> >> > >>> >> From: Fu Wei <fu.wei@linaro.org>
>> >> > >>> >>
>> >> > >>> >> This patch goes with commit:
>> >> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
>> >> > >>> >>  arm64: Move FDT functions to separate module
>> >> > >>> >>
>> >> > >>> >> linux and xen_boot modules can't work without this patch.
>> >> > >>> >>
>> >> > >>> >> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>> >> > >>> >> ---
>> >> > >>> >>  grub-core/Makefile.core.def  | 1 +
>> >> > >>> >>  grub-core/loader/arm64/fdt.c | 5 +++++
>> >> > >>> >>  2 files changed, 6 insertions(+)
>> >> > >>> >>
>> >> > >>> >> diff --git a/grub-core/Makefile.core.def
>> >> > >>> >> b/grub-core/Makefile.core.def
>> >> > >>> >> index 2ef10d1..3ea4e49 100644
>> >> > >>> >> --- a/grub-core/Makefile.core.def
>> >> > >>> >> +++ b/grub-core/Makefile.core.def
>> >> > >>> >> @@ -1674,6 +1674,7 @@ module = {
>> >> > >>> >>  module = {
>> >> > >>> >>    name = fdt;
>> >> > >>> >>    arm64 = loader/arm64/fdt.c;
>> >> > >>> >> +  fdt = lib/fdt.c;
>> >> > >>> >>    enable = arm64;
>> >> > >>> >>  };
>> >> > >>> >>
>> >> > >>> > Please don't add same file to 2 different modules. Remove it
>> >> > >>> > from
>> >> > >>> > Linux
>> >> > >>> > module
>> >> > >>>
>> >> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
>> >> > >>> So please allow me to separate all the fdt code from
>> >> > >>> loader/arm/linux.c; just like arm64.
>> >> > >>>
>> >> > >> I don't think it's necessary at this point. Is xen_boot going to
>> >> > >> be
>> >> > >> available for 32-bit arm as well?
>> >> >
>> >> > And I would like to help to sort out the code for your new fdt module
>> >> > , If you don't mind.
>> >> >
>> >> > But according to my tests, I think we need "fdt = lib/fdt.c;" in
>> >> > module. If we don't separate fdt code from arm, we also need "fdt =
>> >> > lib/fdt.c;" in linux module.
>> >> >
>> >> > Please correct me if I misunderstand something, thanks
>> >> >
>> >> We need arm = lib/fdt.c in linux if we don't restructure. There is no
>> >> reason to include lib/fdt.c in 2 different modules on arm64. Fdt in
>> >> first
>> >> part just mean arm and arm64 in this context
>> >>
>> > As an alternative: enable fdt module on all fdt platforms (enable = fdt)
>> > and
>> > put lib/fdt.c
>> >
>> >> > >>> This patch may become a patchset :-)
>> >> > >>>
>> >> > >>> >> diff --git a/grub-core/loader/arm64/fdt.c
>> >> > >>> >> b/grub-core/loader/arm64/fdt.c
>> >> > >>> >> index 5202c14..d160ca0 100644
>> >> > >>> >> --- a/grub-core/loader/arm64/fdt.c
>> >> > >>> >> +++ b/grub-core/loader/arm64/fdt.c
>> >> > >>> >> @@ -25,6 +25,10 @@
>> >> > >>> >>  #include <grub/file.h>
>> >> > >>> >>  #include <grub/efi/efi.h>
>> >> > >>> >>
>> >> > >>> >> +GRUB_MOD_LICENSE ("GPLv3+");
>> >> > >>> >> +
>> >> > >>> >> +static grub_dl_t my_mod;
>> >> > >>> >> +
>> >> > >>> > What's the reason for my_mod?
>> >> > >>>
>> >> > >>> this is for grub_dl_unref and grub_dl_ref. but I forgot to check
>> >> > >>> this
>> >> > >>> again for this, sorry,
>> >> > >>> will add this later
>> >> > >>>
>> >> > >> Forget dl_ref and dl_unref. Linux module having a function
>> >> > >> reference
>> >> > >> is
>> >> > >> already good enough
>> >> > >>> >>  static void *loaded_fdt;
>> >> > >>> >>  static void *fdt;
>> >> > >>> >>
>> >> > >>> >> @@ -177,6 +181,7 @@ GRUB_MOD_INIT (fdt)
>> >> > >>> >>    cmd_devicetree =
>> >> > >>> >>      grub_register_command ("devicetree", grub_cmd_devicetree,
>> >> > >>> >> 0,
>> >> > >>> >>                            N_("Load DTB file."));
>> >> > >>> >> +  my_mod = mod;
>> >> > >>> >>  }
>> >> > >>> >>
>> >> > >>> >>  GRUB_MOD_FINI (fdt)
>> >> > >>> >> --
>> >> > >>> >> 2.4.3
>> >> > >>> >>
>> >> > >>>
>> >> > >>>
>> >> > >>>
>> >> > >>> --
>> >> > >>> Best regards,
>> >> > >>>
>> >> > >>> Fu Wei
>> >> > >>> Software Engineer
>> >> > >>> Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> >> > >>> Ph: +86 21 61221326(direct)
>> >> > >>> Ph: +86 186 2020 4684 (mobile)
>> >> > >>> Room 1512, Regus One Corporate Avenue,Level 15,
>> >> > >>> One Corporate Avenue,222 Hubin Road,Huangpu District,
>> >> > >>> Shanghai,China 200021
>> >> > >
>> >> > >
>> >> > >
>> >> > > --
>> >> > > Best regards,
>> >> > >
>> >> > > Fu Wei
>> >> > > Software Engineer
>> >> > > Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> >> > > Ph: +86 21 61221326(direct)
>> >> > > Ph: +86 186 2020 4684 (mobile)
>> >> > > Room 1512, Regus One Corporate Avenue,Level 15,
>> >> > > One Corporate Avenue,222 Hubin Road,Huangpu District,
>> >> > > Shanghai,China 200021
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Best regards,
>> >> >
>> >> > Fu Wei
>> >> > Software Engineer
>> >> > Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> >> > Ph: +86 21 61221326(direct)
>> >> > Ph: +86 186 2020 4684 (mobile)
>> >> > Room 1512, Regus One Corporate Avenue,Level 15,
>> >> > One Corporate Avenue,222 Hubin Road,Huangpu District,
>> >> > Shanghai,China 200021
>>
>>
>>
>> --
>> Best regards,
>>
>> Fu Wei
>> Software Engineer
>> Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> Ph: +86 21 61221326(direct)
>> Ph: +86 186 2020 4684 (mobile)
>> Room 1512, Regus One Corporate Avenue,Level 15,
>> One Corporate Avenue,222 Hubin Road,Huangpu District,
>> Shanghai,China 200021

Patch

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 2ef10d1..ea5831f 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1665,7 +1665,6 @@  module = {
   ia64_efi = loader/ia64/efi/linux.c;
   arm = loader/arm/linux.c;
   arm64 = loader/arm64/linux.c;
-  fdt = lib/fdt.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
   enable = noemu;
@@ -1674,7 +1673,9 @@  module = {
 module = {
   name = fdt;
   arm64 = loader/arm64/fdt.c;
-  enable = arm64;
+  arm = loader/arm/fdt.c;
+  common = lib/fdt.c;
+  enable = fdt;
 };

 module = {