[PATCH] drivers: usb: host: Fix GFP_KERNEL in spinlock context

[PATCH] drivers: usb: host: Fix GFP_KERNEL in spinlock context

[shrikant.maurya]

From: Shrikant Maurya <shrikant.maurya@techveda.org>

As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
API's are using GFP_KERNEL to allocate memory, which may sleep.
To ensure atomicity such allocations must be avoided in critical
sections under spinlock.
Fixed by moving part of the code which is using GFP_KERNEL for
memory allocation out of spinlock.

Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
Signed-off-by: Karthik Tummala <karthik@techveda.org>
---
Note:
- trace of device_init_wakeup() leads to creation of a struct
  wakeup_source object and addition of the object to the list
  of wakeup sources. While allocating wakeup_source object, API
  is using GFP_KERNEL. This particular section is removed out of
  spinlock.
- Changeset was compile tested and built(ARCH=arm) on linux-next
  (latest).
- No build issues reported.
- Changeset was not tested on hardware.
- Please discard this changeset, if this is not
  making the code any better.
---
 drivers/usb/host/isp116x-hcd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/isp116x-hcd.c b/drivers/usb/host/isp116x-hcd.c
index 5f9234b..47334e8 100644
--- a/drivers/usb/host/isp116x-hcd.c
+++ b/drivers/usb/host/isp116x-hcd.c
@@ -1319,6 +1319,11 @@ static int isp116x_start(struct usb_hcd *hcd)
 	u32 val;
 	unsigned long flags;
 
+	if (board->remote_wakeup_enable) {
+		if (!device_can_wakeup(hcd->self.controller))
+			device_init_wakeup(hcd->self.controller, 1);
+	}
+
 	spin_lock_irqsave(&isp116x->lock, flags);
 
 	/* clear interrupt status and disable all interrupt sources */
@@ -1369,11 +1374,8 @@ static int isp116x_start(struct usb_hcd *hcd)
 	isp116x->rhdescb = isp116x_read_reg32(isp116x, HCRHDESCB);
 
 	val = 0;
-	if (board->remote_wakeup_enable) {
-		if (!device_can_wakeup(hcd->self.controller))
-			device_init_wakeup(hcd->self.controller, 1);
+	if (board->remote_wakeup_enable)
 		val |= RH_HS_DRWE;
-	}
 	isp116x_write_reg32(isp116x, HCRHSTATUS, val);
 	isp116x->rhstatus = isp116x_read_reg32(isp116x, HCRHSTATUS);
 
-- 
1.9.1

Re: [PATCH] drivers: usb: host: Fix GFP_KERNEL in spinlock context

[Greg KH]

On Fri, Dec 22, 2017 at 11:19:24AM +0530, shrikant.maurya@techveda.org wrote:
> From: Shrikant Maurya <shrikant.maurya@techveda.org>
> 
> As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
> API's are using GFP_KERNEL to allocate memory, which may sleep.
> To ensure atomicity such allocations must be avoided in critical
> sections under spinlock.
> Fixed by moving part of the code which is using GFP_KERNEL for
> memory allocation out of spinlock.
> 
> Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
> Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
> Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
> Signed-off-by: Karthik Tummala <karthik@techveda.org>
> ---
> Note:
> - trace of device_init_wakeup() leads to creation of a struct
>   wakeup_source object and addition of the object to the list
>   of wakeup sources. While allocating wakeup_source object, API
>   is using GFP_KERNEL. This particular section is removed out of
>   spinlock.
> - Changeset was compile tested and built(ARCH=arm) on linux-next
>   (latest).
> - No build issues reported.
> - Changeset was not tested on hardware.

No one was able to test on the hardware itself?  Ideally I'd like to see
that happen to verify nothing breaks here.  No one has this chip on any
devices?

thanks,

greg k-h

Re: [PATCH] drivers: usb: host: Fix GFP_KERNEL in spinlock context

[Shrikant Maurya]

On Friday 22 December 2017 12:43 PM, Greg KH wrote:
> On Fri, Dec 22, 2017 at 11:19:24AM +0530, shrikant.maurya@techveda.org wrote:
>> From: Shrikant Maurya <shrikant.maurya@techveda.org>
>>
>> As reported by Jia-Ju Bai (https://lkml.org/lkml/2017/12/11/872):
>> API's are using GFP_KERNEL to allocate memory, which may sleep.
>> To ensure atomicity such allocations must be avoided in critical
>> sections under spinlock.
>> Fixed by moving part of the code which is using GFP_KERNEL for
>> memory allocation out of spinlock.
>>
>> Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> Signed-off-by: Shrikant Maurya <shrikant.maurya@techveda.org>
>> Signed-off-by: Suniel Mahesh <sunil.m@techveda.org>
>> Signed-off-by: Raghu Bharadwaj <raghu@techveda.org>
>> Signed-off-by: Karthik Tummala <karthik@techveda.org>
>> ---
>> Note:
>> - trace of device_init_wakeup() leads to creation of a struct
>>   wakeup_source object and addition of the object to the list
>>   of wakeup sources. While allocating wakeup_source object, API
>>   is using GFP_KERNEL. This particular section is removed out of
>>   spinlock.
>> - Changeset was compile tested and built(ARCH=arm) on linux-next
>>   (latest).
>> - No build issues reported.
>> - Changeset was not tested on hardware.
> No one was able to test on the hardware itself?  Ideally I'd like to see
> that happen to verify nothing breaks here.  No one has this chip on any
> devices?
>
> thanks,
Sorry Greg, we dont have the chip / hardware, and we are aware that it should
be hardware tested. we were looking at the software aspect.

thanks,
shrikanth
> greg k-h