[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 at 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 at freescale.com>
> Signed-off-by: Nikhil Badola <nikhil.badola at 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.