Patchwork pci_get_subsys: GFP_KERNEL allocations with IRQs disabled

login
register
mail settings
Submitter Feng Tang
Date Aug. 22, 2012, 7:49 a.m.
Message ID <20120822154908.2e6ef3c0@feng-i7>
Download mbox | patch
Permalink /patch/179256/
State Changes Requested
Headers show

Comments

Feng Tang - Aug. 22, 2012, 7:49 a.m.
Hi Fengguang,


On Wed, 22 Aug 2012 10:50:08 +0800
Fengguang Wu <fengguang.wu@intel.com> wrote:

> Feng,
> 
> > I think it's pci_get_subsys() triggered this assert:
> > 
> >         /*
> >          * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
> >          */
> >         if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
> >                 return;
> 
> It's bisected down to this commit:
> 
> commit 55c844a4dd16a4d1fdc0cf2a283ec631a02ec448
> Author:     Feng Tang <feng.tang@intel.com>
> AuthorDate: Wed May 30 23:15:41 2012 +0800
> Commit:     Ingo Molnar <mingo@kernel.org>
> CommitDate: Wed Jun 6 12:03:23 2012 +0200
> 
>     x86/reboot: Fix a warning message triggered by stop_other_cpus()
> 
> Thanks,
> Fengguang

Thanks for the bisection.

Revert my commit should be a solution, but can we simply make the pci_device_id
a local on stack one instead of using sleepable kmalloc for it, as this
sounds fragile when pci_get_subsys get called in a late system reboot stage?

Thanks,
Feng

------------


> 
> > [   91.282131] machine restart
> > [   91.283895] ------------[ cut here ]------------
> > [   91.284731] WARNING: at /c/wfg/linux/kernel/lockdep.c:2739 lockdep_trace_alloc+0x1fb/0x210()
> > [   91.286132] Modules linked in:
> > [   91.286703] Pid: 697, comm: reboot Not tainted 3.5.0-00024-g01ff5db-dirty #4
> > [   91.287859] Call Trace:
> > [   91.288289]  [<81050148>] warn_slowpath_common+0xb8/0x100
> > [   91.289338]  [<8110acdb>] ? lockdep_trace_alloc+0x1fb/0x210
> > [   91.290264]  [<8110acdb>] ? lockdep_trace_alloc+0x1fb/0x210
> > [   91.291161]  [<810501ce>] warn_slowpath_null+0x3e/0x50
> > [   91.292042]  [<8110acdb>] lockdep_trace_alloc+0x1fb/0x210
> > [   91.292934]  [<81228e25>] kmem_cache_alloc_trace+0x55/0x600
> > [   91.292934]  [<813025ca>] ? kobject_put+0x9a/0x160
> > [   91.292934]  [<814e95e0>] ? klist_iter_exit+0x30/0x50
> > [   91.292934]  [<81405881>] ? bus_find_device+0xf1/0x120
> > [   91.292934]  [<81361a3c>] ? pci_get_subsys+0x11c/0x1b0
> > [   91.292934]  [<81361a3c>] pci_get_subsys+0x11c/0x1b0
> > [   91.292934]  [<81361afe>] pci_get_device+0x2e/0x40
> > [   91.292934]  [<81033e25>] mach_reboot_fixups+0xa5/0xd0
> > [   91.292934]  [<81027611>] native_machine_emergency_restart+0x1f1/0x590
> > [   91.292934]  [<814f2e00>] ? printk+0x4b/0x5b
> > [   91.292934]  [<810269ef>] native_machine_restart+0x6f/0x80
> > [   91.292934]  [<810271cc>] machine_restart+0x1c/0x30
> > [   91.292934]  [<810886e0>] kernel_restart+0x70/0xc0
> > [   91.292934]  [<81088a85>] sys_reboot+0x325/0x380
> > [   91.292934]  [<811f796c>] ? handle_pte_fault+0xdc/0x1740
> > [   91.292934]  [<811f93e7>] ? handle_mm_fault+0x417/0x4a0
> > [   91.292934]  [<8103e07b>] ? do_page_fault+0x7fb/0xb30
> > [   91.292934]  [<810b33e7>] ? up_read+0x37/0x70
> > [   91.292934]  [<8103e07b>] ? do_page_fault+0x7fb/0xb30
> > [   91.292934]  [<8123c063>] ? do_sys_open+0x3a3/0x3f0
> > [   91.292934]  [<8123c063>] ? do_sys_open+0x3a3/0x3f0
> > [   91.292934]  [<810b0270>] ? update_rmtp+0xe0/0xe0
> > [   91.292934]  [<8150376e>] ? restore_all+0xf/0xf
> > [   91.292934]  [<8103d880>] ? vmalloc_sync_all+0x320/0x320
> > [   91.292934]  [<81109fca>] ? trace_hardirqs_on_caller+0x28a/0x380
> > [   91.292934]  [<81311594>] ? trace_hardirqs_on_thunk+0xc/0x10
> > [   91.292934]  [<81503735>] syscall_call+0x7/0xb
> > 
> > Thanks,
> > Fengguang
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wu Fengguang - Aug. 22, 2012, 1:02 p.m.
On Wed, Aug 22, 2012 at 03:49:08PM +0800, Tang, Feng wrote:
> Hi Fengguang,
> 
> 
> On Wed, 22 Aug 2012 10:50:08 +0800
> Fengguang Wu <fengguang.wu@intel.com> wrote:
> 
> > Feng,
> > 
> > > I think it's pci_get_subsys() triggered this assert:
> > > 
> > >         /*
> > >          * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
> > >          */
> > >         if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
> > >                 return;
> > 
> > It's bisected down to this commit:
> > 
> > commit 55c844a4dd16a4d1fdc0cf2a283ec631a02ec448
> > Author:     Feng Tang <feng.tang@intel.com>
> > AuthorDate: Wed May 30 23:15:41 2012 +0800
> > Commit:     Ingo Molnar <mingo@kernel.org>
> > CommitDate: Wed Jun 6 12:03:23 2012 +0200
> > 
> >     x86/reboot: Fix a warning message triggered by stop_other_cpus()
> > 
> > Thanks,
> > Fengguang
> 
> Thanks for the bisection.
> 
> Revert my commit should be a solution, but can we simply make the pci_device_id
> a local on stack one instead of using sleepable kmalloc for it, as this
> sounds fragile when pci_get_subsys get called in a late system reboot stage?

Good idea! I like this simple solution. It will sure fix the warning.

Reviewed-by: Fengguang Wu <fengguang.wu@intel.com>

Thanks,
Fengguang

> ------------
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 993d4a0..e5ccede 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -246,7 +246,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
>  			       struct pci_dev *from)
>  {
>  	struct pci_dev *pdev;
> -	struct pci_device_id *id;
> +	struct pci_device_id id;
>  
>  	/*
>  	 * pci_find_subsys() can be called on the ide_setup() path,
> @@ -257,17 +257,12 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
>  	if (unlikely(no_pci_devices()))
>  		return NULL;
>  
> -	id = kzalloc(sizeof(*id), GFP_KERNEL);
> -	if (!id)
> -		return NULL;
> -	id->vendor = vendor;
> -	id->device = device;
> -	id->subvendor = ss_vendor;
> -	id->subdevice = ss_device;
> -
> -	pdev = pci_get_dev_by_id(id, from);
> -	kfree(id);
> +	id.vendor = vendor;
> +	id.device = device;
> +	id.subvendor = ss_vendor;
> +	id.subdevice = ss_device;
>  
> +	pdev = pci_get_dev_by_id(&id, from);
>  	return pdev;
>  }
>  
> 
> 
> > 
> > > [   91.282131] machine restart
> > > [   91.283895] ------------[ cut here ]------------
> > > [   91.284731] WARNING: at /c/wfg/linux/kernel/lockdep.c:2739 lockdep_trace_alloc+0x1fb/0x210()
> > > [   91.286132] Modules linked in:
> > > [   91.286703] Pid: 697, comm: reboot Not tainted 3.5.0-00024-g01ff5db-dirty #4
> > > [   91.287859] Call Trace:
> > > [   91.288289]  [<81050148>] warn_slowpath_common+0xb8/0x100
> > > [   91.289338]  [<8110acdb>] ? lockdep_trace_alloc+0x1fb/0x210
> > > [   91.290264]  [<8110acdb>] ? lockdep_trace_alloc+0x1fb/0x210
> > > [   91.291161]  [<810501ce>] warn_slowpath_null+0x3e/0x50
> > > [   91.292042]  [<8110acdb>] lockdep_trace_alloc+0x1fb/0x210
> > > [   91.292934]  [<81228e25>] kmem_cache_alloc_trace+0x55/0x600
> > > [   91.292934]  [<813025ca>] ? kobject_put+0x9a/0x160
> > > [   91.292934]  [<814e95e0>] ? klist_iter_exit+0x30/0x50
> > > [   91.292934]  [<81405881>] ? bus_find_device+0xf1/0x120
> > > [   91.292934]  [<81361a3c>] ? pci_get_subsys+0x11c/0x1b0
> > > [   91.292934]  [<81361a3c>] pci_get_subsys+0x11c/0x1b0
> > > [   91.292934]  [<81361afe>] pci_get_device+0x2e/0x40
> > > [   91.292934]  [<81033e25>] mach_reboot_fixups+0xa5/0xd0
> > > [   91.292934]  [<81027611>] native_machine_emergency_restart+0x1f1/0x590
> > > [   91.292934]  [<814f2e00>] ? printk+0x4b/0x5b
> > > [   91.292934]  [<810269ef>] native_machine_restart+0x6f/0x80
> > > [   91.292934]  [<810271cc>] machine_restart+0x1c/0x30
> > > [   91.292934]  [<810886e0>] kernel_restart+0x70/0xc0
> > > [   91.292934]  [<81088a85>] sys_reboot+0x325/0x380
> > > [   91.292934]  [<811f796c>] ? handle_pte_fault+0xdc/0x1740
> > > [   91.292934]  [<811f93e7>] ? handle_mm_fault+0x417/0x4a0
> > > [   91.292934]  [<8103e07b>] ? do_page_fault+0x7fb/0xb30
> > > [   91.292934]  [<810b33e7>] ? up_read+0x37/0x70
> > > [   91.292934]  [<8103e07b>] ? do_page_fault+0x7fb/0xb30
> > > [   91.292934]  [<8123c063>] ? do_sys_open+0x3a3/0x3f0
> > > [   91.292934]  [<8123c063>] ? do_sys_open+0x3a3/0x3f0
> > > [   91.292934]  [<810b0270>] ? update_rmtp+0xe0/0xe0
> > > [   91.292934]  [<8150376e>] ? restore_all+0xf/0xf
> > > [   91.292934]  [<8103d880>] ? vmalloc_sync_all+0x320/0x320
> > > [   91.292934]  [<81109fca>] ? trace_hardirqs_on_caller+0x28a/0x380
> > > [   91.292934]  [<81311594>] ? trace_hardirqs_on_thunk+0xc/0x10
> > > [   91.292934]  [<81503735>] syscall_call+0x7/0xb
> > > 
> > > Thanks,
> > > Fengguang
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Aug. 22, 2012, 6:02 p.m.
On Wed, Aug 22, 2012 at 12:49 AM, Feng Tang <feng.tang@intel.com> wrote:
> Hi Fengguang,
>
>
> On Wed, 22 Aug 2012 10:50:08 +0800
> Fengguang Wu <fengguang.wu@intel.com> wrote:
>
>> Feng,
>>
>> > I think it's pci_get_subsys() triggered this assert:
>> >
>> >         /*
>> >          * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
>> >          */
>> >         if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
>> >                 return;
>>
>> It's bisected down to this commit:
>>
>> commit 55c844a4dd16a4d1fdc0cf2a283ec631a02ec448
>> Author:     Feng Tang <feng.tang@intel.com>
>> AuthorDate: Wed May 30 23:15:41 2012 +0800
>> Commit:     Ingo Molnar <mingo@kernel.org>
>> CommitDate: Wed Jun 6 12:03:23 2012 +0200
>>
>>     x86/reboot: Fix a warning message triggered by stop_other_cpus()
>>
>> Thanks,
>> Fengguang
>
> Thanks for the bisection.
>
> Revert my commit should be a solution, but can we simply make the pci_device_id
> a local on stack one instead of using sleepable kmalloc for it, as this
> sounds fragile when pci_get_subsys get called in a late system reboot stage?

I think this is a great idea.  Can you make this a real patch, with a
changelog and Signed-off-by?

We should also remove the obsolete comment about early boot.  I'm not
sure the no_pci_devices() check is needed, either.  And we can make
the same simplification in pci_get_class().

> ------------
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 993d4a0..e5ccede 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -246,7 +246,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
>                                struct pci_dev *from)
>  {
>         struct pci_dev *pdev;
> -       struct pci_device_id *id;
> +       struct pci_device_id id;
>
>         /*
>          * pci_find_subsys() can be called on the ide_setup() path,
> @@ -257,17 +257,12 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
>         if (unlikely(no_pci_devices()))
>                 return NULL;
>
> -       id = kzalloc(sizeof(*id), GFP_KERNEL);
> -       if (!id)
> -               return NULL;
> -       id->vendor = vendor;
> -       id->device = device;
> -       id->subvendor = ss_vendor;
> -       id->subdevice = ss_device;
> -
> -       pdev = pci_get_dev_by_id(id, from);
> -       kfree(id);
> +       id.vendor = vendor;
> +       id.device = device;
> +       id.subvendor = ss_vendor;
> +       id.subdevice = ss_device;
>
> +       pdev = pci_get_dev_by_id(&id, from);

No need for "pdev" here, since we don't have to free anything.

>         return pdev;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Feng Tang - Aug. 23, 2012, 5:45 a.m.
Hi Bjorn,

On Wed, 22 Aug 2012 11:02:52 -0700
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Wed, Aug 22, 2012 at 12:49 AM, Feng Tang <feng.tang@intel.com> wrote:
> > Hi Fengguang,
> >
> >
> > On Wed, 22 Aug 2012 10:50:08 +0800
> > Fengguang Wu <fengguang.wu@intel.com> wrote:
> >
> >> Feng,
> >>
> >> > I think it's pci_get_subsys() triggered this assert:
> >> >
> >> >         /*
> >> >          * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
> >> >          */
> >> >         if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
> >> >                 return;
> >>
> >> It's bisected down to this commit:
> >>
> >> commit 55c844a4dd16a4d1fdc0cf2a283ec631a02ec448
> >> Author:     Feng Tang <feng.tang@intel.com>
> >> AuthorDate: Wed May 30 23:15:41 2012 +0800
> >> Commit:     Ingo Molnar <mingo@kernel.org>
> >> CommitDate: Wed Jun 6 12:03:23 2012 +0200
> >>
> >>     x86/reboot: Fix a warning message triggered by stop_other_cpus()
> >>
> >> Thanks,
> >> Fengguang
> >
> > Thanks for the bisection.
> >
> > Revert my commit should be a solution, but can we simply make the pci_device_id
> > a local on stack one instead of using sleepable kmalloc for it, as this
> > sounds fragile when pci_get_subsys get called in a late system reboot stage?
> 
> I think this is a great idea.  Can you make this a real patch, with a
> changelog and Signed-off-by?

Thanks and will do.

> 
> We should also remove the obsolete comment about early boot.  I'm not
> sure the no_pci_devices() check is needed, either.  And we can make
> the same simplification in pci_get_class().

Will check the no_pci_devices() part, and try to make it a separate
patch for easy reverting in case of error.

> 
> > ------------
> > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > index 993d4a0..e5ccede 100644
> > --- a/drivers/pci/search.c
> > +++ b/drivers/pci/search.c
> > @@ -246,7 +246,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
> >                                struct pci_dev *from)
> >  {
> > +       id.vendor = vendor;
> > +       id.device = device;
> > +       id.subvendor = ss_vendor;
> > +       id.subdevice = ss_device;
> >
> > +       pdev = pci_get_dev_by_id(&id, from);
> 
> No need for "pdev" here, since we don't have to free anything.

ok, will directly return it.

Thanks,
Feng
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 993d4a0..e5ccede 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -246,7 +246,7 @@  struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
 			       struct pci_dev *from)
 {
 	struct pci_dev *pdev;
-	struct pci_device_id *id;
+	struct pci_device_id id;
 
 	/*
 	 * pci_find_subsys() can be called on the ide_setup() path,
@@ -257,17 +257,12 @@  struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
 	if (unlikely(no_pci_devices()))
 		return NULL;
 
-	id = kzalloc(sizeof(*id), GFP_KERNEL);
-	if (!id)
-		return NULL;
-	id->vendor = vendor;
-	id->device = device;
-	id->subvendor = ss_vendor;
-	id->subdevice = ss_device;
-
-	pdev = pci_get_dev_by_id(id, from);
-	kfree(id);
+	id.vendor = vendor;
+	id.device = device;
+	id.subvendor = ss_vendor;
+	id.subdevice = ss_device;
 
+	pdev = pci_get_dev_by_id(&id, from);
 	return pdev;
 }