[U-Boot] [PATCH v2] watchdog: Move watchdog_dev to data section (BSS may not be cleared)

[Stefan Roese]

On 07.04.19 20:16, Daniel Schwierzeck wrote:
> 
> 
> Am 03.04.19 um 09:12 schrieb Stefan Roese:
>> This patch moves all instances of static "watchdog_dev" declarations to
>> the "data" section. This may be needed, as the BSS may not be cleared
>> in the early U-Boot phase, where watchdog_reset() is already beeing
>> called. This may result in incorrect pointer access, as the check to
>> "!watchdog_dev" in watchdog_reset() may not be true and the function
>> may continue to run.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Heiko Schocher <hs at denx.de>
>> Cc: Tom Rini <trini at konsulko.com>
>> Cc: Michal Simek <michal.simek at xilinx.com>
>> Cc: "Marek Behún" <marek.behun at nic.cz>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> ---
>> v2:
>> - Remove now superfluous explicit clearing of watchdog_dev in Xilinx
>>    zynq and zynqmp board_early_init_f() as suggested by Michal
>>
>>   arch/mips/mach-mt7620/cpu.c                          | 2 +-
>>   board/CZ.NIC/turris_mox/turris_mox.c                 | 2 +-
>>   board/CZ.NIC/turris_omnia/turris_omnia.c             | 2 +-
>>   board/xilinx/microblaze-generic/microblaze-generic.c | 2 +-
>>   board/xilinx/zynq/board.c                            | 7 +------
>>   board/xilinx/zynqmp/zynqmp.c                         | 7 +------
>>   6 files changed, 6 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/mips/mach-mt7620/cpu.c b/arch/mips/mach-mt7620/cpu.c
>> index 9e0ca716f7..fe74f26a54 100644
>> --- a/arch/mips/mach-mt7620/cpu.c
>> +++ b/arch/mips/mach-mt7620/cpu.c
>> @@ -70,7 +70,7 @@ int print_cpuinfo(void)
>>   }
>>   
>>   #ifdef CONFIG_WATCHDOG
>> -static struct udevice *watchdog_dev;
>> +static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
> 
> to simplify that line you could use __section(".data")

Yes, thanks for the suggestions. But as you might have noticed, this patch
is only meant as a temporary fix for this upcoming release. This code will
get removed with the generic watchdog implementation that I'm currently
working on. So to not disturb this release, I would prefer to use this
patch as-is.

Thanks,
Stefan

>>   
>>   /* Called by macro WATCHDOG_RESET */
>>   void watchdog_reset(void)
>> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
>> index 65d50a92dd..96cb9c7e5c 100644
>> --- a/board/CZ.NIC/turris_mox/turris_mox.c
>> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
>> @@ -120,7 +120,7 @@ int board_fix_fdt(void *blob)
>>   #endif
>>   
>>   #ifdef CONFIG_WDT_ARMADA_37XX
>> -static struct udevice *watchdog_dev;
>> +static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
>>   
>>   void watchdog_reset(void)
>>   {
>> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
>> index 1f7650cb36..c7f6479a0c 100644
>> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
>> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
>> @@ -365,7 +365,7 @@ static bool disable_mcu_watchdog(void)
>>   #endif
>>   
>>   #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION)
>> -static struct udevice *watchdog_dev = NULL;
>> +static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
>>   #endif
>>   
>>   int board_init(void)
>> diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c
>> index c946ec3208..28c9efa3a2 100644
>> --- a/board/xilinx/microblaze-generic/microblaze-generic.c
>> +++ b/board/xilinx/microblaze-generic/microblaze-generic.c
>> @@ -25,7 +25,7 @@
>>   DECLARE_GLOBAL_DATA_PTR;
>>   
>>   #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
>> -static struct udevice *watchdog_dev;
>> +static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
>>   #endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */
>>   
>>   ulong ram_base;
>> diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
>> index b4b84df576..ea26aad16f 100644
>> --- a/board/xilinx/zynq/board.c
>> +++ b/board/xilinx/zynq/board.c
>> @@ -19,17 +19,12 @@
>>   DECLARE_GLOBAL_DATA_PTR;
>>   
>>   #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
>> -static struct udevice *watchdog_dev;
>> +static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
>>   #endif
>>   
>>   #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_BOARD_EARLY_INIT_F)
>>   int board_early_init_f(void)
>>   {
>> -# if defined(CONFIG_WDT)
>> -	/* bss is not cleared at time when watchdog_reset() is called */
>> -	watchdog_dev = NULL;
>> -# endif
>> -
>>   	return 0;
>>   }
>>   #endif
>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
>> index 5e1d2116bc..db27247850 100644
>> --- a/board/xilinx/zynqmp/zynqmp.c
>> +++ b/board/xilinx/zynqmp/zynqmp.c
>> @@ -25,7 +25,7 @@
>>   DECLARE_GLOBAL_DATA_PTR;
>>   
>>   #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT)
>> -static struct udevice *watchdog_dev;
>> +static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL;
>>   #endif
>>   
>>   #if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ZYNQMPPL) && \
>> @@ -322,11 +322,6 @@ int board_early_init_f(void)
>>   	ret = psu_init();
>>   #endif
>>   
>> -#if defined(CONFIG_WDT) && !defined(CONFIG_SPL_BUILD)
>> -	/* bss is not cleared at time when watchdog_reset() is called */
>> -	watchdog_dev = NULL;
>> -#endif
>> -
>>   	return ret;
>>   }
>>   
>>
> 

Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de