[U-Boot] [PATCH] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[U-Boot] [PATCH] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[Lukasz Majewski]

Up till this commit passing NULL as input parameter was allowed, but not
handled properly.

When one passed NULL to one of this function parameters, the code was
executed causing data abort.

However, what is more interesting, the abort was not caught because of code
execution in HYP mode with masked CPSR A bit ("Imprecise Data Abort mask bit).
The TI's AM57xx SoC switch to HYP mode with A bit masked in lowlevel_init.S
due to SMC call. Such operation (by default) is performed in SoC ROM code.

The problem would pop up when one:
- Switch back to SVC mode after disabling LPAE support
- Somebody enables A bit (by executing cpsie a asm instruction)

and then the previously described exception would be caught.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 arch/arm/cpu/armv7/omap-common/clocks-common.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/cpu/armv7/omap-common/clocks-common.c b/arch/arm/cpu/armv7/omap-common/clocks-common.c
index 097b8e3..157155a 100644
--- a/arch/arm/cpu/armv7/omap-common/clocks-common.c
+++ b/arch/arm/cpu/armv7/omap-common/clocks-common.c
@@ -822,27 +822,29 @@ void do_enable_clocks(u32 const *clk_domains,
 	u32 i, max = 100;
 
 	/* Put the clock domains in SW_WKUP mode */
-	for (i = 0; (i < max) && clk_domains[i]; i++) {
+	for (i = 0; (i < max) && clk_domains && clk_domains[i]; i++) {
 		enable_clock_domain(clk_domains[i],
 				    CD_CLKCTRL_CLKTRCTRL_SW_WKUP);
 	}
 
 	/* Clock modules that need to be put in HW_AUTO */
-	for (i = 0; (i < max) && clk_modules_hw_auto[i]; i++) {
+	for (i = 0; (i < max) && clk_modules_hw_auto &&
+		     clk_modules_hw_auto[i]; i++) {
 		enable_clock_module(clk_modules_hw_auto[i],
 				    MODULE_CLKCTRL_MODULEMODE_HW_AUTO,
 				    wait_for_enable);
 	};
 
 	/* Clock modules that need to be put in SW_EXPLICIT_EN mode */
-	for (i = 0; (i < max) && clk_modules_explicit_en[i]; i++) {
+	for (i = 0; (i < max) && clk_modules_explicit_en &&
+		     clk_modules_explicit_en[i]; i++) {
 		enable_clock_module(clk_modules_explicit_en[i],
 				    MODULE_CLKCTRL_MODULEMODE_SW_EXPLICIT_EN,
 				    wait_for_enable);
 	};
 
 	/* Put the clock domains in HW_AUTO mode now */
-	for (i = 0; (i < max) && clk_domains[i]; i++) {
+	for (i = 0; (i < max) && clk_domains && clk_domains[i]; i++) {
 		enable_clock_domain(clk_domains[i],
 				    CD_CLKCTRL_CLKTRCTRL_HW_AUTO);
 	}
-- 
2.1.4

[U-Boot] [PATCH] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[Lokesh Vutla]

On 3/24/2017 3:54 AM, Lukasz Majewski wrote:
> Up till this commit passing NULL as input parameter was allowed, but not
> handled properly.
> 
> When one passed NULL to one of this function parameters, the code was
> executed causing data abort.
> 
> However, what is more interesting, the abort was not caught because of code
> execution in HYP mode with masked CPSR A bit ("Imprecise Data Abort mask bit).
> The TI's AM57xx SoC switch to HYP mode with A bit masked in lowlevel_init.S
> due to SMC call. Such operation (by default) is performed in SoC ROM code.
> 
> The problem would pop up when one:
> - Switch back to SVC mode after disabling LPAE support
> - Somebody enables A bit (by executing cpsie a asm instruction)
> 
> and then the previously described exception would be caught.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  arch/arm/cpu/armv7/omap-common/clocks-common.c | 10 ++++++----

This has been moved to arch/arm/mach-omap2/clocks-common.c
Please use the latest U-Boot.

>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/omap-common/clocks-common.c b/arch/arm/cpu/armv7/omap-common/clocks-common.c
> index 097b8e3..157155a 100644
> --- a/arch/arm/cpu/armv7/omap-common/clocks-common.c
> +++ b/arch/arm/cpu/armv7/omap-common/clocks-common.c
> @@ -822,27 +822,29 @@ void do_enable_clocks(u32 const *clk_domains,
>  	u32 i, max = 100;
>  
>  	/* Put the clock domains in SW_WKUP mode */
> -	for (i = 0; (i < max) && clk_domains[i]; i++) {
> +	for (i = 0; (i < max) && clk_domains && clk_domains[i]; i++) {

Instead of checking for clk_domains every time, can we use max as
ARRAY_SIZE(clk_domains)? Similarly other places.

Thanks and regards,
Lokesh

>  		enable_clock_domain(clk_domains[i],
>  				    CD_CLKCTRL_CLKTRCTRL_SW_WKUP);
>  	}
>  
>  	/* Clock modules that need to be put in HW_AUTO */
> -	for (i = 0; (i < max) && clk_modules_hw_auto[i]; i++) {
> +	for (i = 0; (i < max) && clk_modules_hw_auto &&
> +		     clk_modules_hw_auto[i]; i++) {
>  		enable_clock_module(clk_modules_hw_auto[i],
>  				    MODULE_CLKCTRL_MODULEMODE_HW_AUTO,
>  				    wait_for_enable);
>  	};
>  
>  	/* Clock modules that need to be put in SW_EXPLICIT_EN mode */
> -	for (i = 0; (i < max) && clk_modules_explicit_en[i]; i++) {
> +	for (i = 0; (i < max) && clk_modules_explicit_en &&
> +		     clk_modules_explicit_en[i]; i++) {
>  		enable_clock_module(clk_modules_explicit_en[i],
>  				    MODULE_CLKCTRL_MODULEMODE_SW_EXPLICIT_EN,
>  				    wait_for_enable);
>  	};
>  
>  	/* Put the clock domains in HW_AUTO mode now */
> -	for (i = 0; (i < max) && clk_domains[i]; i++) {
> +	for (i = 0; (i < max) && clk_domains && clk_domains[i]; i++) {
>  		enable_clock_domain(clk_domains[i],
>  				    CD_CLKCTRL_CLKTRCTRL_HW_AUTO);
>  	}
> 

[U-Boot] [PATCH] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[Lukasz Majewski]

Hi Lokesh,

> 
> 
> On 3/24/2017 3:54 AM, Lukasz Majewski wrote:
> > Up till this commit passing NULL as input parameter was allowed,
> > but not handled properly.
> > 
> > When one passed NULL to one of this function parameters, the code
> > was executed causing data abort.
> > 
> > However, what is more interesting, the abort was not caught because
> > of code execution in HYP mode with masked CPSR A bit ("Imprecise
> > Data Abort mask bit). The TI's AM57xx SoC switch to HYP mode with A
> > bit masked in lowlevel_init.S due to SMC call. Such operation (by
> > default) is performed in SoC ROM code.
> > 
> > The problem would pop up when one:
> > - Switch back to SVC mode after disabling LPAE support
> > - Somebody enables A bit (by executing cpsie a asm instruction)
> > 
> > and then the previously described exception would be caught.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  arch/arm/cpu/armv7/omap-common/clocks-common.c | 10 ++++++----
> 
> This has been moved to arch/arm/mach-omap2/clocks-common.c
> Please use the latest U-Boot.

Ok. I'm not working on a cutting-edge u-boot.

> 
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/cpu/armv7/omap-common/clocks-common.c
> > b/arch/arm/cpu/armv7/omap-common/clocks-common.c index
> > 097b8e3..157155a 100644 ---
> > a/arch/arm/cpu/armv7/omap-common/clocks-common.c +++
> > b/arch/arm/cpu/armv7/omap-common/clocks-common.c @@ -822,27 +822,29
> > @@ void do_enable_clocks(u32 const *clk_domains, u32 i, max = 100;
> >  
> >  	/* Put the clock domains in SW_WKUP mode */
> > -	for (i = 0; (i < max) && clk_domains[i]; i++) {
> > +	for (i = 0; (i < max) && clk_domains && clk_domains[i];
> > i++) {
> 
> Instead of checking for clk_domains every time, can we use max as
> ARRAY_SIZE(clk_domains)?

do_enable_clocks() accepts pointer to u32 as an argument
(clk_domains). IMHO the ARRAY_SIZE(clk_domains) would be 1 - always.

> Similarly other places.
> 
> Thanks and regards,
> Lokesh
> 
> >  		enable_clock_domain(clk_domains[i],
> >  				    CD_CLKCTRL_CLKTRCTRL_SW_WKUP);
> >  	}
> >  
> >  	/* Clock modules that need to be put in HW_AUTO */
> > -	for (i = 0; (i < max) && clk_modules_hw_auto[i]; i++) {
> > +	for (i = 0; (i < max) && clk_modules_hw_auto &&
> > +		     clk_modules_hw_auto[i]; i++) {
> >  		enable_clock_module(clk_modules_hw_auto[i],
> >  				    MODULE_CLKCTRL_MODULEMODE_HW_AUTO,
> >  				    wait_for_enable);
> >  	};
> >  
> >  	/* Clock modules that need to be put in SW_EXPLICIT_EN
> > mode */
> > -	for (i = 0; (i < max) && clk_modules_explicit_en[i]; i++) {
> > +	for (i = 0; (i < max) && clk_modules_explicit_en &&
> > +		     clk_modules_explicit_en[i]; i++) {
> >  		enable_clock_module(clk_modules_explicit_en[i],
> >  				    MODULE_CLKCTRL_MODULEMODE_SW_EXPLICIT_EN,
> >  				    wait_for_enable);
> >  	};
> >  
> >  	/* Put the clock domains in HW_AUTO mode now */
> > -	for (i = 0; (i < max) && clk_domains[i]; i++) {
> > +	for (i = 0; (i < max) && clk_domains && clk_domains[i];
> > i++) { enable_clock_domain(clk_domains[i],
> >  				    CD_CLKCTRL_CLKTRCTRL_HW_AUTO);
> >  	}
> > 




Best regards,

Lukasz Majewski

--

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

[U-Boot] [PATCH] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[Lokesh Vutla]

On Friday 24 March 2017 02:41 PM, Lukasz Majewski wrote:
> Hi Lokesh,
> 
>>
>>
>> On 3/24/2017 3:54 AM, Lukasz Majewski wrote:
>>> Up till this commit passing NULL as input parameter was allowed,
>>> but not handled properly.
>>>
>>> When one passed NULL to one of this function parameters, the code
>>> was executed causing data abort.
>>>
>>> However, what is more interesting, the abort was not caught because
>>> of code execution in HYP mode with masked CPSR A bit ("Imprecise
>>> Data Abort mask bit). The TI's AM57xx SoC switch to HYP mode with A
>>> bit masked in lowlevel_init.S due to SMC call. Such operation (by
>>> default) is performed in SoC ROM code.
>>>
>>> The problem would pop up when one:
>>> - Switch back to SVC mode after disabling LPAE support
>>> - Somebody enables A bit (by executing cpsie a asm instruction)
>>>
>>> and then the previously described exception would be caught.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>> ---
>>>  arch/arm/cpu/armv7/omap-common/clocks-common.c | 10 ++++++----
>>
>> This has been moved to arch/arm/mach-omap2/clocks-common.c
>> Please use the latest U-Boot.
> 
> Ok. I'm not working on a cutting-edge u-boot.
> 
>>
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/omap-common/clocks-common.c
>>> b/arch/arm/cpu/armv7/omap-common/clocks-common.c index
>>> 097b8e3..157155a 100644 ---
>>> a/arch/arm/cpu/armv7/omap-common/clocks-common.c +++
>>> b/arch/arm/cpu/armv7/omap-common/clocks-common.c @@ -822,27 +822,29
>>> @@ void do_enable_clocks(u32 const *clk_domains, u32 i, max = 100;
>>>  
>>>  	/* Put the clock domains in SW_WKUP mode */
>>> -	for (i = 0; (i < max) && clk_domains[i]; i++) {
>>> +	for (i = 0; (i < max) && clk_domains && clk_domains[i];
>>> i++) {
>>
>> Instead of checking for clk_domains every time, can we use max as
>> ARRAY_SIZE(clk_domains)?
> 
> do_enable_clocks() accepts pointer to u32 as an argument
> (clk_domains). IMHO the ARRAY_SIZE(clk_domains) would be 1 - always.
> 

you are right. Discard my comment.

Thanks and regards,
Lokesh

[U-Boot] [PATCH v2] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[Lukasz Majewski]

Up till this commit passing NULL as input parameter was allowed, but not
handled properly.

When one passed NULL to one of this function parameters, the code was
executed causing data abort.

However, what is more interesting, the abort was not caught because of code
execution in HYP mode with masked CPSR A bit ("Imprecise Data Abort mask bit).
The TI's AM57xx SoC switch to HYP mode with A bit masked in lowlevel_init.S
due to SMC call. Such operation (by default) is performed in SoC ROM code.

The problem would pop up when one:
- Switch back to SVC mode after disabling LPAE support
- Somebody enables A bit (by executing cpsie a asm instruction)

and then the previously described exception would be caught.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Rebase to the newest "master" branch
---
 arch/arm/mach-omap2/clocks-common.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/clocks-common.c b/arch/arm/mach-omap2/clocks-common.c
index 84f93e7..93c4c6f 100644
--- a/arch/arm/mach-omap2/clocks-common.c
+++ b/arch/arm/mach-omap2/clocks-common.c
@@ -828,27 +828,29 @@ void do_enable_clocks(u32 const *clk_domains,
 	u32 i, max = 100;
 
 	/* Put the clock domains in SW_WKUP mode */
-	for (i = 0; (i < max) && clk_domains[i]; i++) {
+	for (i = 0; (i < max) && clk_domains && clk_domains[i]; i++) {
 		enable_clock_domain(clk_domains[i],
 				    CD_CLKCTRL_CLKTRCTRL_SW_WKUP);
 	}
 
 	/* Clock modules that need to be put in HW_AUTO */
-	for (i = 0; (i < max) && clk_modules_hw_auto[i]; i++) {
+	for (i = 0; (i < max) && clk_modules_hw_auto &&
+		     clk_modules_hw_auto[i]; i++) {
 		enable_clock_module(clk_modules_hw_auto[i],
 				    MODULE_CLKCTRL_MODULEMODE_HW_AUTO,
 				    wait_for_enable);
 	};
 
 	/* Clock modules that need to be put in SW_EXPLICIT_EN mode */
-	for (i = 0; (i < max) && clk_modules_explicit_en[i]; i++) {
+	for (i = 0; (i < max) && clk_modules_explicit_en &&
+		     clk_modules_explicit_en[i]; i++) {
 		enable_clock_module(clk_modules_explicit_en[i],
 				    MODULE_CLKCTRL_MODULEMODE_SW_EXPLICIT_EN,
 				    wait_for_enable);
 	};
 
 	/* Put the clock domains in HW_AUTO mode now */
-	for (i = 0; (i < max) && clk_domains[i]; i++) {
+	for (i = 0; (i < max) && clk_domains && clk_domains[i]; i++) {
 		enable_clock_domain(clk_domains[i],
 				    CD_CLKCTRL_CLKTRCTRL_HW_AUTO);
 	}
-- 
2.1.4

[U-Boot] [PATCH v2] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[Tom Rini]

On Mon, Mar 27, 2017 at 10:15:27AM +0200, Lukasz Majewski wrote:

> Up till this commit passing NULL as input parameter was allowed, but not
> handled properly.
> 
> When one passed NULL to one of this function parameters, the code was
> executed causing data abort.
> 
> However, what is more interesting, the abort was not caught because of code
> execution in HYP mode with masked CPSR A bit ("Imprecise Data Abort mask bit).
> The TI's AM57xx SoC switch to HYP mode with A bit masked in lowlevel_init.S
> due to SMC call. Such operation (by default) is performed in SoC ROM code.
> 
> The problem would pop up when one:
> - Switch back to SVC mode after disabling LPAE support
> - Somebody enables A bit (by executing cpsie a asm instruction)
> 
> and then the previously described exception would be caught.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170404/4c75993a/attachment.sig>

[U-Boot] [U-Boot, v2] ti: clocks: Fix do_enable_clocks() to accept NULL pointers as input parameters

[Tom Rini]

On Mon, Mar 27, 2017 at 10:15:27AM +0200, Lukasz Majewski wrote:

> Up till this commit passing NULL as input parameter was allowed, but not
> handled properly.
> 
> When one passed NULL to one of this function parameters, the code was
> executed causing data abort.
> 
> However, what is more interesting, the abort was not caught because of code
> execution in HYP mode with masked CPSR A bit ("Imprecise Data Abort mask bit).
> The TI's AM57xx SoC switch to HYP mode with A bit masked in lowlevel_init.S
> due to SMC call. Such operation (by default) is performed in SoC ROM code.
> 
> The problem would pop up when one:
> - Switch back to SVC mode after disabling LPAE support
> - Somebody enables A bit (by executing cpsie a asm instruction)
> 
> and then the previously described exception would be caught.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170410/a1c6b171/attachment.sig>