[PATCH] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[PATCH] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Robert Baldyga]

This patch fixes bug described here:
https://lkml.org/lkml/2014/12/22/185

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/dwc2/core_intr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index ad43c5b..668c8dd 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -476,13 +476,13 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
 	u32 gintsts;
 	irqreturn_t retval = IRQ_NONE;
 
+	spin_lock(&hsotg->lock);
+
 	if (!dwc2_is_controller_alive(hsotg)) {
 		dev_warn(hsotg->dev, "Controller is dead\n");
 		goto out;
 	}
 
-	spin_lock(&hsotg->lock);
-
 	gintsts = dwc2_read_common_intr(hsotg);
 	if (gintsts & ~GINTSTS_PRTINT)
 		retval = IRQ_HANDLED;
-- 
1.9.1

RE: [PATCH] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Paul Zimmerman]

> From: Robert Baldyga [mailto:r.baldyga@samsung.com]
> Sent: Tuesday, January 13, 2015 2:58 AM
> 
> This patch fixes bug described here:
> https://lkml.org/lkml/2014/12/22/185
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/usb/dwc2/core_intr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index ad43c5b..668c8dd 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -476,13 +476,13 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
>  	u32 gintsts;
>  	irqreturn_t retval = IRQ_NONE;
> 
> +	spin_lock(&hsotg->lock);
> +
>  	if (!dwc2_is_controller_alive(hsotg)) {
>  		dev_warn(hsotg->dev, "Controller is dead\n");
>  		goto out;

This isn't right, now the spinlock isn't released if we take this path.

Besides, this patch doesn't really make sense. How could taking the
spinlock affect the value returned from dwc2_is_controller_alive? All
it does is read from the GSNPSID register, and that register is never
written to.

Are you absolutely sure this is the fix?

-- 
Paul

>  	}
> 
> -	spin_lock(&hsotg->lock);
> -
>  	gintsts = dwc2_read_common_intr(hsotg);
>  	if (gintsts & ~GINTSTS_PRTINT)
>  		retval = IRQ_HANDLED;
> --
> 1.9.1

Re: [PATCH] usb: dwc2: call dwc2_is_controller_alive() under spinlock

[Robert Baldyga]

On 01/13/2015 10:12 PM, Paul Zimmerman wrote:
>> From: Robert Baldyga [mailto:r.baldyga@samsung.com]
>> Sent: Tuesday, January 13, 2015 2:58 AM
>>
>> This patch fixes bug described here:
>> https://lkml.org/lkml/2014/12/22/185
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/usb/dwc2/core_intr.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index ad43c5b..668c8dd 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -476,13 +476,13 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
>>  	u32 gintsts;
>>  	irqreturn_t retval = IRQ_NONE;
>>
>> +	spin_lock(&hsotg->lock);
>> +
>>  	if (!dwc2_is_controller_alive(hsotg)) {
>>  		dev_warn(hsotg->dev, "Controller is dead\n");
>>  		goto out;
> 
> This isn't right, now the spinlock isn't released if we take this path.
> 
> Besides, this patch doesn't really make sense. How could taking the
> spinlock affect the value returned from dwc2_is_controller_alive? All
> it does is read from the GSNPSID register, and that register is never
> written to.
> 
> Are you absolutely sure this is the fix?
> 

I also don't know how is it possible that reading from GSNPSID can break
data transfer, but it looks like it does.

The problem is not about value taken from GSNPSID. This value is always
proper, and dwc2_is_controller_alive() always returns true. The problem
is that the operation of reading this register affects data transmission
process, if it's not done under spinlock. I have no idea why can it be.

I will fix this patch to release spinlock in all cases.

Thanks,
Robert Baldyga