[PATCH 1/1] usb: cdns3: Put the cdns set active part outside the spin lock

[PATCH 1/1] usb: cdns3: Put the cdns set active part outside the spin lock

[Xiaolei Wang]

The device may be scheduled during the resume process,
so this cannot appear in atomic operations. Since
pm_runtime_set_active will resume suppliers, put set
active outside the spin lock, which is only used to
protect the struct cdns data structure, otherwise the
kernel will report the following warning:

  BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1163
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 651, name: sh
  preempt_count: 1, expected: 0
  RCU nest depth: 0, expected: 0
  CPU: 0 PID: 651 Comm: sh Tainted: G        WC         6.1.20 #1
  Hardware name: Freescale i.MX8QM MEK (DT)
  Call trace:
    dump_backtrace.part.0+0xe0/0xf0
    show_stack+0x18/0x30
    dump_stack_lvl+0x64/0x80
    dump_stack+0x1c/0x38
    __might_resched+0x1fc/0x240
    __might_sleep+0x68/0xc0
    __pm_runtime_resume+0x9c/0xe0
    rpm_get_suppliers+0x68/0x1b0
    __pm_runtime_set_status+0x298/0x560
    cdns_resume+0xb0/0x1c0
    cdns3_controller_resume.isra.0+0x1e0/0x250
    cdns3_plat_resume+0x28/0x40

Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---
 drivers/usb/cdns3/cdns3-plat.c |  3 ++-
 drivers/usb/cdns3/cdnsp-pci.c  |  3 ++-
 drivers/usb/cdns3/core.c       | 14 +++++++++++---
 drivers/usb/cdns3/core.h       |  3 ++-
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
index 2bc5d094548b..726b2e4f67e4 100644
--- a/drivers/usb/cdns3/cdns3-plat.c
+++ b/drivers/usb/cdns3/cdns3-plat.c
@@ -256,9 +256,10 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
 	cdns3_set_platform_suspend(cdns->dev, false, false);
 
 	spin_lock_irqsave(&cdns->lock, flags);
-	cdns_resume(cdns, !PMSG_IS_AUTO(msg));
+	cdns_resume(cdns);
 	cdns->in_lpm = false;
 	spin_unlock_irqrestore(&cdns->lock, flags);
+	cdns_set_active(cdns, !PMSG_IS_AUTO(msg));
 	if (cdns->wakeup_pending) {
 		cdns->wakeup_pending = false;
 		enable_irq(cdns->wakeup_irq);
diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
index 7b151f5af3cc..0725668ffea4 100644
--- a/drivers/usb/cdns3/cdnsp-pci.c
+++ b/drivers/usb/cdns3/cdnsp-pci.c
@@ -208,8 +208,9 @@ static int __maybe_unused cdnsp_pci_resume(struct device *dev)
 	int ret;
 
 	spin_lock_irqsave(&cdns->lock, flags);
-	ret = cdns_resume(cdns, 1);
+	ret = cdns_resume(cdns);
 	spin_unlock_irqrestore(&cdns->lock, flags);
+	cdns_set_active(cdns, 1);
 
 	return ret;
 }
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index dbcdf3b24b47..661d5597fb68 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -522,7 +522,7 @@ int cdns_suspend(struct cdns *cdns)
 }
 EXPORT_SYMBOL_GPL(cdns_suspend);
 
-int cdns_resume(struct cdns *cdns, u8 set_active)
+int cdns_resume(struct cdns *cdns)
 {
 	struct device *dev = cdns->dev;
 	enum usb_role real_role;
@@ -556,15 +556,23 @@ int cdns_resume(struct cdns *cdns, u8 set_active)
 	if (cdns->roles[cdns->role]->resume)
 		cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
 
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cdns_resume);
+
+void cdns_set_active(struct cdns *cdns, u8 set_active)
+{
+	struct device *dev = cdns->dev;
+
 	if (set_active) {
 		pm_runtime_disable(dev);
 		pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
 	}
 
-	return 0;
+	return;
 }
-EXPORT_SYMBOL_GPL(cdns_resume);
+EXPORT_SYMBOL_GPL(cdns_set_active);
 #endif /* CONFIG_PM_SLEEP */
 
 MODULE_AUTHOR("Peter Chen <peter.chen@nxp.com>");
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 2d332a788871..0f429042f997 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -125,8 +125,9 @@ int cdns_init(struct cdns *cdns);
 int cdns_remove(struct cdns *cdns);
 
 #ifdef CONFIG_PM_SLEEP
-int cdns_resume(struct cdns *cdns, u8 set_active);
+int cdns_resume(struct cdns *cdns);
 int cdns_suspend(struct cdns *cdns);
+void cdns_set_active(struct cdns *cdns, u8 set_active);
 #else /* CONFIG_PM_SLEEP */
 static inline int cdns_resume(struct cdns *cdns, u8 set_active)
 { return 0; }
-- 
2.25.1

Re: [PATCH 1/1] usb: cdns3: Put the cdns set active part outside the spin lock

[wangxiaolei]

On 5/5/23 6:33 PM, Greg KH wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Thu, May 04, 2023 at 09:16:02PM +0800, Xiaolei Wang wrote:
>> The device may be scheduled during the resume process,
>> so this cannot appear in atomic operations. Since
>> pm_runtime_set_active will resume suppliers, put set
>> active outside the spin lock, which is only used to
>> protect the struct cdns data structure, otherwise the
>> kernel will report the following warning:
>>
>>    BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1163
>>    in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 651, name: sh
>>    preempt_count: 1, expected: 0
>>    RCU nest depth: 0, expected: 0
>>    CPU: 0 PID: 651 Comm: sh Tainted: G        WC         6.1.20 #1
>>    Hardware name: Freescale i.MX8QM MEK (DT)
>>    Call trace:
>>      dump_backtrace.part.0+0xe0/0xf0
>>      show_stack+0x18/0x30
>>      dump_stack_lvl+0x64/0x80
>>      dump_stack+0x1c/0x38
>>      __might_resched+0x1fc/0x240
>>      __might_sleep+0x68/0xc0
>>      __pm_runtime_resume+0x9c/0xe0
>>      rpm_get_suppliers+0x68/0x1b0
>>      __pm_runtime_set_status+0x298/0x560
>>      cdns_resume+0xb0/0x1c0
>>      cdns3_controller_resume.isra.0+0x1e0/0x250
>>      cdns3_plat_resume+0x28/0x40
>>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>>   drivers/usb/cdns3/cdns3-plat.c |  3 ++-
>>   drivers/usb/cdns3/cdnsp-pci.c  |  3 ++-
>>   drivers/usb/cdns3/core.c       | 14 +++++++++++---
>>   drivers/usb/cdns3/core.h       |  3 ++-
>>   4 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
>> index 2bc5d094548b..726b2e4f67e4 100644
>> --- a/drivers/usb/cdns3/cdns3-plat.c
>> +++ b/drivers/usb/cdns3/cdns3-plat.c
>> @@ -256,9 +256,10 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
>>        cdns3_set_platform_suspend(cdns->dev, false, false);
>>
>>        spin_lock_irqsave(&cdns->lock, flags);
>> -     cdns_resume(cdns, !PMSG_IS_AUTO(msg));
>> +     cdns_resume(cdns);
>>        cdns->in_lpm = false;
>>        spin_unlock_irqrestore(&cdns->lock, flags);
>> +     cdns_set_active(cdns, !PMSG_IS_AUTO(msg));
>>        if (cdns->wakeup_pending) {
>>                cdns->wakeup_pending = false;
>>                enable_irq(cdns->wakeup_irq);
>> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
>> index 7b151f5af3cc..0725668ffea4 100644
>> --- a/drivers/usb/cdns3/cdnsp-pci.c
>> +++ b/drivers/usb/cdns3/cdnsp-pci.c
>> @@ -208,8 +208,9 @@ static int __maybe_unused cdnsp_pci_resume(struct device *dev)
>>        int ret;
>>
>>        spin_lock_irqsave(&cdns->lock, flags);
>> -     ret = cdns_resume(cdns, 1);
>> +     ret = cdns_resume(cdns);
>>        spin_unlock_irqrestore(&cdns->lock, flags);
>> +     cdns_set_active(cdns, 1);
>>
>>        return ret;
>>   }
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index dbcdf3b24b47..661d5597fb68 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -522,7 +522,7 @@ int cdns_suspend(struct cdns *cdns)
>>   }
>>   EXPORT_SYMBOL_GPL(cdns_suspend);
>>
>> -int cdns_resume(struct cdns *cdns, u8 set_active)
>> +int cdns_resume(struct cdns *cdns)
>>   {
>>        struct device *dev = cdns->dev;
>>        enum usb_role real_role;
>> @@ -556,15 +556,23 @@ int cdns_resume(struct cdns *cdns, u8 set_active)
>>        if (cdns->roles[cdns->role]->resume)
>>                cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
>>
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(cdns_resume);
>> +
>> +void cdns_set_active(struct cdns *cdns, u8 set_active)
>> +{
>> +     struct device *dev = cdns->dev;
>> +
>>        if (set_active) {
>>                pm_runtime_disable(dev);
>>                pm_runtime_set_active(dev);
>>                pm_runtime_enable(dev);
>>        }
>>
>> -     return 0;
>> +     return;
>>   }
>> -EXPORT_SYMBOL_GPL(cdns_resume);
>> +EXPORT_SYMBOL_GPL(cdns_set_active);
>>   #endif /* CONFIG_PM_SLEEP */
>>
>>   MODULE_AUTHOR("Peter Chen <peter.chen@nxp.com>");
>> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> index 2d332a788871..0f429042f997 100644
>> --- a/drivers/usb/cdns3/core.h
>> +++ b/drivers/usb/cdns3/core.h
>> @@ -125,8 +125,9 @@ int cdns_init(struct cdns *cdns);
>>   int cdns_remove(struct cdns *cdns);
>>
>>   #ifdef CONFIG_PM_SLEEP
>> -int cdns_resume(struct cdns *cdns, u8 set_active);
>> +int cdns_resume(struct cdns *cdns);
>>   int cdns_suspend(struct cdns *cdns);
>> +void cdns_set_active(struct cdns *cdns, u8 set_active);
>>   #else /* CONFIG_PM_SLEEP */
>>   static inline int cdns_resume(struct cdns *cdns, u8 set_active)
>>   { return 0; }
>> --
>> 2.25.1
>>
>
> Hi,
>
> This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> You have sent him a patch that has triggered this response.
>
> Right now, the development tree you have sent a patch for is "closed"
> due to the timing of the merge window.  Don't worry, the patch(es) you
> have sent are not lost, and will be looked at after the merge window is
> over (after the -rc1 kernel is released by Linus).
>
> So thank you for your patience and your patches will be reviewed at this
> later time, you do not have to do anything further, this is just a short
> note to let you know the patch status and so you don't worry they didn't
> make it through.

Can someone help review this patch?

thanks

xiaolei

>
> thanks,
>
> greg k-h's patch email bot

Re: [PATCH 1/1] usb: cdns3: Put the cdns set active part outside the spin lock

[Greg KH]

On Wed, Jun 14, 2023 at 11:16:05AM +0800, wangxiaolei wrote:
> 
> On 5/5/23 6:33 PM, Greg KH wrote:
> > CAUTION: This email comes from a non Wind River email account!
> > Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > On Thu, May 04, 2023 at 09:16:02PM +0800, Xiaolei Wang wrote:
> > > The device may be scheduled during the resume process,
> > > so this cannot appear in atomic operations. Since
> > > pm_runtime_set_active will resume suppliers, put set
> > > active outside the spin lock, which is only used to
> > > protect the struct cdns data structure, otherwise the
> > > kernel will report the following warning:
> > > 
> > >    BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1163
> > >    in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 651, name: sh
> > >    preempt_count: 1, expected: 0
> > >    RCU nest depth: 0, expected: 0
> > >    CPU: 0 PID: 651 Comm: sh Tainted: G        WC         6.1.20 #1
> > >    Hardware name: Freescale i.MX8QM MEK (DT)
> > >    Call trace:
> > >      dump_backtrace.part.0+0xe0/0xf0
> > >      show_stack+0x18/0x30
> > >      dump_stack_lvl+0x64/0x80
> > >      dump_stack+0x1c/0x38
> > >      __might_resched+0x1fc/0x240
> > >      __might_sleep+0x68/0xc0
> > >      __pm_runtime_resume+0x9c/0xe0
> > >      rpm_get_suppliers+0x68/0x1b0
> > >      __pm_runtime_set_status+0x298/0x560
> > >      cdns_resume+0xb0/0x1c0
> > >      cdns3_controller_resume.isra.0+0x1e0/0x250
> > >      cdns3_plat_resume+0x28/0x40
> > > 
> > > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> > > ---
> > >   drivers/usb/cdns3/cdns3-plat.c |  3 ++-
> > >   drivers/usb/cdns3/cdnsp-pci.c  |  3 ++-
> > >   drivers/usb/cdns3/core.c       | 14 +++++++++++---
> > >   drivers/usb/cdns3/core.h       |  3 ++-
> > >   4 files changed, 17 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> > > index 2bc5d094548b..726b2e4f67e4 100644
> > > --- a/drivers/usb/cdns3/cdns3-plat.c
> > > +++ b/drivers/usb/cdns3/cdns3-plat.c
> > > @@ -256,9 +256,10 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
> > >        cdns3_set_platform_suspend(cdns->dev, false, false);
> > > 
> > >        spin_lock_irqsave(&cdns->lock, flags);
> > > -     cdns_resume(cdns, !PMSG_IS_AUTO(msg));
> > > +     cdns_resume(cdns);
> > >        cdns->in_lpm = false;
> > >        spin_unlock_irqrestore(&cdns->lock, flags);
> > > +     cdns_set_active(cdns, !PMSG_IS_AUTO(msg));
> > >        if (cdns->wakeup_pending) {
> > >                cdns->wakeup_pending = false;
> > >                enable_irq(cdns->wakeup_irq);
> > > diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> > > index 7b151f5af3cc..0725668ffea4 100644
> > > --- a/drivers/usb/cdns3/cdnsp-pci.c
> > > +++ b/drivers/usb/cdns3/cdnsp-pci.c
> > > @@ -208,8 +208,9 @@ static int __maybe_unused cdnsp_pci_resume(struct device *dev)
> > >        int ret;
> > > 
> > >        spin_lock_irqsave(&cdns->lock, flags);
> > > -     ret = cdns_resume(cdns, 1);
> > > +     ret = cdns_resume(cdns);
> > >        spin_unlock_irqrestore(&cdns->lock, flags);
> > > +     cdns_set_active(cdns, 1);
> > > 
> > >        return ret;
> > >   }
> > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > > index dbcdf3b24b47..661d5597fb68 100644
> > > --- a/drivers/usb/cdns3/core.c
> > > +++ b/drivers/usb/cdns3/core.c
> > > @@ -522,7 +522,7 @@ int cdns_suspend(struct cdns *cdns)
> > >   }
> > >   EXPORT_SYMBOL_GPL(cdns_suspend);
> > > 
> > > -int cdns_resume(struct cdns *cdns, u8 set_active)
> > > +int cdns_resume(struct cdns *cdns)
> > >   {
> > >        struct device *dev = cdns->dev;
> > >        enum usb_role real_role;
> > > @@ -556,15 +556,23 @@ int cdns_resume(struct cdns *cdns, u8 set_active)
> > >        if (cdns->roles[cdns->role]->resume)
> > >                cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
> > > 
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_resume);
> > > +
> > > +void cdns_set_active(struct cdns *cdns, u8 set_active)
> > > +{
> > > +     struct device *dev = cdns->dev;
> > > +
> > >        if (set_active) {
> > >                pm_runtime_disable(dev);
> > >                pm_runtime_set_active(dev);
> > >                pm_runtime_enable(dev);
> > >        }
> > > 
> > > -     return 0;
> > > +     return;
> > >   }
> > > -EXPORT_SYMBOL_GPL(cdns_resume);
> > > +EXPORT_SYMBOL_GPL(cdns_set_active);
> > >   #endif /* CONFIG_PM_SLEEP */
> > > 
> > >   MODULE_AUTHOR("Peter Chen <peter.chen@nxp.com>");
> > > diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> > > index 2d332a788871..0f429042f997 100644
> > > --- a/drivers/usb/cdns3/core.h
> > > +++ b/drivers/usb/cdns3/core.h
> > > @@ -125,8 +125,9 @@ int cdns_init(struct cdns *cdns);
> > >   int cdns_remove(struct cdns *cdns);
> > > 
> > >   #ifdef CONFIG_PM_SLEEP
> > > -int cdns_resume(struct cdns *cdns, u8 set_active);
> > > +int cdns_resume(struct cdns *cdns);
> > >   int cdns_suspend(struct cdns *cdns);
> > > +void cdns_set_active(struct cdns *cdns, u8 set_active);
> > >   #else /* CONFIG_PM_SLEEP */
> > >   static inline int cdns_resume(struct cdns *cdns, u8 set_active)
> > >   { return 0; }
> > > --
> > > 2.25.1
> > > 
> > 
> > Hi,
> > 
> > This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> > You have sent him a patch that has triggered this response.
> > 
> > Right now, the development tree you have sent a patch for is "closed"
> > due to the timing of the merge window.  Don't worry, the patch(es) you
> > have sent are not lost, and will be looked at after the merge window is
> > over (after the -rc1 kernel is released by Linus).
> > 
> > So thank you for your patience and your patches will be reviewed at this
> > later time, you do not have to do anything further, this is just a short
> > note to let you know the patch status and so you don't worry they didn't
> > make it through.
> 
> Can someone help review this patch?

It breaks the build:

drivers/usb/cdns3/core.c: In function ‘cdns_resume’:
drivers/usb/cdns3/core.c:527:24: error: unused variable ‘dev’ [-Werror=unused-variable]
  527 |         struct device *dev = cdns->dev;
      |                        ^~~


How did you test it?

thanks,

greg k-h

Re: [PATCH 1/1] usb: cdns3: Put the cdns set active part outside the spin lock

[wang xiaolei]

On 6/15/23 8:49 PM, Greg KH wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Wed, Jun 14, 2023 at 11:16:05AM +0800, wangxiaolei wrote:
>> On 5/5/23 6:33 PM, Greg KH wrote:
>>> CAUTION: This email comes from a non Wind River email account!
>>> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>
>>> On Thu, May 04, 2023 at 09:16:02PM +0800, Xiaolei Wang wrote:
>>>> The device may be scheduled during the resume process,
>>>> so this cannot appear in atomic operations. Since
>>>> pm_runtime_set_active will resume suppliers, put set
>>>> active outside the spin lock, which is only used to
>>>> protect the struct cdns data structure, otherwise the
>>>> kernel will report the following warning:
>>>>
>>>>     BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1163
>>>>     in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 651, name: sh
>>>>     preempt_count: 1, expected: 0
>>>>     RCU nest depth: 0, expected: 0
>>>>     CPU: 0 PID: 651 Comm: sh Tainted: G        WC         6.1.20 #1
>>>>     Hardware name: Freescale i.MX8QM MEK (DT)
>>>>     Call trace:
>>>>       dump_backtrace.part.0+0xe0/0xf0
>>>>       show_stack+0x18/0x30
>>>>       dump_stack_lvl+0x64/0x80
>>>>       dump_stack+0x1c/0x38
>>>>       __might_resched+0x1fc/0x240
>>>>       __might_sleep+0x68/0xc0
>>>>       __pm_runtime_resume+0x9c/0xe0
>>>>       rpm_get_suppliers+0x68/0x1b0
>>>>       __pm_runtime_set_status+0x298/0x560
>>>>       cdns_resume+0xb0/0x1c0
>>>>       cdns3_controller_resume.isra.0+0x1e0/0x250
>>>>       cdns3_plat_resume+0x28/0x40
>>>>
>>>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>>>> ---
>>>>    drivers/usb/cdns3/cdns3-plat.c |  3 ++-
>>>>    drivers/usb/cdns3/cdnsp-pci.c  |  3 ++-
>>>>    drivers/usb/cdns3/core.c       | 14 +++++++++++---
>>>>    drivers/usb/cdns3/core.h       |  3 ++-
>>>>    4 files changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
>>>> index 2bc5d094548b..726b2e4f67e4 100644
>>>> --- a/drivers/usb/cdns3/cdns3-plat.c
>>>> +++ b/drivers/usb/cdns3/cdns3-plat.c
>>>> @@ -256,9 +256,10 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
>>>>         cdns3_set_platform_suspend(cdns->dev, false, false);
>>>>
>>>>         spin_lock_irqsave(&cdns->lock, flags);
>>>> -     cdns_resume(cdns, !PMSG_IS_AUTO(msg));
>>>> +     cdns_resume(cdns);
>>>>         cdns->in_lpm = false;
>>>>         spin_unlock_irqrestore(&cdns->lock, flags);
>>>> +     cdns_set_active(cdns, !PMSG_IS_AUTO(msg));
>>>>         if (cdns->wakeup_pending) {
>>>>                 cdns->wakeup_pending = false;
>>>>                 enable_irq(cdns->wakeup_irq);
>>>> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
>>>> index 7b151f5af3cc..0725668ffea4 100644
>>>> --- a/drivers/usb/cdns3/cdnsp-pci.c
>>>> +++ b/drivers/usb/cdns3/cdnsp-pci.c
>>>> @@ -208,8 +208,9 @@ static int __maybe_unused cdnsp_pci_resume(struct device *dev)
>>>>         int ret;
>>>>
>>>>         spin_lock_irqsave(&cdns->lock, flags);
>>>> -     ret = cdns_resume(cdns, 1);
>>>> +     ret = cdns_resume(cdns);
>>>>         spin_unlock_irqrestore(&cdns->lock, flags);
>>>> +     cdns_set_active(cdns, 1);
>>>>
>>>>         return ret;
>>>>    }
>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>> index dbcdf3b24b47..661d5597fb68 100644
>>>> --- a/drivers/usb/cdns3/core.c
>>>> +++ b/drivers/usb/cdns3/core.c
>>>> @@ -522,7 +522,7 @@ int cdns_suspend(struct cdns *cdns)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(cdns_suspend);
>>>>
>>>> -int cdns_resume(struct cdns *cdns, u8 set_active)
>>>> +int cdns_resume(struct cdns *cdns)
>>>>    {
>>>>         struct device *dev = cdns->dev;
>>>>         enum usb_role real_role;
>>>> @@ -556,15 +556,23 @@ int cdns_resume(struct cdns *cdns, u8 set_active)
>>>>         if (cdns->roles[cdns->role]->resume)
>>>>                 cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
>>>>
>>>> +     return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(cdns_resume);
>>>> +
>>>> +void cdns_set_active(struct cdns *cdns, u8 set_active)
>>>> +{
>>>> +     struct device *dev = cdns->dev;
>>>> +
>>>>         if (set_active) {
>>>>                 pm_runtime_disable(dev);
>>>>                 pm_runtime_set_active(dev);
>>>>                 pm_runtime_enable(dev);
>>>>         }
>>>>
>>>> -     return 0;
>>>> +     return;
>>>>    }
>>>> -EXPORT_SYMBOL_GPL(cdns_resume);
>>>> +EXPORT_SYMBOL_GPL(cdns_set_active);
>>>>    #endif /* CONFIG_PM_SLEEP */
>>>>
>>>>    MODULE_AUTHOR("Peter Chen <peter.chen@nxp.com>");
>>>> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>>>> index 2d332a788871..0f429042f997 100644
>>>> --- a/drivers/usb/cdns3/core.h
>>>> +++ b/drivers/usb/cdns3/core.h
>>>> @@ -125,8 +125,9 @@ int cdns_init(struct cdns *cdns);
>>>>    int cdns_remove(struct cdns *cdns);
>>>>
>>>>    #ifdef CONFIG_PM_SLEEP
>>>> -int cdns_resume(struct cdns *cdns, u8 set_active);
>>>> +int cdns_resume(struct cdns *cdns);
>>>>    int cdns_suspend(struct cdns *cdns);
>>>> +void cdns_set_active(struct cdns *cdns, u8 set_active);
>>>>    #else /* CONFIG_PM_SLEEP */
>>>>    static inline int cdns_resume(struct cdns *cdns, u8 set_active)
>>>>    { return 0; }
>>>> --
>>>> 2.25.1
>>>>
>>> Hi,
>>>
>>> This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
>>> You have sent him a patch that has triggered this response.
>>>
>>> Right now, the development tree you have sent a patch for is "closed"
>>> due to the timing of the merge window.  Don't worry, the patch(es) you
>>> have sent are not lost, and will be looked at after the merge window is
>>> over (after the -rc1 kernel is released by Linus).
>>>
>>> So thank you for your patience and your patches will be reviewed at this
>>> later time, you do not have to do anything further, this is just a short
>>> note to let you know the patch status and so you don't worry they didn't
>>> make it through.
>> Can someone help review this patch?
> It breaks the build:
>
> drivers/usb/cdns3/core.c: In function ‘cdns_resume’:
> drivers/usb/cdns3/core.c:527:24: error: unused variable ‘dev’ [-Werror=unused-variable]
>    527 |         struct device *dev = cdns->dev;
>        |                        ^~~
>
>
> How did you test it?

Oh, sorry, this may be because I did not set the compilation parameters,

which caused me to ignore this warning. I will delete the unused code 
and send the v2 version


thanks

xiaolei

>
> thanks,
>
> greg k-h

RE: [PATCH 1/1] usb: cdns3: Put the cdns set active part outside the spin lock

[Pawel Laszczak]

>The device may be scheduled during the resume process, so this cannot
>appear in atomic operations. Since pm_runtime_set_active will resume
>suppliers, put set active outside the spin lock, which is only used to protect the
>struct cdns data structure, otherwise the kernel will report the following
>warning:
>
>  BUG: sleeping function called from invalid context at
>drivers/base/power/runtime.c:1163
>  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 651, name: sh
>  preempt_count: 1, expected: 0
>  RCU nest depth: 0, expected: 0
>  CPU: 0 PID: 651 Comm: sh Tainted: G        WC         6.1.20 #1
>  Hardware name: Freescale i.MX8QM MEK (DT)
>  Call trace:
>    dump_backtrace.part.0+0xe0/0xf0
>    show_stack+0x18/0x30
>    dump_stack_lvl+0x64/0x80
>    dump_stack+0x1c/0x38
>    __might_resched+0x1fc/0x240
>    __might_sleep+0x68/0xc0
>    __pm_runtime_resume+0x9c/0xe0
>    rpm_get_suppliers+0x68/0x1b0
>    __pm_runtime_set_status+0x298/0x560
>    cdns_resume+0xb0/0x1c0
>    cdns3_controller_resume.isra.0+0x1e0/0x250
>    cdns3_plat_resume+0x28/0x40
>
>Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>

Reviewed-by: Pawel Laszczak <pawell@cadence.com>

>---
> drivers/usb/cdns3/cdns3-plat.c |  3 ++-  drivers/usb/cdns3/cdnsp-pci.c  |  3
>++-
> drivers/usb/cdns3/core.c       | 14 +++++++++++---
> drivers/usb/cdns3/core.h       |  3 ++-
> 4 files changed, 17 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
>index 2bc5d094548b..726b2e4f67e4 100644
>--- a/drivers/usb/cdns3/cdns3-plat.c
>+++ b/drivers/usb/cdns3/cdns3-plat.c
>@@ -256,9 +256,10 @@ static int cdns3_controller_resume(struct device
>*dev, pm_message_t msg)
> 	cdns3_set_platform_suspend(cdns->dev, false, false);
>
> 	spin_lock_irqsave(&cdns->lock, flags);
>-	cdns_resume(cdns, !PMSG_IS_AUTO(msg));
>+	cdns_resume(cdns);
> 	cdns->in_lpm = false;
> 	spin_unlock_irqrestore(&cdns->lock, flags);
>+	cdns_set_active(cdns, !PMSG_IS_AUTO(msg));
> 	if (cdns->wakeup_pending) {
> 		cdns->wakeup_pending = false;
> 		enable_irq(cdns->wakeup_irq);
>diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
>index 7b151f5af3cc..0725668ffea4 100644
>--- a/drivers/usb/cdns3/cdnsp-pci.c
>+++ b/drivers/usb/cdns3/cdnsp-pci.c
>@@ -208,8 +208,9 @@ static int __maybe_unused cdnsp_pci_resume(struct
>device *dev)
> 	int ret;
>
> 	spin_lock_irqsave(&cdns->lock, flags);
>-	ret = cdns_resume(cdns, 1);
>+	ret = cdns_resume(cdns);
> 	spin_unlock_irqrestore(&cdns->lock, flags);
>+	cdns_set_active(cdns, 1);
>
> 	return ret;
> }
>diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index
>dbcdf3b24b47..661d5597fb68 100644
>--- a/drivers/usb/cdns3/core.c
>+++ b/drivers/usb/cdns3/core.c
>@@ -522,7 +522,7 @@ int cdns_suspend(struct cdns *cdns)  }
>EXPORT_SYMBOL_GPL(cdns_suspend);
>
>-int cdns_resume(struct cdns *cdns, u8 set_active)
>+int cdns_resume(struct cdns *cdns)
> {
> 	struct device *dev = cdns->dev;
> 	enum usb_role real_role;
>@@ -556,15 +556,23 @@ int cdns_resume(struct cdns *cdns, u8 set_active)
> 	if (cdns->roles[cdns->role]->resume)
> 		cdns->roles[cdns->role]->resume(cdns,
>cdns_power_is_lost(cdns));
>
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(cdns_resume);
>+
>+void cdns_set_active(struct cdns *cdns, u8 set_active) {
>+	struct device *dev = cdns->dev;
>+
> 	if (set_active) {
> 		pm_runtime_disable(dev);
> 		pm_runtime_set_active(dev);
> 		pm_runtime_enable(dev);
> 	}
>
>-	return 0;
>+	return;
> }
>-EXPORT_SYMBOL_GPL(cdns_resume);
>+EXPORT_SYMBOL_GPL(cdns_set_active);
> #endif /* CONFIG_PM_SLEEP */
>
> MODULE_AUTHOR("Peter Chen <peter.chen@nxp.com>"); diff --git
>a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h index
>2d332a788871..0f429042f997 100644
>--- a/drivers/usb/cdns3/core.h
>+++ b/drivers/usb/cdns3/core.h
>@@ -125,8 +125,9 @@ int cdns_init(struct cdns *cdns);  int
>cdns_remove(struct cdns *cdns);
>
> #ifdef CONFIG_PM_SLEEP
>-int cdns_resume(struct cdns *cdns, u8 set_active);
>+int cdns_resume(struct cdns *cdns);
> int cdns_suspend(struct cdns *cdns);
>+void cdns_set_active(struct cdns *cdns, u8 set_active);
> #else /* CONFIG_PM_SLEEP */
> static inline int cdns_resume(struct cdns *cdns, u8 set_active)  { return 0; }
>--
>2.25.1