diff mbox series

usb: cdnsp: Fix lack of spin_lock_irqsave/spin_lock_restore

Message ID 20211213122001.47370-1-pawell@gli-login.cadence.com
State New
Headers show
Series usb: cdnsp: Fix lack of spin_lock_irqsave/spin_lock_restore | expand

Commit Message

Pawel Laszczak Dec. 13, 2021, 12:20 p.m. UTC
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(+)

Comments

Peter Chen Dec. 13, 2021, 1:29 p.m. UTC | #1
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
>
Pawel Laszczak Dec. 13, 2021, 2:24 p.m. UTC | #2
>
>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
diff mbox series

Patch

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;
 }