| Message ID | ff1ba206-a40a-9c6b-9240-a546c3fdbea7@ti.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] Problem with initialize in mmc_initialize in mmc.c | expand |
On Monday 30 October 2017 07:37 PM, Faiz Abbas wrote: > Hi, > > The variable *initialized* in mmc_initialize() is declared as static and > initialised to 0 in the following commit. This makes the compiler put it > in the .bss section of the image. > > commit 1b26bab12e85e8b0d382d6775e40d14445249574 > Author: Daniel Kochmański <dkochmanski@turtle-solutions.eu> > Date: Fri May 29 16:55:43 2015 +0200 > > mmc: Protect `mmc_initialize` from initialising mmc multiple times > > `mmc_initialize` might be called multiple times leading to the > mmc-controllers being initialised twice, and initialising the > `mmc_devices` list head twice which may lead to memory leaks. > > Signed-off-by: Daniel Kochmański <dkochmanski@turtle-solutions.eu> > CC: Roy Spliet <r.spliet@ultimaker.com> > Cc: Ian Campbell <ijc@hellion.org.uk> > CC: Pantelis Antoniou <panto@antoniou-consulting.com> > Acked-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index da47037..f12546a 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -1762,6 +1762,11 @@ static void do_preinit(void) > > int mmc_initialize(bd_t *bis) > { > + static int initialized = 0; > + if (initialized) /* Avoid initializing mmc multiple times */ > + return 0; > + initialized = 1; > + > INIT_LIST_HEAD (&mmc_devices); > cur_dev_num = 0; > > > .bss should not be accessed in u-boot before relocation because it > overlaps with fdt and writing to variables in .bss can corrupt the fdt. > MMC can be probed before relocation if it contains the u-boot > environment. Therefore, I tried to move this variable to the .data > section by > > static int initialized __attribute__((section(".data"))); > > When *initialized* was a part of .bss it was getting re-initilized to 0 > as a part of relocation. Therefore, mmc was getting probed again > successfully after relocation with new addresses for mmc devices. > > Now when *initialized* is not a part of .bss, it holds its value across > relocation and a second call to mmc_initialize() returns without doing > anything. However, the *mmc_devices* list containing pointers to the > older locations of mmc devices is not refreshed and thus mmc devices > fail to enumerate. > > So *initialized* is a problem whether it is in .data or .bss. > I am not sure how to fix this. Any help is appreciated. I have been told that all pointers are supposed to be updated during relocation. Can anyone point to relevant code or example of the same? Thanks, Faiz
Hi Faiz, On 11/02/2017 11:23 PM, Faiz Abbas wrote: > > > On Monday 30 October 2017 07:37 PM, Faiz Abbas wrote: >> Hi, >> >> The variable *initialized* in mmc_initialize() is declared as static and >> initialised to 0 in the following commit. This makes the compiler put it >> in the .bss section of the image. >> >> commit 1b26bab12e85e8b0d382d6775e40d14445249574 >> Author: Daniel Kochmański <dkochmanski@turtle-solutions.eu> >> Date: Fri May 29 16:55:43 2015 +0200 >> >> mmc: Protect `mmc_initialize` from initialising mmc multiple times >> >> `mmc_initialize` might be called multiple times leading to the >> mmc-controllers being initialised twice, and initialising the >> `mmc_devices` list head twice which may lead to memory leaks. >> >> Signed-off-by: Daniel Kochmański <dkochmanski@turtle-solutions.eu> >> CC: Roy Spliet <r.spliet@ultimaker.com> >> Cc: Ian Campbell <ijc@hellion.org.uk> >> CC: Pantelis Antoniou <panto@antoniou-consulting.com> >> Acked-by: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >> index da47037..f12546a 100644 >> --- a/drivers/mmc/mmc.c >> +++ b/drivers/mmc/mmc.c >> @@ -1762,6 +1762,11 @@ static void do_preinit(void) >> >> int mmc_initialize(bd_t *bis) >> { >> + static int initialized = 0; >> + if (initialized) /* Avoid initializing mmc multiple times */ >> + return 0; >> + initialized = 1; >> + >> INIT_LIST_HEAD (&mmc_devices); >> cur_dev_num = 0; >> >> >> .bss should not be accessed in u-boot before relocation because it >> overlaps with fdt and writing to variables in .bss can corrupt the fdt. >> MMC can be probed before relocation if it contains the u-boot >> environment. Therefore, I tried to move this variable to the .data >> section by Will check with T32..but i didn't have seen about similar case.. You means it should be the multiple initializing? Best Regards, Jaehoon Chung >> >> static int initialized __attribute__((section(".data"))); >> >> When *initialized* was a part of .bss it was getting re-initilized to 0 >> as a part of relocation. Therefore, mmc was getting probed again >> successfully after relocation with new addresses for mmc devices. >> >> Now when *initialized* is not a part of .bss, it holds its value across >> relocation and a second call to mmc_initialize() returns without doing >> anything. However, the *mmc_devices* list containing pointers to the >> older locations of mmc devices is not refreshed and thus mmc devices >> fail to enumerate. >> >> So *initialized* is a problem whether it is in .data or .bss. >> I am not sure how to fix this. Any help is appreciated. > > I have been told that all pointers are supposed to be updated during > relocation. Can anyone point to relevant code or example of the same? > > Thanks, > Faiz > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot >
Hi, On 11/27/2017 05:16 PM, Jaehoon Chung wrote: > Hi Faiz, Remove the some mail account, because of private problem. > > On 11/02/2017 11:23 PM, Faiz Abbas wrote: >> >> >> On Monday 30 October 2017 07:37 PM, Faiz Abbas wrote: >>> Hi, >>> >>> The variable *initialized* in mmc_initialize() is declared as static and >>> initialised to 0 in the following commit. This makes the compiler put it >>> in the .bss section of the image. >>> >>> commit 1b26bab12e85e8b0d382d6775e40d14445249574 >>> Author: Daniel Kochmański <dkochmanski@turtle-solutions.eu> >>> Date: Fri May 29 16:55:43 2015 +0200 >>> >>> mmc: Protect `mmc_initialize` from initialising mmc multiple times >>> >>> `mmc_initialize` might be called multiple times leading to the >>> mmc-controllers being initialised twice, and initialising the >>> `mmc_devices` list head twice which may lead to memory leaks. >>> >>> Signed-off-by: Daniel Kochmański <dkochmanski@turtle-solutions.eu> >>> CC: Roy Spliet <r.spliet@ultimaker.com> >>> Cc: Ian Campbell <ijc@hellion.org.uk> >>> CC: Pantelis Antoniou <panto@antoniou-consulting.com> >>> Acked-by: Hans de Goede <hdegoede@redhat.com> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> >>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >>> index da47037..f12546a 100644 >>> --- a/drivers/mmc/mmc.c >>> +++ b/drivers/mmc/mmc.c >>> @@ -1762,6 +1762,11 @@ static void do_preinit(void) >>> >>> int mmc_initialize(bd_t *bis) >>> { >>> + static int initialized = 0; >>> + if (initialized) /* Avoid initializing mmc multiple times */ >>> + return 0; >>> + initialized = 1; >>> + >>> INIT_LIST_HEAD (&mmc_devices); >>> cur_dev_num = 0; >>> >>> >>> .bss should not be accessed in u-boot before relocation because it >>> overlaps with fdt and writing to variables in .bss can corrupt the fdt. >>> MMC can be probed before relocation if it contains the u-boot >>> environment. Therefore, I tried to move this variable to the .data >>> section by > > Will check with T32..but i didn't have seen about similar case.. > You means it should be the multiple initializing? > > Best Regards, > Jaehoon Chung > >>> >>> static int initialized __attribute__((section(".data"))); >>> >>> When *initialized* was a part of .bss it was getting re-initilized to 0 >>> as a part of relocation. Therefore, mmc was getting probed again >>> successfully after relocation with new addresses for mmc devices. >>> >>> Now when *initialized* is not a part of .bss, it holds its value across >>> relocation and a second call to mmc_initialize() returns without doing >>> anything. However, the *mmc_devices* list containing pointers to the >>> older locations of mmc devices is not refreshed and thus mmc devices >>> fail to enumerate. >>> >>> So *initialized* is a problem whether it is in .data or .bss. >>> I am not sure how to fix this. Any help is appreciated. >> >> I have been told that all pointers are supposed to be updated during >> relocation. Can anyone point to relevant code or example of the same? >> >> Thanks, >> Faiz >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> https://lists.denx.de/listinfo/u-boot >> >
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index da47037..f12546a 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -1762,6 +1762,11 @@ static void do_preinit(void) int mmc_initialize(bd_t *bis) { + static int initialized = 0; + if (initialized) /* Avoid initializing mmc multiple times */ + return 0; + initialized = 1; + INIT_LIST_HEAD (&mmc_devices); cur_dev_num = 0;
Hi, The variable *initialized* in mmc_initialize() is declared as static and initialised to 0 in the following commit. This makes the compiler put it in the .bss section of the image. commit 1b26bab12e85e8b0d382d6775e40d14445249574 Author: Daniel Kochmański <dkochmanski@turtle-solutions.eu> Date: Fri May 29 16:55:43 2015 +0200 mmc: Protect `mmc_initialize` from initialising mmc multiple times `mmc_initialize` might be called multiple times leading to the mmc-controllers being initialised twice, and initialising the `mmc_devices` list head twice which may lead to memory leaks. Signed-off-by: Daniel Kochmański <dkochmanski@turtle-solutions.eu> CC: Roy Spliet <r.spliet@ultimaker.com> Cc: Ian Campbell <ijc@hellion.org.uk> CC: Pantelis Antoniou <panto@antoniou-consulting.com> Acked-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> .bss should not be accessed in u-boot before relocation because it overlaps with fdt and writing to variables in .bss can corrupt the fdt. MMC can be probed before relocation if it contains the u-boot environment. Therefore, I tried to move this variable to the .data section by static int initialized __attribute__((section(".data"))); When *initialized* was a part of .bss it was getting re-initilized to 0 as a part of relocation. Therefore, mmc was getting probed again successfully after relocation with new addresses for mmc devices. Now when *initialized* is not a part of .bss, it holds its value across relocation and a second call to mmc_initialize() returns without doing anything. However, the *mmc_devices* list containing pointers to the older locations of mmc devices is not refreshed and thus mmc devices fail to enumerate. So *initialized* is a problem whether it is in .data or .bss. I am not sure how to fix this. Any help is appreciated. Thanks, Faiz