[U-Boot] [PATCH] fsl/usb: fix phy_type checking
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);
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,
Hi Albert,
-----Original Message----- From: Albert ARIBAUD [mailto:albert.u.boot@aribaud.net] Sent: Monday, February 17, 2014 3:05 PM To: Badola Nikhil-B46172 Cc: u-boot@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.
Hi nikhil.badola@freescale.com,
On Mon, 17 Feb 2014 10:06:29 +0000, "nikhil.badola@freescale.com" nikhil.badola@freescale.com wrote:
Hi Albert,
-----Original Message----- From: Albert ARIBAUD [mailto:albert.u.boot@aribaud.net] Sent: Monday, February 17, 2014 3:05 PM To: Badola Nikhil-B46172 Cc: u-boot@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,
participants (3)
-
Albert ARIBAUD -
Nikhil Badola -
nikhil.badolaļ¼ freescale.com