All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
@ 2017-10-09  8:16 ` Jia-Ju Bai
  0 siblings, 0 replies; 11+ messages in thread
From: Jia-Ju Bai @ 2017-10-09  8:16 UTC (permalink / raw)
  To: patrik.r.jakobsson, airlied, bhelgaas, forest, gregkh, simon,
	scott, tvboxspy, dan.a.cashman, golubev.mikhail
  Cc: devel, linux-pci, linux-kernel, dri-devel, Jia-Ju Bai

The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
The function call paths are:
gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
  gma_resume_pci
    pci_set_power_state
      __pci_start_power_transition (drivers/pci/pci.c)
        msleep --> may sleep

gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
  gma_resume_pci
    pci_enable_device
      pci_enable_device_flags (drivers/pci/pci.c)
        do_pci_enable_device
          pci_set_power_state
            __pci_start_power_transition
              msleep --> may sleep

vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
  pci_set_power_state
    __pci_start_power_transition (drivers/pci/pci.c)
      msleep --> may sleep

To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition

These bugs are found by my static analysis tool and my code review.

Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
 drivers/pci/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6078dfc..7b763a3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -823,7 +823,7 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
 		 */
 		if (dev->runtime_d3cold) {
 			if (dev->d3cold_delay)
-				msleep(dev->d3cold_delay);
+				mdelay(dev->d3cold_delay);
 			/*
 			 * When powering on a bridge from D3cold, the
 			 * whole hierarchy may be powered on into
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
@ 2017-10-09  8:16 ` Jia-Ju Bai
  0 siblings, 0 replies; 11+ messages in thread
From: Jia-Ju Bai @ 2017-10-09  8:16 UTC (permalink / raw)
  To: patrik.r.jakobsson, airlied, bhelgaas, forest, gregkh, simon,
	scott, tvboxspy, dan.a.cashman, golubev.mikhail
  Cc: devel, linux-pci, Jia-Ju Bai, linux-kernel, dri-devel

The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
The function call paths are:
gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
  gma_resume_pci
    pci_set_power_state
      __pci_start_power_transition (drivers/pci/pci.c)
        msleep --> may sleep

gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
  gma_resume_pci
    pci_enable_device
      pci_enable_device_flags (drivers/pci/pci.c)
        do_pci_enable_device
          pci_set_power_state
            __pci_start_power_transition
              msleep --> may sleep

vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
  pci_set_power_state
    __pci_start_power_transition (drivers/pci/pci.c)
      msleep --> may sleep

To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition

These bugs are found by my static analysis tool and my code review.

Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
 drivers/pci/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6078dfc..7b763a3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -823,7 +823,7 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
 		 */
 		if (dev->runtime_d3cold) {
 			if (dev->d3cold_delay)
-				msleep(dev->d3cold_delay);
+				mdelay(dev->d3cold_delay);
 			/*
 			 * When powering on a bridge from D3cold, the
 			 * whole hierarchy may be powered on into
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
  2017-10-09  8:16 ` Jia-Ju Bai
  (?)
@ 2017-10-09  8:17 ` Greg KH
  2017-10-09  8:32     ` Jia-Ju Bai
  -1 siblings, 1 reply; 11+ messages in thread
From: Greg KH @ 2017-10-09  8:17 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: patrik.r.jakobsson, airlied, bhelgaas, forest, simon, scott,
	tvboxspy, dan.a.cashman, golubev.mikhail, devel, linux-pci,
	linux-kernel, dri-devel

On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
> The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
> The function call paths are:
> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>   gma_resume_pci
>     pci_set_power_state
>       __pci_start_power_transition (drivers/pci/pci.c)
>         msleep --> may sleep
> 
> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>   gma_resume_pci
>     pci_enable_device
>       pci_enable_device_flags (drivers/pci/pci.c)
>         do_pci_enable_device
>           pci_set_power_state
>             __pci_start_power_transition
>               msleep --> may sleep
> 
> vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
>   pci_set_power_state
>     __pci_start_power_transition (drivers/pci/pci.c)
>       msleep --> may sleep
> 
> To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition
> 
> These bugs are found by my static analysis tool and my code review.

Wait, no, why not fix the callers to not have a spinlock.  Those are the
only users of these calls that are doing so incorrectly, don't change
the PCI core for the fault of 2 broken drivers.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
  2017-10-09  8:17 ` Greg KH
@ 2017-10-09  8:32     ` Jia-Ju Bai
  0 siblings, 0 replies; 11+ messages in thread
From: Jia-Ju Bai @ 2017-10-09  8:32 UTC (permalink / raw)
  To: Greg KH
  Cc: patrik.r.jakobsson, airlied, bhelgaas, forest, simon, scott,
	tvboxspy, dan.a.cashman, golubev.mikhail, devel, linux-pci,
	linux-kernel, dri-devel

Oh, sorry, I will send the patches for each driver.


Thanks,
Jia-Ju Bai

On 2017/10/9 16:17, Greg KH wrote:
> On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
>> The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
>> The function call paths are:
>> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>>    gma_resume_pci
>>      pci_set_power_state
>>        __pci_start_power_transition (drivers/pci/pci.c)
>>          msleep --> may sleep
>>
>> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>>    gma_resume_pci
>>      pci_enable_device
>>        pci_enable_device_flags (drivers/pci/pci.c)
>>          do_pci_enable_device
>>            pci_set_power_state
>>              __pci_start_power_transition
>>                msleep --> may sleep
>>
>> vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
>>    pci_set_power_state
>>      __pci_start_power_transition (drivers/pci/pci.c)
>>        msleep --> may sleep
>>
>> To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition
>>
>> These bugs are found by my static analysis tool and my code review.
> Wait, no, why not fix the callers to not have a spinlock.  Those are the
> only users of these calls that are doing so incorrectly, don't change
> the PCI core for the fault of 2 broken drivers.
>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
@ 2017-10-09  8:32     ` Jia-Ju Bai
  0 siblings, 0 replies; 11+ messages in thread
From: Jia-Ju Bai @ 2017-10-09  8:32 UTC (permalink / raw)
  To: Greg KH
  Cc: devel, scott, airlied, linux-pci, dri-devel, dan.a.cashman,
	linux-kernel, patrik.r.jakobsson, tvboxspy, forest, simon,
	bhelgaas, golubev.mikhail

Oh, sorry, I will send the patches for each driver.


Thanks,
Jia-Ju Bai

On 2017/10/9 16:17, Greg KH wrote:
> On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
>> The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
>> The function call paths are:
>> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>>    gma_resume_pci
>>      pci_set_power_state
>>        __pci_start_power_transition (drivers/pci/pci.c)
>>          msleep --> may sleep
>>
>> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>>    gma_resume_pci
>>      pci_enable_device
>>        pci_enable_device_flags (drivers/pci/pci.c)
>>          do_pci_enable_device
>>            pci_set_power_state
>>              __pci_start_power_transition
>>                msleep --> may sleep
>>
>> vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
>>    pci_set_power_state
>>      __pci_start_power_transition (drivers/pci/pci.c)
>>        msleep --> may sleep
>>
>> To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition
>>
>> These bugs are found by my static analysis tool and my code review.
> Wait, no, why not fix the callers to not have a spinlock.  Those are the
> only users of these calls that are doing so incorrectly, don't change
> the PCI core for the fault of 2 broken drivers.
>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
  2017-10-09  8:16 ` Jia-Ju Bai
  (?)
  (?)
@ 2017-10-09 17:15 ` Bjorn Helgaas
  2017-10-09 17:18     ` Bjorn Helgaas
  2017-10-11 13:33     ` Dan Carpenter
  -1 siblings, 2 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2017-10-09 17:15 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: patrik.r.jakobsson, airlied, bhelgaas, forest, gregkh, simon,
	scott, tvboxspy, dan.a.cashman, golubev.mikhail, devel,
	linux-pci, linux-kernel, dri-devel, Rafael J. Wysocki, linux-pm

[+cc Rafael, linux-pm]

Hi Jia-Ju,

On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
> The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
> The function call paths are:
> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>   gma_resume_pci
>     pci_set_power_state
>       __pci_start_power_transition (drivers/pci/pci.c)
>         msleep --> may sleep
> 
> gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>   gma_resume_pci
>     pci_enable_device
>       pci_enable_device_flags (drivers/pci/pci.c)
>         do_pci_enable_device
>           pci_set_power_state
>             __pci_start_power_transition
>               msleep --> may sleep
> 
> vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
>   pci_set_power_state
>     __pci_start_power_transition (drivers/pci/pci.c)
>       msleep --> may sleep
> 
> To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition
> 
> These bugs are found by my static analysis tool and my code review.

We can either

  - change pci_set_power_state() so it can be called while holding a
    spinlock (as this patch does), or
    
  - change the drivers so they don't hold the spinlock while calling 
    pci_set_power_state().

I think the latter is better because d3cold_delay is typically 100ms,
and that's a long time to spin with interrupts disabled.

I assume it's easy to produce an actual failure here?  Why haven't we
seen bug reports about this?

Bjorn

> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> ---
>  drivers/pci/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6078dfc..7b763a3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -823,7 +823,7 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
>  		 */
>  		if (dev->runtime_d3cold) {
>  			if (dev->d3cold_delay)
> -				msleep(dev->d3cold_delay);
> +				mdelay(dev->d3cold_delay);
>  			/*
>  			 * When powering on a bridge from D3cold, the
>  			 * whole hierarchy may be powered on into
> -- 
> 1.7.9.5
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
  2017-10-09 17:15 ` Bjorn Helgaas
@ 2017-10-09 17:18     ` Bjorn Helgaas
  2017-10-11 13:33     ` Dan Carpenter
  1 sibling, 0 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2017-10-09 17:18 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: patrik.r.jakobsson, airlied, bhelgaas, forest, gregkh, simon,
	scott, tvboxspy, dan.a.cashman, golubev.mikhail, devel,
	linux-pci, linux-kernel, dri-devel, Rafael J. Wysocki, linux-pm

On Mon, Oct 09, 2017 at 12:15:17PM -0500, Bjorn Helgaas wrote:
> [+cc Rafael, linux-pm]
> 
> Hi Jia-Ju,
> 
> On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
> > The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
> > The function call paths are:
> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
> >   gma_resume_pci
> >     pci_set_power_state
> >       __pci_start_power_transition (drivers/pci/pci.c)
> >         msleep --> may sleep
> > 
> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
> >   gma_resume_pci
> >     pci_enable_device
> >       pci_enable_device_flags (drivers/pci/pci.c)
> >         do_pci_enable_device
> >           pci_set_power_state
> >             __pci_start_power_transition
> >               msleep --> may sleep
> > 
> > vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
> >   pci_set_power_state
> >     __pci_start_power_transition (drivers/pci/pci.c)
> >       msleep --> may sleep
> > 
> > To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition
> > 
> > These bugs are found by my static analysis tool and my code review.
> 
> We can either
> 
>   - change pci_set_power_state() so it can be called while holding a
>     spinlock (as this patch does), or
>     
>   - change the drivers so they don't hold the spinlock while calling 
>     pci_set_power_state().
> 
> I think the latter is better because d3cold_delay is typically 100ms,
> and that's a long time to spin with interrupts disabled.
> 
> I assume it's easy to produce an actual failure here?  Why haven't we
> seen bug reports about this?

Sigh, could have saved myself some time if I'd read the whole thread
before responding :)  Sorry for repeating what Greg already said!

> > Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> > ---
> >  drivers/pci/pci.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 6078dfc..7b763a3 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -823,7 +823,7 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
> >  		 */
> >  		if (dev->runtime_d3cold) {
> >  			if (dev->d3cold_delay)
> > -				msleep(dev->d3cold_delay);
> > +				mdelay(dev->d3cold_delay);
> >  			/*
> >  			 * When powering on a bridge from D3cold, the
> >  			 * whole hierarchy may be powered on into
> > -- 
> > 1.7.9.5
> > 
> > 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
@ 2017-10-09 17:18     ` Bjorn Helgaas
  0 siblings, 0 replies; 11+ messages in thread
From: Bjorn Helgaas @ 2017-10-09 17:18 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: devel, Rafael J. Wysocki, scott, airlied, gregkh, dri-devel,
	dan.a.cashman, linux-pci, linux-kernel, patrik.r.jakobsson,
	tvboxspy, linux-pm, forest, simon, bhelgaas, golubev.mikhail

On Mon, Oct 09, 2017 at 12:15:17PM -0500, Bjorn Helgaas wrote:
> [+cc Rafael, linux-pm]
> 
> Hi Jia-Ju,
> 
> On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
> > The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
> > The function call paths are:
> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
> >   gma_resume_pci
> >     pci_set_power_state
> >       __pci_start_power_transition (drivers/pci/pci.c)
> >         msleep --> may sleep
> > 
> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
> >   gma_resume_pci
> >     pci_enable_device
> >       pci_enable_device_flags (drivers/pci/pci.c)
> >         do_pci_enable_device
> >           pci_set_power_state
> >             __pci_start_power_transition
> >               msleep --> may sleep
> > 
> > vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
> >   pci_set_power_state
> >     __pci_start_power_transition (drivers/pci/pci.c)
> >       msleep --> may sleep
> > 
> > To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition
> > 
> > These bugs are found by my static analysis tool and my code review.
> 
> We can either
> 
>   - change pci_set_power_state() so it can be called while holding a
>     spinlock (as this patch does), or
>     
>   - change the drivers so they don't hold the spinlock while calling 
>     pci_set_power_state().
> 
> I think the latter is better because d3cold_delay is typically 100ms,
> and that's a long time to spin with interrupts disabled.
> 
> I assume it's easy to produce an actual failure here?  Why haven't we
> seen bug reports about this?

Sigh, could have saved myself some time if I'd read the whole thread
before responding :)  Sorry for repeating what Greg already said!

> > Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> > ---
> >  drivers/pci/pci.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 6078dfc..7b763a3 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -823,7 +823,7 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
> >  		 */
> >  		if (dev->runtime_d3cold) {
> >  			if (dev->d3cold_delay)
> > -				msleep(dev->d3cold_delay);
> > +				mdelay(dev->d3cold_delay);
> >  			/*
> >  			 * When powering on a bridge from D3cold, the
> >  			 * whole hierarchy may be powered on into
> > -- 
> > 1.7.9.5
> > 
> > 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
  2017-10-09 17:18     ` Bjorn Helgaas
  (?)
@ 2017-10-09 18:21     ` Rafael J. Wysocki
  -1 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2017-10-09 18:21 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jia-Ju Bai, patrik.r.jakobsson, David Airlie, Bjorn Helgaas,
	forest, Greg Kroah-Hartman, simon, scott, tvboxspy,
	dan.a.cashman, golubev.mikhail, devel, Linux PCI,
	Linux Kernel Mailing List, dri-devel, Rafael J. Wysocki,
	Linux PM

On Mon, Oct 9, 2017 at 7:18 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Oct 09, 2017 at 12:15:17PM -0500, Bjorn Helgaas wrote:
>> [+cc Rafael, linux-pm]
>>
>> Hi Jia-Ju,
>>
>> On Mon, Oct 09, 2017 at 04:16:20PM +0800, Jia-Ju Bai wrote:
>> > The drivers vt6655 and gma500 call pci_set_power_state under a spinlock, which may sleep.
>> > The function call paths are:
>> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>> >   gma_resume_pci
>> >     pci_set_power_state
>> >       __pci_start_power_transition (drivers/pci/pci.c)
>> >         msleep --> may sleep
>> >
>> > gma_power_begin (acquire the spinlock) (drivers/gpu/drm/gma500/power.c)
>> >   gma_resume_pci
>> >     pci_enable_device
>> >       pci_enable_device_flags (drivers/pci/pci.c)
>> >         do_pci_enable_device
>> >           pci_set_power_state
>> >             __pci_start_power_transition
>> >               msleep --> may sleep
>> >
>> > vt6655_suspend (acquire the spinlock) (drivers/staging/vt6655/device_main.c)
>> >   pci_set_power_state
>> >     __pci_start_power_transition (drivers/pci/pci.c)
>> >       msleep --> may sleep
>> >
>> > To fix these bugs, msleep is replaced with mdelay in __pci_start_power_transition
>> >
>> > These bugs are found by my static analysis tool and my code review.
>>
>> We can either
>>
>>   - change pci_set_power_state() so it can be called while holding a
>>     spinlock (as this patch does), or
>>
>>   - change the drivers so they don't hold the spinlock while calling
>>     pci_set_power_state().
>>
>> I think the latter is better because d3cold_delay is typically 100ms,
>> and that's a long time to spin with interrupts disabled.
>>
>> I assume it's easy to produce an actual failure here?  Why haven't we
>> seen bug reports about this?
>
> Sigh, could have saved myself some time if I'd read the whole thread
> before responding :)  Sorry for repeating what Greg already said!

Well, calling pci_set_power_state() with a spinlock held is a bug,
plain and simple, among other things because it may involve running
AML.  Messing up with the single delay in it simply doesn't help.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
  2017-10-09 17:15 ` Bjorn Helgaas
@ 2017-10-11 13:33     ` Dan Carpenter
  2017-10-11 13:33     ` Dan Carpenter
  1 sibling, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2017-10-11 13:33 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jia-Ju Bai, devel, Rafael J. Wysocki, scott, airlied, gregkh,
	dri-devel, dan.a.cashman, linux-pci, linux-kernel,
	patrik.r.jakobsson, tvboxspy, linux-pm, forest, simon, bhelgaas,
	golubev.mikhail

On Mon, Oct 09, 2017 at 12:15:17PM -0500, Bjorn Helgaas wrote:
> I assume it's easy to produce an actual failure here?  Why haven't we
> seen bug reports about this?

The bug was detected with static analysis.  You have to enable a debug
feature in the .config if you want sleeping with spinlock held warnings.
Otherwise you'd have to hit the deadlock and you have to be pretty
unlucky to hit it so these bugs sometimes do go unreported for a long
time.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state
@ 2017-10-11 13:33     ` Dan Carpenter
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2017-10-11 13:33 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: devel, simon, scott, airlied, gregkh, linux-pm, dan.a.cashman,
	Rafael J. Wysocki, linux-kernel, dri-devel, tvboxspy, Jia-Ju Bai,
	patrik.r.jakobsson, linux-pci, bhelgaas, golubev.mikhail, forest

On Mon, Oct 09, 2017 at 12:15:17PM -0500, Bjorn Helgaas wrote:
> I assume it's easy to produce an actual failure here?  Why haven't we
> seen bug reports about this?

The bug was detected with static analysis.  You have to enable a debug
feature in the .config if you want sleeping with spinlock held warnings.
Otherwise you'd have to hit the deadlock and you have to be pretty
unlucky to hit it so these bugs sometimes do go unreported for a long
time.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-10-11 13:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-09  8:16 [PATCH] pci: Fix a possible sleep-in-atomic bug in pci_set_power_state Jia-Ju Bai
2017-10-09  8:16 ` Jia-Ju Bai
2017-10-09  8:17 ` Greg KH
2017-10-09  8:32   ` Jia-Ju Bai
2017-10-09  8:32     ` Jia-Ju Bai
2017-10-09 17:15 ` Bjorn Helgaas
2017-10-09 17:18   ` Bjorn Helgaas
2017-10-09 17:18     ` Bjorn Helgaas
2017-10-09 18:21     ` Rafael J. Wysocki
2017-10-11 13:33   ` Dan Carpenter
2017-10-11 13:33     ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.