All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
@ 2016-07-21 10:41 Ravi Babu
  2016-07-21 11:42 ` Marek Vasut
  0 siblings, 1 reply; 15+ messages in thread
From: Ravi Babu @ 2016-07-21 10:41 UTC (permalink / raw)
  To: u-boot

The crash at dwc3 driver observed due to offset misalignment
of structure members across files causing wrong code generation
and leads to crash, the issue is found during dfu test.

For instance, ther is is mismatch in code generation to
access the address of structure member dwc->dep[0] in
gadget.c and ep0.c. This leads to NULL pointer reference
casuing the crash. The inclusion of common.h fixes the issue.

The crash occurs due to below commit[1], revert of this patch
resolves the issue.

>>[1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b
>>Author: Masahiro Yamada <yamada.masahiro@socionext.com>
>>Date:   Tue Jun 28 10:48:40 2016 +0900

types.h: move and redefine resource_size_t

Currently, this is only defined in arch/arm/include/asm/types.h,
so move it to include/linux/types.h to make it available for all
architectures.

I defined it with phys_addr_t as Linux does.  I needed to surround
the define with #ifdef __KERNEL__ ... #endif to avoid build errors
in tools building.  (Host tools should not include <linux/types.h>
in the first place, but this is already messy in U-Boot...)

>>Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>Reviewed-by: Simon Glass <sjg@chromium.org>

Signed-off-by: Ravi Babu <ravibabu@ti.com>
---
 drivers/usb/dwc3/ep0.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 12b133f..f49a06e 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -14,6 +14,7 @@
  * SPDX-License-Identifier:     GPL-2.0
  */
 
+#include <common.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 10:41 [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch Ravi Babu
@ 2016-07-21 11:42 ` Marek Vasut
  2016-07-21 12:29   ` B, Ravi
  0 siblings, 1 reply; 15+ messages in thread
From: Marek Vasut @ 2016-07-21 11:42 UTC (permalink / raw)
  To: u-boot

On 07/21/2016 12:41 PM, Ravi Babu wrote:
> The crash at dwc3 driver observed due to offset misalignment
> of structure members across files causing wrong code generation
> and leads to crash, the issue is found during dfu test.
> 
> For instance, ther is is mismatch in code generation to
> access the address of structure member dwc->dep[0] in
> gadget.c and ep0.c. This leads to NULL pointer reference
> casuing the crash. The inclusion of common.h fixes the issue.

Please explain why this patch fixes the issue.

Make the explanation terse, it took me quite a while to extrapolate the
message from the text.

> The crash occurs due to below commit[1], revert of this patch
> resolves the issue.
> 
>>> [1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b
>>> Author: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> Date:   Tue Jun 28 10:48:40 2016 +0900
> 
> types.h: move and redefine resource_size_t

No need to include the whole commit message of another commit, just the
subject is enough. Also, I dunno why you add two levels of indent to the
headers of the commit, but not to the subject, this is real confusing.

> Currently, this is only defined in arch/arm/include/asm/types.h,
> so move it to include/linux/types.h to make it available for all
> architectures.
> 
> I defined it with phys_addr_t as Linux does.  I needed to surround
> the define with #ifdef __KERNEL__ ... #endif to avoid build errors
> in tools building.  (Host tools should not include <linux/types.h>
> in the first place, but this is already messy in U-Boot...)
> 
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Signed-off-by: Ravi Babu <ravibabu@ti.com>
> ---
>  drivers/usb/dwc3/ep0.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 12b133f..f49a06e 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -14,6 +14,7 @@
>   * SPDX-License-Identifier:     GPL-2.0
>   */
>  
> +#include <common.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 11:42 ` Marek Vasut
@ 2016-07-21 12:29   ` B, Ravi
  2016-07-21 12:35     ` Marek Vasut
  2016-07-22  5:10     ` Masahiro Yamada
  0 siblings, 2 replies; 15+ messages in thread
From: B, Ravi @ 2016-07-21 12:29 UTC (permalink / raw)
  To: u-boot

Hi Marek

>> The crash at dwc3 driver observed due to offset misalignment of 
>> structure members across files causing wrong code generation and leads 
>> to crash, the issue is found during dfu test.
>> 
>> For instance, ther is is mismatch in code generation to access the 
>> address of structure member dwc->dep[0] in gadget.c and ep0.c. This 
>> leads to NULL pointer reference casuing the crash. The inclusion of 
>> common.h fixes the issue.

>Please explain why this patch fixes the issue.

Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.

>Make the explanation terse, it took me quite a while to extrapolate the message from the text.

>> The crash occurs due to below commit[1], revert of this patch resolves 
>> the issue.
>> 
>> [1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b
>> Author: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Date:   Tue Jun 28 10:48:40 2016 +0900
>>
>>types.h: move and redefine resource_size_t

>No need to include the whole commit message of another commit, just the subject is enough. Also, I dunno why you add two levels of indent to the headers of the commit, but not to the subject, this is real confusing.

My bad, sorry for causing confusion. 

Regards
Ravi 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 12:29   ` B, Ravi
@ 2016-07-21 12:35     ` Marek Vasut
  2016-07-21 12:44       ` B, Ravi
  2016-07-22  5:10     ` Masahiro Yamada
  1 sibling, 1 reply; 15+ messages in thread
From: Marek Vasut @ 2016-07-21 12:35 UTC (permalink / raw)
  To: u-boot

On 07/21/2016 02:29 PM, B, Ravi wrote:
> Hi Marek
> 
>>> The crash at dwc3 driver observed due to offset misalignment of 
>>> structure members across files causing wrong code generation and leads 
>>> to crash, the issue is found during dfu test.
>>>
>>> For instance, ther is is mismatch in code generation to access the 
>>> address of structure member dwc->dep[0] in gadget.c and ep0.c. This 
>>> leads to NULL pointer reference casuing the crash. The inclusion of 
>>> common.h fixes the issue.
> 
>> Please explain why this patch fixes the issue.
> 
> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.

This stuff should be in the commit message. Still, git grep
resource_size_t does not show that it's used in gadget.c , so
I don't understand how this patch can fix things.

Also, please fix your mailer to break at 80 chars per line.

>> Make the explanation terse, it took me quite a while to extrapolate the message from the text.
> 
>>> The crash occurs due to below commit[1], revert of this patch resolves 
>>> the issue.
>>>
>>> [1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b
>>> Author: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> Date:   Tue Jun 28 10:48:40 2016 +0900
>>>
>>> types.h: move and redefine resource_size_t
> 
>> No need to include the whole commit message of another commit, just the subject is enough. Also, I dunno why you add two levels of indent to the headers of the commit, but not to the subject, this is real confusing.
> 
> My bad, sorry for causing confusion. 
> 
> Regards
> Ravi 
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 12:35     ` Marek Vasut
@ 2016-07-21 12:44       ` B, Ravi
  2016-07-21 12:54         ` Marek Vasut
  0 siblings, 1 reply; 15+ messages in thread
From: B, Ravi @ 2016-07-21 12:44 UTC (permalink / raw)
  To: u-boot

Hi Marek
 
>>> Please explain why this patch fixes the issue.
>> 
>> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.

>This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.

Thanks, I will add this text to commit message. The resource_size_t is used in dwc3 structure
defined in drivers/usb/dwc3/core.h

struct dwc3 {
	....
	struct resource  xhci_resource[DWC3_XHCI_RESOURCES_NUM];
	..
};
The resource structure defined in include/linux/ioport.h 
struct resource {
	resource_size_t start;
	resource_size_t end;
}

>Also, please fix your mailer to break at 80 chars per line.

Sure.

Regards
Ravi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 12:44       ` B, Ravi
@ 2016-07-21 12:54         ` Marek Vasut
  2016-07-21 13:03           ` B, Ravi
  0 siblings, 1 reply; 15+ messages in thread
From: Marek Vasut @ 2016-07-21 12:54 UTC (permalink / raw)
  To: u-boot

On 07/21/2016 02:44 PM, B, Ravi wrote:
> Hi Marek
>  
>>>> Please explain why this patch fixes the issue.
>>>
>>> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.
> 
>> This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
> 
> Thanks, I will add this text to commit message. The resource_size_t is used in dwc3 structure
> defined in drivers/usb/dwc3/core.h
> 
> struct dwc3 {
> 	....
> 	struct resource  xhci_resource[DWC3_XHCI_RESOURCES_NUM];
> 	..
> };
> The resource structure defined in include/linux/ioport.h 
> struct resource {
> 	resource_size_t start;
> 	resource_size_t end;
> }

OK, I am starting to get a better picture of this issue.
But somehow I suspect there is a deeper problem with the
includes -- how is it possible that the compiler didn't
complain about the resource_size_t being undefined, but
instead used (incorrectly sized) variant ?

>> Also, please fix your mailer to break at 80 chars per line.
> 
> Sure.
> 
> Regards
> Ravi
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 12:54         ` Marek Vasut
@ 2016-07-21 13:03           ` B, Ravi
  2016-07-21 13:05             ` Marek Vasut
  0 siblings, 1 reply; 15+ messages in thread
From: B, Ravi @ 2016-07-21 13:03 UTC (permalink / raw)
  To: u-boot

Hi Marek

>>  
>>>>> Please explain why this patch fixes the issue.
>>>>
>>>> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.
>> 
>>> This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
>> 
>> Thanks, I will add this text to commit message. The resource_size_t is 
>> used in dwc3 structure defined in drivers/usb/dwc3/core.h
>> 
>> struct dwc3 {
>> 	....
>> 	struct resource  xhci_resource[DWC3_XHCI_RESOURCES_NUM];
>> 	..
>> };
>> The resource structure defined in include/linux/ioport.h struct 
>> resource {
>> 	resource_size_t start;
>> 	resource_size_t end;
>> }

>OK, I am starting to get a better picture of this issue.
>But somehow I suspect there is a deeper problem with the includes -- how is it possible that the compiler didn't complain about the resource_size_t being undefined, but instead used (incorrectly sized) variant ?

Yeah, I agree.  Compiler not complaining, may be due to fallback to old definitions (32 bit) of resource_size_t because of right header file is not included.

Regards
Ravi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 13:03           ` B, Ravi
@ 2016-07-21 13:05             ` Marek Vasut
  0 siblings, 0 replies; 15+ messages in thread
From: Marek Vasut @ 2016-07-21 13:05 UTC (permalink / raw)
  To: u-boot

On 07/21/2016 03:03 PM, B, Ravi wrote:
> Hi Marek
> 
>>>  
>>>>>> Please explain why this patch fixes the issue.
>>>>>
>>>>> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.
>>>
>>>> This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
>>>
>>> Thanks, I will add this text to commit message. The resource_size_t is 
>>> used in dwc3 structure defined in drivers/usb/dwc3/core.h
>>>
>>> struct dwc3 {
>>> 	....
>>> 	struct resource  xhci_resource[DWC3_XHCI_RESOURCES_NUM];
>>> 	..
>>> };
>>> The resource structure defined in include/linux/ioport.h struct 
>>> resource {
>>> 	resource_size_t start;
>>> 	resource_size_t end;
>>> }
> 
>> OK, I am starting to get a better picture of this issue.
>> But somehow I suspect there is a deeper problem with the includes -- how is it possible that the compiler didn't complain about the resource_size_t being undefined, but instead used (incorrectly sized) variant ?
> 
> Yeah, I agree.  Compiler not complaining, may be due to fallback to old definitions (32 bit) of resource_size_t because of right header file is not included.

Well please look into it and properly explain why this happens.

And fix your damned mailer !

> Regards
> Ravi
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-21 12:29   ` B, Ravi
  2016-07-21 12:35     ` Marek Vasut
@ 2016-07-22  5:10     ` Masahiro Yamada
  2016-07-22  8:06       ` B, Ravi
                         ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Masahiro Yamada @ 2016-07-22  5:10 UTC (permalink / raw)
  To: u-boot

Hi.


2016-07-21 21:29 GMT+09:00 B, Ravi <ravibabu@ti.com>:
> Hi Marek
>
>>> The crash at dwc3 driver observed due to offset misalignment of
>>> structure members across files causing wrong code generation and leads
>>> to crash, the issue is found during dfu test.
>>>
>>> For instance, ther is is mismatch in code generation to access the
>>> address of structure member dwc->dep[0] in gadget.c and ep0.c. This
>>> leads to NULL pointer reference casuing the crash. The inclusion of
>>> common.h fixes the issue.
>
>>Please explain why this patch fixes the issue.
>
> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.


I will explain it more precisely.


Both gadget.c and ep.c include <linux/types.h>

See,
 include/linux/types.h is included
   from include/linux/kernel.h  included
   from drivers/usb/dwc3/ep0.c


So, <linux/types.h> is not a problem.


The root cause of problem is:
gadget.c include <config.h>, but ep0.c does not.


If <config.h> is not included, any CONFIGs
from the board header are defined.


The size of phys_addr_t depends on CONFIG_PHYS_64BIT
as you see in:


#ifdef CONFIG_PHYS_64BIT
typedef unsigned long long phys_addr_t;
typedef unsigned long long phys_size_t;
#else
/* DMA addresses are 32-bits wide */
typedef unsigned long phys_addr_t;
typedef unsigned long phys_size_t;
#endif


So, phys_addr_t is 8 byte in gadget.c
and phys_addr_t is 4 byte in ep0.c


My commit changed resource_size_t
based on phys_addr_t, so it triggered
the problem for DWC3, which had already potentially existed.


CONFIGs in Kconfig are guaranteed to be defined for all files,
but CONFIGs in board headers are not.

So we need to make sure to add
#include <common.h> (or #include <config.h>)
in each source file.


So, your patch is doing a right thing.

I will issue my Reviewed-by when you update the git-log.


(Moving CONFIG_PHYS_64BIT is a right thing as well)


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-22  5:10     ` Masahiro Yamada
@ 2016-07-22  8:06       ` B, Ravi
  2016-07-22 10:19       ` Marek Vasut
  2016-07-22 11:55       ` Tom Rini
  2 siblings, 0 replies; 15+ messages in thread
From: B, Ravi @ 2016-07-22  8:06 UTC (permalink / raw)
  To: u-boot

Hi Masahiro-san

>>
>>>Please explain why this patch fixes the issue.
>>
>> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.

>I will explain it more precisely.
>Both gadget.c and ep.c include <linux/types.h>
>See,
>include/linux/types.h is included
>   from include/linux/kernel.h  included
>   from drivers/usb/dwc3/ep0.c

>So, <linux/types.h> is not a problem.

>The root cause of problem is:
>gadget.c include <config.h>, but ep0.c does not.

>If <config.h> is not included, any CONFIGs from the board header are defined.

>The size of phys_addr_t depends on CONFIG_PHYS_64BIT as you see in:

>#ifdef CONFIG_PHYS_64BIT
>typedef unsigned long long phys_addr_t;
>typedef unsigned long long phys_size_t;
>#else
>/* DMA addresses are 32-bits wide */
>typedef unsigned long phys_addr_t;
>typedef unsigned long phys_size_t;
>#endif

>So, phys_addr_t is 8 byte in gadget.c
>and phys_addr_t is 4 byte in ep0.c

>My commit changed resource_size_t
>based on phys_addr_t, so it triggered
>the problem for DWC3, which had already potentially existed.

>CONFIGs in Kconfig are guaranteed to be defined for all files, but CONFIGs in board headers are not.

>So we need to make sure to add
>#include <common.h> (or #include <config.h>) in each source file.

>So, your patch is doing a right thing.

>I will issue my Reviewed-by when you update the git-log.


> (Moving CONFIG_PHYS_64BIT is a right thing as well)

Thanks for detailed explanation. 
Correct config.h or common.h is needed for all source files ( particulary for all source or device 
drivers uses resource_size_t). I will update the git log and resend the patch.

Regards
Ravi 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-22  5:10     ` Masahiro Yamada
  2016-07-22  8:06       ` B, Ravi
@ 2016-07-22 10:19       ` Marek Vasut
  2016-07-22 11:55       ` Tom Rini
  2 siblings, 0 replies; 15+ messages in thread
From: Marek Vasut @ 2016-07-22 10:19 UTC (permalink / raw)
  To: u-boot

On 07/22/2016 07:10 AM, Masahiro Yamada wrote:
> Hi.
> 
> 
> 2016-07-21 21:29 GMT+09:00 B, Ravi <ravibabu@ti.com>:
>> Hi Marek
>>
>>>> The crash at dwc3 driver observed due to offset misalignment of
>>>> structure members across files causing wrong code generation and leads
>>>> to crash, the issue is found during dfu test.
>>>>
>>>> For instance, ther is is mismatch in code generation to access the
>>>> address of structure member dwc->dep[0] in gadget.c and ep0.c. This
>>>> leads to NULL pointer reference casuing the crash. The inclusion of
>>>> common.h fixes the issue.
>>
>>> Please explain why this patch fixes the issue.
>>
>> Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
> 
> 
> I will explain it more precisely.
> 
> 
> Both gadget.c and ep.c include <linux/types.h>
> 
> See,
>  include/linux/types.h is included
>    from include/linux/kernel.h  included
>    from drivers/usb/dwc3/ep0.c
> 
> 
> So, <linux/types.h> is not a problem.
> 
> 
> The root cause of problem is:
> gadget.c include <config.h>, but ep0.c does not.
> 
> 
> If <config.h> is not included, any CONFIGs
> from the board header are defined.
> 
> 
> The size of phys_addr_t depends on CONFIG_PHYS_64BIT
> as you see in:
> 
> 
> #ifdef CONFIG_PHYS_64BIT
> typedef unsigned long long phys_addr_t;
> typedef unsigned long long phys_size_t;
> #else
> /* DMA addresses are 32-bits wide */
> typedef unsigned long phys_addr_t;
> typedef unsigned long phys_size_t;
> #endif
> 
> 
> So, phys_addr_t is 8 byte in gadget.c
> and phys_addr_t is 4 byte in ep0.c
> 
> 
> My commit changed resource_size_t
> based on phys_addr_t, so it triggered
> the problem for DWC3, which had already potentially existed.
> 
> 
> CONFIGs in Kconfig are guaranteed to be defined for all files,
> but CONFIGs in board headers are not.
> 
> So we need to make sure to add
> #include <common.h> (or #include <config.h>)
> in each source file.
> 
> 
> So, your patch is doing a right thing.
> 
> I will issue my Reviewed-by when you update the git-log.
> 
> 
> (Moving CONFIG_PHYS_64BIT is a right thing as well)
> 
> 
Thanks for the sensible and understandable explanation.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-22  5:10     ` Masahiro Yamada
  2016-07-22  8:06       ` B, Ravi
  2016-07-22 10:19       ` Marek Vasut
@ 2016-07-22 11:55       ` Tom Rini
  2016-07-26 12:59         ` B, Ravi
  2 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2016-07-22 11:55 UTC (permalink / raw)
  To: u-boot

On Fri, Jul 22, 2016 at 02:10:22PM +0900, Masahiro Yamada wrote:
> Hi.
> 
> 
> 2016-07-21 21:29 GMT+09:00 B, Ravi <ravibabu@ti.com>:
> > Hi Marek
> >
> >>> The crash at dwc3 driver observed due to offset misalignment of
> >>> structure members across files causing wrong code generation and leads
> >>> to crash, the issue is found during dfu test.
> >>>
> >>> For instance, ther is is mismatch in code generation to access the
> >>> address of structure member dwc->dep[0] in gadget.c and ep0.c. This
> >>> leads to NULL pointer reference casuing the crash. The inclusion of
> >>> common.h fixes the issue.
> >
> >>Please explain why this patch fixes the issue.
> >
> > Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence  size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
> 
> 
> I will explain it more precisely.
> 
> 
> Both gadget.c and ep.c include <linux/types.h>
> 
> See,
>  include/linux/types.h is included
>    from include/linux/kernel.h  included
>    from drivers/usb/dwc3/ep0.c
> 
> 
> So, <linux/types.h> is not a problem.
> 
> 
> The root cause of problem is:
> gadget.c include <config.h>, but ep0.c does not.
> 
> 
> If <config.h> is not included, any CONFIGs
> from the board header are defined.
> 
> 
> The size of phys_addr_t depends on CONFIG_PHYS_64BIT
> as you see in:
> 
> 
> #ifdef CONFIG_PHYS_64BIT
> typedef unsigned long long phys_addr_t;
> typedef unsigned long long phys_size_t;
> #else
> /* DMA addresses are 32-bits wide */
> typedef unsigned long phys_addr_t;
> typedef unsigned long phys_size_t;
> #endif
> 
> 
> So, phys_addr_t is 8 byte in gadget.c
> and phys_addr_t is 4 byte in ep0.c
> 
> 
> My commit changed resource_size_t
> based on phys_addr_t, so it triggered
> the problem for DWC3, which had already potentially existed.
> 
> 
> CONFIGs in Kconfig are guaranteed to be defined for all files,
> but CONFIGs in board headers are not.
> 
> So we need to make sure to add
> #include <common.h> (or #include <config.h>)
> in each source file.
> 
> 
> So, your patch is doing a right thing.
> 
> I will issue my Reviewed-by when you update the git-log.
> 
> 
> (Moving CONFIG_PHYS_64BIT is a right thing as well)

Can we move PHYS_64BIT to Kconfig instead here please?  This is the kind
of thing we should be able to select based on SoC / board.  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/20160722/3a8a14c6/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-22 11:55       ` Tom Rini
@ 2016-07-26 12:59         ` B, Ravi
  2016-07-26 13:04           ` Tom Rini
  0 siblings, 1 reply; 15+ messages in thread
From: B, Ravi @ 2016-07-26 12:59 UTC (permalink / raw)
  To: u-boot

Tom

>> based on phys_addr_t, so it triggered
>> the problem for DWC3, which had already potentially existed.
>> 
>> 
>> CONFIGs in Kconfig are guaranteed to be defined for all files, but 
>> CONFIGs in board headers are not.
>> 
>> So we need to make sure to add
>> #include <common.h> (or #include <config.h>) in each source file.
>>
>> 
>> So, your patch is doing a right thing.
>> 
>> I will issue my Reviewed-by when you update the git-log.
>> 
>> 
>> (Moving CONFIG_PHYS_64BIT is a right thing as well)

>Can we move PHYS_64BIT to Kconfig instead here please?  This is the kind of thing we should be able to select based on SoC / board.  Thanks!

Yes, moving to PHYS_64BIT to Kconfig would be ideal fix. Thanks for pointing out. 

I have posted patch.  
Commit: bac9e0: Kconfig: dra7x: Kconfig based PHYS_64BIT select based on Soc type 

Regards
Ravi 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-26 12:59         ` B, Ravi
@ 2016-07-26 13:04           ` Tom Rini
  2016-07-26 13:16             ` B, Ravi
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2016-07-26 13:04 UTC (permalink / raw)
  To: u-boot

On Tue, Jul 26, 2016 at 12:59:14PM +0000, B, Ravi wrote:
> Tom
> 
> >> based on phys_addr_t, so it triggered
> >> the problem for DWC3, which had already potentially existed.
> >> 
> >> 
> >> CONFIGs in Kconfig are guaranteed to be defined for all files, but 
> >> CONFIGs in board headers are not.
> >> 
> >> So we need to make sure to add
> >> #include <common.h> (or #include <config.h>) in each source file.
> >>
> >> 
> >> So, your patch is doing a right thing.
> >> 
> >> I will issue my Reviewed-by when you update the git-log.
> >> 
> >> 
> >> (Moving CONFIG_PHYS_64BIT is a right thing as well)
> 
> >Can we move PHYS_64BIT to Kconfig instead here please?  This is the kind of thing we should be able to select based on SoC / board.  Thanks!
> 
> Yes, moving to PHYS_64BIT to Kconfig would be ideal fix. Thanks for pointing out. 
> 
> I have posted patch.  
> Commit: bac9e0: Kconfig: dra7x: Kconfig based PHYS_64BIT select based on Soc type 

You missed that Masahiro posted one as well that converted all targets
:(  Can you please grab his and confirm that it fixes the problem as
well?  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/20160726/5c1f5c23/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
  2016-07-26 13:04           ` Tom Rini
@ 2016-07-26 13:16             ` B, Ravi
  0 siblings, 0 replies; 15+ messages in thread
From: B, Ravi @ 2016-07-26 13:16 UTC (permalink / raw)
  To: u-boot

Hi Tom

>> >> So, your patch is doing a right thing.
>> >> 
>> >> I will issue my Reviewed-by when you update the git-log.
>> >> 
>> >> 
>> >> (Moving CONFIG_PHYS_64BIT is a right thing as well)
>> 
>> >Can we move PHYS_64BIT to Kconfig instead here please?  This is the kind of thing we should be able to select based on SoC / board.  Thanks!
>> 
>> Yes, moving to PHYS_64BIT to Kconfig would be ideal fix. Thanks for pointing out. 
>> 
>> I have posted patch.  
>> Commit: bac9e0: Kconfig: dra7x: Kconfig based PHYS_64BIT select based 
>> on Soc type

>You missed that Masahiro posted one as well that converted all targets :(  Can you please grab his and confirm that it fixes the problem as well?  Thanks!

Oh yes, I missed it. Just reviewed it's the similar change and tested , worked as well. Thanks.

Regards
Ravi

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2016-07-26 13:16 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-21 10:41 [U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch Ravi Babu
2016-07-21 11:42 ` Marek Vasut
2016-07-21 12:29   ` B, Ravi
2016-07-21 12:35     ` Marek Vasut
2016-07-21 12:44       ` B, Ravi
2016-07-21 12:54         ` Marek Vasut
2016-07-21 13:03           ` B, Ravi
2016-07-21 13:05             ` Marek Vasut
2016-07-22  5:10     ` Masahiro Yamada
2016-07-22  8:06       ` B, Ravi
2016-07-22 10:19       ` Marek Vasut
2016-07-22 11:55       ` Tom Rini
2016-07-26 12:59         ` B, Ravi
2016-07-26 13:04           ` Tom Rini
2016-07-26 13:16             ` B, Ravi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.