[PATCH] usb: cdnsp: Fix lack of spin_lock_irqsave/spin_lock_restore

[PATCH] usb: cdnsp: Fix lack of spin_lock_irqsave/spin_lock_restore

[Pawel Laszczak]

From: Pawel Laszczak <pawell@cadence.com>

Patch puts content of cdnsp_gadget_pullup function inside
spin_lock_irqsave and spin_lock_restore section.
This construction is required here to keep the data consistency,
otherwise some data can be changed e.g. from interrupt context.

Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
Reported-by: Ken (Jian) He <jianhe@ambarella.com>
cc: <stable@vger.kernel.org>
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/cdnsp-gadget.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
index f6d231760a6a..d0c040556984 100644
--- a/drivers/usb/cdns3/cdnsp-gadget.c
+++ b/drivers/usb/cdns3/cdnsp-gadget.c
@@ -1544,8 +1544,10 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
 {
 	struct cdnsp_device *pdev = gadget_to_cdnsp(gadget);
 	struct cdns *cdns = dev_get_drvdata(pdev->dev);
+	unsigned long flags;
 
 	trace_cdnsp_pullup(is_on);
+	spin_lock_irqsave(&pdev->lock, flags);
 
 	if (!is_on) {
 		cdnsp_reset_device(pdev);
@@ -1553,6 +1555,9 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
 	} else {
 		cdns_set_vbus(cdns);
 	}
+
+	spin_unlock_irqrestore(&pdev->lock, flags);
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH] usb: cdnsp: Fix lack of spin_lock_irqsave/spin_lock_restore

[Peter Chen]

On 21-12-13 13:20:01, Pawel Laszczak wrote:
> From: Pawel Laszczak <pawell@cadence.com>
> 
> Patch puts content of cdnsp_gadget_pullup function inside
> spin_lock_irqsave and spin_lock_restore section.
> This construction is required here to keep the data consistency,
> otherwise some data can be changed e.g. from interrupt context.
> 
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> Reported-by: Ken (Jian) He <jianhe@ambarella.com>
> cc: <stable@vger.kernel.org>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/cdnsp-gadget.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
> index f6d231760a6a..d0c040556984 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -1544,8 +1544,10 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
>  {
>  	struct cdnsp_device *pdev = gadget_to_cdnsp(gadget);
>  	struct cdns *cdns = dev_get_drvdata(pdev->dev);
> +	unsigned long flags;
>  
>  	trace_cdnsp_pullup(is_on);
> +	spin_lock_irqsave(&pdev->lock, flags);

If the interrupt bottom half is pending, the consistent issue may still
exist, you may let the bottom half has finished first, eg: calling
disable_irq before spin_lock_irqsave.

Peter
>  
>  	if (!is_on) {
>  		cdnsp_reset_device(pdev);
> @@ -1553,6 +1555,9 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
>  	} else {
>  		cdns_set_vbus(cdns);
>  	}
> +
> +	spin_unlock_irqrestore(&pdev->lock, flags);
> +
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 

-- 

Thanks,
Peter Chen

RE: [PATCH] usb: cdnsp: Fix lack of spin_lock_irqsave/spin_lock_restore

[Pawel Laszczak]

>
>On 21-12-13 13:20:01, Pawel Laszczak wrote:
>> From: Pawel Laszczak <pawell@cadence.com>
>>
>> Patch puts content of cdnsp_gadget_pullup function inside
>> spin_lock_irqsave and spin_lock_restore section.
>> This construction is required here to keep the data consistency,
>> otherwise some data can be changed e.g. from interrupt context.
>>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
>> Reported-by: Ken (Jian) He <jianhe@ambarella.com>
>> cc: <stable@vger.kernel.org>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>  drivers/usb/cdns3/cdnsp-gadget.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
>> index f6d231760a6a..d0c040556984 100644
>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>> @@ -1544,8 +1544,10 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
>>  {
>>  	struct cdnsp_device *pdev = gadget_to_cdnsp(gadget);
>>  	struct cdns *cdns = dev_get_drvdata(pdev->dev);
>> +	unsigned long flags;
>>
>>  	trace_cdnsp_pullup(is_on);
>> +	spin_lock_irqsave(&pdev->lock, flags);
>
>If the interrupt bottom half is pending, the consistent issue may still
>exist, you may let the bottom half has finished first, eg: calling
>disable_irq before spin_lock_irqsave.
>
>Peter
>>

But bottom half procedure is also protected by spin lock, so it will be waiting for completion
cdnsp_gadget_pullup and vice versa.

I think you means the case when driver in bottom half function release the spin lock before calling some API function.
and in this moment the pullup function starts to be handled. 
I didn't detect such issue, but theoretically it is possible.

Let me test option with disable_irq before spin_lock_irqsave.

>>  	if (!is_on) {
>>  		cdnsp_reset_device(pdev);
>> @@ -1553,6 +1555,9 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
>>  	} else {
>>  		cdns_set_vbus(cdns);
>>  	}
>> +
>> +	spin_unlock_irqrestore(&pdev->lock, flags);
>> +
>>  	return 0;
>>  }
>>
>> --
>> 2.25.1
>>
>

--

Thanks,
Pawel Laszczak