[U-Boot] [PATCH] fsl/usb: fix phy_type checking

[U-Boot] [PATCH] fsl/usb: fix phy_type checking

[Nikhil Badola]

Strcmp should not be used to check the argument of phy_type which maybe parsed
by hwconfig_subarg. Hwconfig_subarg returns part of hwconfig starting from the argument
(if it has the argument) till the end of the string. Since phy_type could be either
'utmi' or 'ulpi', strncmp should be used along with length limited to 4

Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
---
 drivers/usb/host/ehci-fsl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 45e5d6a..1ca7cf5 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -86,7 +86,7 @@ int ehci_hcd_init(int index, enum usb_init_type init,
 #endif
 	}
 
-	if (!strcmp(phy_type, "utmi")) {
+	if (!strncmp(phy_type, "utmi", 4)) {
 #if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY)
 		setbits_be32(&ehci->control, PHY_CLK_SEL_UTMI);
 		setbits_be32(&ehci->control, UTMI_PHY_EN);
-- 
1.7.11.7

[U-Boot] [PATCH] fsl/usb: fix phy_type checking

[Albert ARIBAUD]

Hi Nikhil,

On Mon, 17 Feb 2014 14:39:47 +0530, Nikhil Badola
<nikhil.badola@freescale.com> wrote:

> Strcmp should not be used to check the argument of phy_type which maybe parsed
> by hwconfig_subarg. Hwconfig_subarg returns part of hwconfig starting from the argument
> (if it has the argument) till the end of the string. Since phy_type could be either
> 'utmi' or 'ulpi', strncmp should be used along with length limited to 4

Not sure I understand what exact problem you are considering here. If
you know that phy_type is either "utmi" or "ulpi", then there is no
benefit in switching from strcmp() to strncmp() since there is no risk
that strcmp() overruns any of its arguments.

> Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
> Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> ---
>  drivers/usb/host/ehci-fsl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 45e5d6a..1ca7cf5 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -86,7 +86,7 @@ int ehci_hcd_init(int index, enum usb_init_type init,
>  #endif
>  	}
>  
> -	if (!strcmp(phy_type, "utmi")) {
> +	if (!strncmp(phy_type, "utmi", 4)) {
>  #if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY)
>  		setbits_be32(&ehci->control, PHY_CLK_SEL_UTMI);
>  		setbits_be32(&ehci->control, UTMI_PHY_EN);


Amicalement,
-- 
Albert.

[U-Boot] [PATCH] fsl/usb: fix phy_type checking

[nikhil.badola at freescale.com]

Hi Albert,

-----Original Message-----
From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net] 
Sent: Monday, February 17, 2014 3:05 PM
To: Badola Nikhil-B46172
Cc: u-boot at lists.denx.de; Xie Shaohui-B21989
Subject: Re: [U-Boot] [PATCH] fsl/usb: fix phy_type checking

Hi Nikhil,

On Mon, 17 Feb 2014 14:39:47 +0530, Nikhil Badola <nikhil.badola@freescale.com> wrote:

> Strcmp should not be used to check the argument of phy_type which 
> maybe parsed by hwconfig_subarg. Hwconfig_subarg returns part of 
> hwconfig starting from the argument (if it has the argument) till the 
> end of the string. Since phy_type could be either 'utmi' or 'ulpi', 
> strncmp should be used along with length limited to 4

Not sure I understand what exact problem you are considering here. If you know that phy_type is either "utmi" or "ulpi", then there is no benefit in switching from strcmp() to strncmp() since there is no risk that strcmp() overruns any of its arguments.


[Nikhil Badola] :
Strcmp() won't overrun any of its arguments but it does check NULL character at the end of both strings.
Let me explain this with an example:
Consider the hwconfig string to be defined as 
hwconfig=usb1:dr_mode=host,phy_type=utmi;fsl_ddr:bank_intlv=auto

When hwconfig string is parsed for "phy_type" sub-argument, it returns string "utmi;fsl_ddr:bank_intlv=auto" which gets stored in phy_type pointer. When strcmp() is used, then '\0' character of "utmi" string is compared with ';' of phy_type hence returning non-zero value.


> Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
> Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> ---
>  drivers/usb/host/ehci-fsl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c 
> index 45e5d6a..1ca7cf5 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -86,7 +86,7 @@ int ehci_hcd_init(int index, enum usb_init_type 
> init,  #endif
>  	}
>  
> -	if (!strcmp(phy_type, "utmi")) {
> +	if (!strncmp(phy_type, "utmi", 4)) {
>  #if defined(CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY)
>  		setbits_be32(&ehci->control, PHY_CLK_SEL_UTMI);
>  		setbits_be32(&ehci->control, UTMI_PHY_EN);


Amicalement,
--
Albert.

[U-Boot] [PATCH] fsl/usb: fix phy_type checking

[Albert ARIBAUD]

Hi nikhil.badola at freescale.com,

On Mon, 17 Feb 2014 10:06:29 +0000, "nikhil.badola at freescale.com"
<nikhil.badola@freescale.com> wrote:

> Hi Albert,
> 
> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net] 
> Sent: Monday, February 17, 2014 3:05 PM
> To: Badola Nikhil-B46172
> Cc: u-boot at lists.denx.de; Xie Shaohui-B21989
> Subject: Re: [U-Boot] [PATCH] fsl/usb: fix phy_type checking
> 
> Hi Nikhil,
> 
> On Mon, 17 Feb 2014 14:39:47 +0530, Nikhil Badola <nikhil.badola@freescale.com> wrote:
> 
> > Strcmp should not be used to check the argument of phy_type which 
> > maybe parsed by hwconfig_subarg. Hwconfig_subarg returns part of 
> > hwconfig starting from the argument (if it has the argument) till the 
> > end of the string. Since phy_type could be either 'utmi' or 'ulpi', 
> > strncmp should be used along with length limited to 4
> 
> Not sure I understand what exact problem you are considering here. If you know that phy_type is either "utmi" or "ulpi", then there is no benefit in switching from strcmp() to strncmp() since there is no risk that strcmp() overruns any of its arguments.

(seems like your mailer does not quote properly. Can you fix this?)

> [Nikhil Badola] :
> Strcmp() won't overrun any of its arguments but it does check NULL character at the end of both strings.
> Let me explain this with an example:
> Consider the hwconfig string to be defined as 
> hwconfig=usb1:dr_mode=host,phy_type=utmi;fsl_ddr:bank_intlv=auto
> 
> When hwconfig string is parsed for "phy_type" sub-argument, it returns string "utmi;fsl_ddr:bank_intlv=auto" which gets stored in phy_type pointer. When strcmp() is used, then '\0' character of "utmi" string is compared with ';' of phy_type hence returning non-zero value.

That is much clearer. Can you post a V2 with an amended commit message,
something like "limit phy_type comparison to the 4 first characters, so
that a comparison of "utmi;fsl_ddr:bank_intlv=auto" with "utmi" will
succeed".

> Amicalement,
> --
> Albert.

Amicalement,
-- 
Albert.