Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Bjorn Helgaas 
On Sat, Sep 08, 2012 at 11:40:52AM -0700, Yinghai Lu wrote:
> On Sat, Sep 8, 2012 at 8:34 AM, Feng Tang  wrote:
> >> This makes lspci work again on my side. The caveat is, kzalloc() will
> >> zero out all data while the new local variable leaves some data
> >> uninitialized.
> >
> > Yes, thanks for the quick root cause and fix to the bug in my code.
> 
> Can you resubmit your patch with two extra "memset" line?

I updated the patch as follows and rebased my "next" branch to include it:

commit e664f5bd55247bba3a6ebd61f83d6c9cd87ce0de
Author: Feng Tang 
Date:   Thu Aug 23 15:45:03 2012 +0800

PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid kmalloc

This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682

pci_get_subsys() may get called in late system reboot stage, using
a sleepable kmalloc() sounds fragile and will cause a kernel warning
with my recent commmit 55c844a "x86/reboot: Fix a warning message
triggered by stop_other_cpus()" which disable local interrupt in
late system shutdown/reboot phase. Using a local parameter instead
will fix it and make it eligible for calling forom atomic context.

Do the same change for the pci_get_class() as suggested by Bjorn Helgaas

[bhelgaas: changelog, clear pci_device_id on stack with memset()]
Bisected-by: Fengguang Wu 
Signed-off-by: Feng Tang 
Signed-off-by: Bjorn Helgaas 
Reviewed-by: Fengguang Wu 

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 993d4a0..e0a0310 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -245,8 +245,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
unsigned int device,
   unsigned int ss_vendor, unsigned int ss_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,18 +256,13 @@ 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);
+   memset(, 0, sizeof(id));
+   id.vendor = vendor;
+   id.device = device;
+   id.subvendor = ss_vendor;
+   id.subdevice = ss_device;
 
-   return pdev;
+   return pci_get_dev_by_id(, from);
 }
 
 /**
@@ -307,19 +301,14 @@ pci_get_device(unsigned int vendor, unsigned int device, 
struct pci_dev *from)
  */
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
 {
-   struct pci_dev *dev;
-   struct pci_device_id *id;
+   struct pci_device_id id;
 
-   id = kzalloc(sizeof(*id), GFP_KERNEL);
-   if (!id)
-   return NULL;
-   id->vendor = id->device = id->subvendor = id->subdevice = PCI_ANY_ID;
-   id->class_mask = PCI_ANY_ID;
-   id->class = class;
+   memset(, 0, sizeof(id));
+   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
+   id.class_mask = PCI_ANY_ID;
+   id.class = class;
 
-   dev = pci_get_dev_by_id(id, from);
-   kfree(id);
-   return dev;
+   return pci_get_dev_by_id(, from);
 }
 
 /**
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Yinghai Lu 
On Sat, Sep 8, 2012 at 8:34 AM, Feng Tang  wrote:
>> This makes lspci work again on my side. The caveat is, kzalloc() will
>> zero out all data while the new local variable leaves some data
>> uninitialized.
>
> Yes, thanks for the quick root cause and fix to the bug in my code.

Can you resubmit your patch with two extra "memset" line?

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Feng Tang 
On Sat, 8 Sep 2012 21:42:20 +0800
Fengguang Wu  wrote:

> On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:
> 
> > > with this one in pci/next pci config in /sys are not created.
> > >
> > > 10:~ # lspci -tv
> > > pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
> > > lspci: Unable to read the standard configuration space header of
> > > device :00:03.0
> > > pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
> > > lspci: Unable to read the standard configuration space header of
> > > device :00:02.0
> > > pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
> > > lspci: Unable to read the standard configuration space header of
> > > device :00:01.3
> > > pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
> > > lspci: Unable to read the standard configuration space header of
> > > device :00:01.1
> > > pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
> > > lspci: Unable to read the standard configuration space header of
> > > device :00:01.0
> > > pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
> > > lspci: Unable to read the standard configuration space header of
> > > device :00:00.0
> > > -[:00]-
> > >
> > > bisected to this commit
> > >
> > > ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
> > > commit ccee7d23102f5e5765ec24779c5b77472af8f79e
> > > Author: Feng Tang 
> > > Date:   Thu Aug 23 15:45:03 2012 +0800
> > >
> > > PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
> > > kmalloc
> > >
> > > This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
> > >
> > > pci_get_subsys() may get called in late system reboot stage, using
> > > a sleepable kmalloc() sounds fragile and will cause a kernel warning
> > > with my recent commmit 55c844a "x86/reboot: Fix a warning message
> > > triggered by stop_other_cpus()" which disable local interrupt in
> > > late system shutdown/reboot phase. Using a local parameter instead
> > > will fix it and make it eligible for calling forom atomic context.
> > >
> > > Do the same change for the pci_get_class() as suggested by Bjorn 
> > > Helgaas
> > >
> > > [bhelgaas: changelog]
> > > Bisected-by: Fengguang Wu 
> > > Signed-off-by: Feng Tang 
> > > Signed-off-by: Bjorn Helgaas 
> > > Reviewed-by: Fengguang Wu 
> > >
> > > :04 04 dee62a035816b73abc68e40de8f21c7349efc4cb
> > > 70b2a6258bffa1ab963bd650d8f5d02da774fbce M  drivers
> > >
> > > so the stack get overrun ?
> > >
> > > Bjorn, I think it is this one that cause lspci broken that I mentioned
> > > during meeting at San Diego.
> 
> This makes lspci work again on my side. The caveat is, kzalloc() will
> zero out all data while the new local variable leaves some data
> uninitialized.

Yes, thanks for the quick root cause and fix to the bug in my code.

- Feng


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Yinghai Lu 
On Sat, Sep 8, 2012 at 6:42 AM, Fengguang Wu  wrote:
> On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:
>
>> > with this one in pci/next pci config in /sys are not created.
>> >
>> > 10:~ # lspci -tv
>> > pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
>> > lspci: Unable to read the standard configuration space header of
>> > device :00:03.0
>> > pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
>> > lspci: Unable to read the standard configuration space header of
>> > device :00:02.0
>> > pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
>> > lspci: Unable to read the standard configuration space header of
>> > device :00:01.3
>> > pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
>> > lspci: Unable to read the standard configuration space header of
>> > device :00:01.1
>> > pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
>> > lspci: Unable to read the standard configuration space header of
>> > device :00:01.0
>> > pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
>> > lspci: Unable to read the standard configuration space header of
>> > device :00:00.0
>> > -[:00]-
>> >
>> > bisected to this commit
>> >
>> > ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
>> > commit ccee7d23102f5e5765ec24779c5b77472af8f79e
>> > Author: Feng Tang 
>> > Date:   Thu Aug 23 15:45:03 2012 +0800
>> >
>> > PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
>> > kmalloc
>> >
>> > This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
>> >
>> > pci_get_subsys() may get called in late system reboot stage, using
>> > a sleepable kmalloc() sounds fragile and will cause a kernel warning
>> > with my recent commmit 55c844a "x86/reboot: Fix a warning message
>> > triggered by stop_other_cpus()" which disable local interrupt in
>> > late system shutdown/reboot phase. Using a local parameter instead
>> > will fix it and make it eligible for calling forom atomic context.
>> >
>> > Do the same change for the pci_get_class() as suggested by Bjorn 
>> > Helgaas
>> >
>> > [bhelgaas: changelog]
>> > Bisected-by: Fengguang Wu 
>> > Signed-off-by: Feng Tang 
>> > Signed-off-by: Bjorn Helgaas 
>> > Reviewed-by: Fengguang Wu 
>> >
>> > :04 04 dee62a035816b73abc68e40de8f21c7349efc4cb
>> > 70b2a6258bffa1ab963bd650d8f5d02da774fbce M  drivers
>> >
>> > so the stack get overrun ?
>> >
>> > Bjorn, I think it is this one that cause lspci broken that I mentioned
>> > during meeting at San Diego.
>
> This makes lspci work again on my side. The caveat is, kzalloc() will
> zero out all data while the new local variable leaves some data
> uninitialized.
>
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 78a08b1..9148b6e 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -245,7 +245,12 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
> unsigned int device,
>unsigned int ss_vendor, unsigned int ss_device,
>struct pci_dev *from)
>  {
> -   struct pci_device_id id;
> +   struct pci_device_id id = {
> +   .vendor = vendor,
> +   .device = device,
> +   .subvendor = ss_vendor,
> +   .subdevice = ss_device,
> +   };
>
> /*
>  * pci_find_subsys() can be called on the ide_setup() path,
> @@ -256,11 +261,6 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
> unsigned int device,
> if (unlikely(no_pci_devices()))
> return NULL;
>
> -   id.vendor = vendor;
> -   id.device = device;
> -   id.subvendor = ss_vendor;
> -   id.subdevice = ss_device;
> -

yes, here forget to clear .class and .class_mask

> return pci_get_dev_by_id(, from);
>  }
>
> @@ -300,11 +300,14 @@ pci_get_device(unsigned int vendor, unsigned int 
> device, struct pci_dev *from)
>   */
>  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
>  {
> -   struct pci_device_id id;
> -
> -   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
> -   id.class_mask = PCI_ANY_ID;
> -   id.class = class;
> +   struct pci_device_id id = {
> +   .vendor = PCI_ANY_ID,
> +   .device = PCI_ANY_ID,
> +   .subvendor = PCI_ANY_ID,
> +   .subdevice = PCI_ANY_ID,
> +   .class_mask = PCI_ANY_ID,
> +   .class = class,
> +   };
>
> return pci_get_dev_by_id(, from);
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Fengguang Wu 
On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:

> > with this one in pci/next pci config in /sys are not created.
> >
> > 10:~ # lspci -tv
> > pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
> > lspci: Unable to read the standard configuration space header of
> > device :00:03.0
> > pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
> > lspci: Unable to read the standard configuration space header of
> > device :00:02.0
> > pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
> > lspci: Unable to read the standard configuration space header of
> > device :00:01.3
> > pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
> > lspci: Unable to read the standard configuration space header of
> > device :00:01.1
> > pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
> > lspci: Unable to read the standard configuration space header of
> > device :00:01.0
> > pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
> > lspci: Unable to read the standard configuration space header of
> > device :00:00.0
> > -[:00]-
> >
> > bisected to this commit
> >
> > ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
> > commit ccee7d23102f5e5765ec24779c5b77472af8f79e
> > Author: Feng Tang 
> > Date:   Thu Aug 23 15:45:03 2012 +0800
> >
> > PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
> > kmalloc
> >
> > This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
> >
> > pci_get_subsys() may get called in late system reboot stage, using
> > a sleepable kmalloc() sounds fragile and will cause a kernel warning
> > with my recent commmit 55c844a "x86/reboot: Fix a warning message
> > triggered by stop_other_cpus()" which disable local interrupt in
> > late system shutdown/reboot phase. Using a local parameter instead
> > will fix it and make it eligible for calling forom atomic context.
> >
> > Do the same change for the pci_get_class() as suggested by Bjorn Helgaas
> >
> > [bhelgaas: changelog]
> > Bisected-by: Fengguang Wu 
> > Signed-off-by: Feng Tang 
> > Signed-off-by: Bjorn Helgaas 
> > Reviewed-by: Fengguang Wu 
> >
> > :04 04 dee62a035816b73abc68e40de8f21c7349efc4cb
> > 70b2a6258bffa1ab963bd650d8f5d02da774fbce M  drivers
> >
> > so the stack get overrun ?
> >
> > Bjorn, I think it is this one that cause lspci broken that I mentioned
> > during meeting at San Diego.

This makes lspci work again on my side. The caveat is, kzalloc() will
zero out all data while the new local variable leaves some data
uninitialized.

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 78a08b1..9148b6e 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -245,7 +245,12 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
unsigned int device,
   unsigned int ss_vendor, unsigned int ss_device,
   struct pci_dev *from)
 {
-   struct pci_device_id id;
+   struct pci_device_id id = {
+   .vendor = vendor,
+   .device = device,
+   .subvendor = ss_vendor,
+   .subdevice = ss_device,
+   };
 
/*
 * pci_find_subsys() can be called on the ide_setup() path,
@@ -256,11 +261,6 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
unsigned int device,
if (unlikely(no_pci_devices()))
return NULL;
 
-   id.vendor = vendor;
-   id.device = device;
-   id.subvendor = ss_vendor;
-   id.subdevice = ss_device;
-
return pci_get_dev_by_id(, from);
 }
 
@@ -300,11 +300,14 @@ pci_get_device(unsigned int vendor, unsigned int device, 
struct pci_dev *from)
  */
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
 {
-   struct pci_device_id id;
-
-   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
-   id.class_mask = PCI_ANY_ID;
-   id.class = class;
+   struct pci_device_id id = {
+   .vendor = PCI_ANY_ID,
+   .device = PCI_ANY_ID,
+   .subvendor = PCI_ANY_ID,
+   .subdevice = PCI_ANY_ID,
+   .class_mask = PCI_ANY_ID,
+   .class = class,
+   };
 
return pci_get_dev_by_id(, from);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Fengguang Wu 
On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:

  with this one in pci/next pci config in /sys are not created.
 
  10:~ # lspci -tv
  pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:03.0
  pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:02.0
  pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
  lspci: Unable to read the standard configuration space header of
  device :00:01.3
  pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
  lspci: Unable to read the standard configuration space header of
  device :00:01.1
  pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:01.0
  pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:00.0
  -[:00]-
 
  bisected to this commit
 
  ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
  commit ccee7d23102f5e5765ec24779c5b77472af8f79e
  Author: Feng Tang feng.t...@intel.com
  Date:   Thu Aug 23 15:45:03 2012 +0800
 
  PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
  kmalloc
 
  This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
 
  pci_get_subsys() may get called in late system reboot stage, using
  a sleepable kmalloc() sounds fragile and will cause a kernel warning
  with my recent commmit 55c844a x86/reboot: Fix a warning message
  triggered by stop_other_cpus() which disable local interrupt in
  late system shutdown/reboot phase. Using a local parameter instead
  will fix it and make it eligible for calling forom atomic context.
 
  Do the same change for the pci_get_class() as suggested by Bjorn Helgaas
 
  [bhelgaas: changelog]
  Bisected-by: Fengguang Wu fengguang...@intel.com
  Signed-off-by: Feng Tang feng.t...@intel.com
  Signed-off-by: Bjorn Helgaas bhelg...@google.com
  Reviewed-by: Fengguang Wu fengguang...@intel.com
 
  :04 04 dee62a035816b73abc68e40de8f21c7349efc4cb
  70b2a6258bffa1ab963bd650d8f5d02da774fbce M  drivers
 
  so the stack get overrun ?
 
  Bjorn, I think it is this one that cause lspci broken that I mentioned
  during meeting at San Diego.

This makes lspci work again on my side. The caveat is, kzalloc() will
zero out all data while the new local variable leaves some data
uninitialized.

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 78a08b1..9148b6e 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -245,7 +245,12 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
unsigned int device,
   unsigned int ss_vendor, unsigned int ss_device,
   struct pci_dev *from)
 {
-   struct pci_device_id id;
+   struct pci_device_id id = {
+   .vendor = vendor,
+   .device = device,
+   .subvendor = ss_vendor,
+   .subdevice = ss_device,
+   };
 
/*
 * pci_find_subsys() can be called on the ide_setup() path,
@@ -256,11 +261,6 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
unsigned int device,
if (unlikely(no_pci_devices()))
return NULL;
 
-   id.vendor = vendor;
-   id.device = device;
-   id.subvendor = ss_vendor;
-   id.subdevice = ss_device;
-
return pci_get_dev_by_id(id, from);
 }
 
@@ -300,11 +300,14 @@ pci_get_device(unsigned int vendor, unsigned int device, 
struct pci_dev *from)
  */
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
 {
-   struct pci_device_id id;
-
-   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
-   id.class_mask = PCI_ANY_ID;
-   id.class = class;
+   struct pci_device_id id = {
+   .vendor = PCI_ANY_ID,
+   .device = PCI_ANY_ID,
+   .subvendor = PCI_ANY_ID,
+   .subdevice = PCI_ANY_ID,
+   .class_mask = PCI_ANY_ID,
+   .class = class,
+   };
 
return pci_get_dev_by_id(id, from);
 }
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Yinghai Lu 
On Sat, Sep 8, 2012 at 6:42 AM, Fengguang Wu fengguang...@intel.com wrote:
 On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:

  with this one in pci/next pci config in /sys are not created.
 
  10:~ # lspci -tv
  pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:03.0
  pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:02.0
  pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
  lspci: Unable to read the standard configuration space header of
  device :00:01.3
  pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
  lspci: Unable to read the standard configuration space header of
  device :00:01.1
  pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:01.0
  pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
  lspci: Unable to read the standard configuration space header of
  device :00:00.0
  -[:00]-
 
  bisected to this commit
 
  ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
  commit ccee7d23102f5e5765ec24779c5b77472af8f79e
  Author: Feng Tang feng.t...@intel.com
  Date:   Thu Aug 23 15:45:03 2012 +0800
 
  PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
  kmalloc
 
  This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
 
  pci_get_subsys() may get called in late system reboot stage, using
  a sleepable kmalloc() sounds fragile and will cause a kernel warning
  with my recent commmit 55c844a x86/reboot: Fix a warning message
  triggered by stop_other_cpus() which disable local interrupt in
  late system shutdown/reboot phase. Using a local parameter instead
  will fix it and make it eligible for calling forom atomic context.
 
  Do the same change for the pci_get_class() as suggested by Bjorn 
  Helgaas
 
  [bhelgaas: changelog]
  Bisected-by: Fengguang Wu fengguang...@intel.com
  Signed-off-by: Feng Tang feng.t...@intel.com
  Signed-off-by: Bjorn Helgaas bhelg...@google.com
  Reviewed-by: Fengguang Wu fengguang...@intel.com
 
  :04 04 dee62a035816b73abc68e40de8f21c7349efc4cb
  70b2a6258bffa1ab963bd650d8f5d02da774fbce M  drivers
 
  so the stack get overrun ?
 
  Bjorn, I think it is this one that cause lspci broken that I mentioned
  during meeting at San Diego.

 This makes lspci work again on my side. The caveat is, kzalloc() will
 zero out all data while the new local variable leaves some data
 uninitialized.

 diff --git a/drivers/pci/search.c b/drivers/pci/search.c
 index 78a08b1..9148b6e 100644
 --- a/drivers/pci/search.c
 +++ b/drivers/pci/search.c
 @@ -245,7 +245,12 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
 unsigned int device,
unsigned int ss_vendor, unsigned int ss_device,
struct pci_dev *from)
  {
 -   struct pci_device_id id;
 +   struct pci_device_id id = {
 +   .vendor = vendor,
 +   .device = device,
 +   .subvendor = ss_vendor,
 +   .subdevice = ss_device,
 +   };

 /*
  * pci_find_subsys() can be called on the ide_setup() path,
 @@ -256,11 +261,6 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
 unsigned int device,
 if (unlikely(no_pci_devices()))
 return NULL;

 -   id.vendor = vendor;
 -   id.device = device;
 -   id.subvendor = ss_vendor;
 -   id.subdevice = ss_device;
 -

yes, here forget to clear .class and .class_mask

 return pci_get_dev_by_id(id, from);
  }

 @@ -300,11 +300,14 @@ pci_get_device(unsigned int vendor, unsigned int 
 device, struct pci_dev *from)
   */
  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
  {
 -   struct pci_device_id id;
 -
 -   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
 -   id.class_mask = PCI_ANY_ID;
 -   id.class = class;
 +   struct pci_device_id id = {
 +   .vendor = PCI_ANY_ID,
 +   .device = PCI_ANY_ID,
 +   .subvendor = PCI_ANY_ID,
 +   .subdevice = PCI_ANY_ID,
 +   .class_mask = PCI_ANY_ID,
 +   .class = class,
 +   };

 return pci_get_dev_by_id(id, from);
  }
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Feng Tang 
On Sat, 8 Sep 2012 21:42:20 +0800
Fengguang Wu fengguang...@intel.com wrote:

 On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:
 
   with this one in pci/next pci config in /sys are not created.
  
   10:~ # lspci -tv
   pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
   lspci: Unable to read the standard configuration space header of
   device :00:03.0
   pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
   lspci: Unable to read the standard configuration space header of
   device :00:02.0
   pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
   lspci: Unable to read the standard configuration space header of
   device :00:01.3
   pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
   lspci: Unable to read the standard configuration space header of
   device :00:01.1
   pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
   lspci: Unable to read the standard configuration space header of
   device :00:01.0
   pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
   lspci: Unable to read the standard configuration space header of
   device :00:00.0
   -[:00]-
  
   bisected to this commit
  
   ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
   commit ccee7d23102f5e5765ec24779c5b77472af8f79e
   Author: Feng Tang feng.t...@intel.com
   Date:   Thu Aug 23 15:45:03 2012 +0800
  
   PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
   kmalloc
  
   This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
  
   pci_get_subsys() may get called in late system reboot stage, using
   a sleepable kmalloc() sounds fragile and will cause a kernel warning
   with my recent commmit 55c844a x86/reboot: Fix a warning message
   triggered by stop_other_cpus() which disable local interrupt in
   late system shutdown/reboot phase. Using a local parameter instead
   will fix it and make it eligible for calling forom atomic context.
  
   Do the same change for the pci_get_class() as suggested by Bjorn 
   Helgaas
  
   [bhelgaas: changelog]
   Bisected-by: Fengguang Wu fengguang...@intel.com
   Signed-off-by: Feng Tang feng.t...@intel.com
   Signed-off-by: Bjorn Helgaas bhelg...@google.com
   Reviewed-by: Fengguang Wu fengguang...@intel.com
  
   :04 04 dee62a035816b73abc68e40de8f21c7349efc4cb
   70b2a6258bffa1ab963bd650d8f5d02da774fbce M  drivers
  
   so the stack get overrun ?
  
   Bjorn, I think it is this one that cause lspci broken that I mentioned
   during meeting at San Diego.
 
 This makes lspci work again on my side. The caveat is, kzalloc() will
 zero out all data while the new local variable leaves some data
 uninitialized.

Yes, thanks for the quick root cause and fix to the bug in my code.

- Feng


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Yinghai Lu 
On Sat, Sep 8, 2012 at 8:34 AM, Feng Tang feng.t...@intel.com wrote:
 This makes lspci work again on my side. The caveat is, kzalloc() will
 zero out all data while the new local variable leaves some data
 uninitialized.

 Yes, thanks for the quick root cause and fix to the bug in my code.

Can you resubmit your patch with two extra memset line?

Thanks

Yinghai
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-08 Thread Bjorn Helgaas 
On Sat, Sep 08, 2012 at 11:40:52AM -0700, Yinghai Lu wrote:
 On Sat, Sep 8, 2012 at 8:34 AM, Feng Tang feng.t...@intel.com wrote:
  This makes lspci work again on my side. The caveat is, kzalloc() will
  zero out all data while the new local variable leaves some data
  uninitialized.
 
  Yes, thanks for the quick root cause and fix to the bug in my code.
 
 Can you resubmit your patch with two extra memset line?

I updated the patch as follows and rebased my next branch to include it:

commit e664f5bd55247bba3a6ebd61f83d6c9cd87ce0de
Author: Feng Tang feng.t...@intel.com
Date:   Thu Aug 23 15:45:03 2012 +0800

PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid kmalloc

This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682

pci_get_subsys() may get called in late system reboot stage, using
a sleepable kmalloc() sounds fragile and will cause a kernel warning
with my recent commmit 55c844a x86/reboot: Fix a warning message
triggered by stop_other_cpus() which disable local interrupt in
late system shutdown/reboot phase. Using a local parameter instead
will fix it and make it eligible for calling forom atomic context.

Do the same change for the pci_get_class() as suggested by Bjorn Helgaas

[bhelgaas: changelog, clear pci_device_id on stack with memset()]
Bisected-by: Fengguang Wu fengguang...@intel.com
Signed-off-by: Feng Tang feng.t...@intel.com
Signed-off-by: Bjorn Helgaas bhelg...@google.com
Reviewed-by: Fengguang Wu fengguang...@intel.com

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 993d4a0..e0a0310 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -245,8 +245,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
unsigned int device,
   unsigned int ss_vendor, unsigned int ss_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,18 +256,13 @@ 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);
+   memset(id, 0, sizeof(id));
+   id.vendor = vendor;
+   id.device = device;
+   id.subvendor = ss_vendor;
+   id.subdevice = ss_device;
 
-   return pdev;
+   return pci_get_dev_by_id(id, from);
 }
 
 /**
@@ -307,19 +301,14 @@ pci_get_device(unsigned int vendor, unsigned int device, 
struct pci_dev *from)
  */
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
 {
-   struct pci_dev *dev;
-   struct pci_device_id *id;
+   struct pci_device_id id;
 
-   id = kzalloc(sizeof(*id), GFP_KERNEL);
-   if (!id)
-   return NULL;
-   id-vendor = id-device = id-subvendor = id-subdevice = PCI_ANY_ID;
-   id-class_mask = PCI_ANY_ID;
-   id-class = class;
+   memset(id, 0, sizeof(id));
+   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
+   id.class_mask = PCI_ANY_ID;
+   id.class = class;
 
-   dev = pci_get_dev_by_id(id, from);
-   kfree(id);
-   return dev;
+   return pci_get_dev_by_id(id, from);
 }
 
 /**
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-07 Thread Greg Kroah-Hartman 
On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:
> Greg,
> 
> Any reason for using kmalloc instead of local variable during your
> rewriting pci search code?
> 
> commit 95247b57ed844511a212265b45cf9a919753aea1
> Author: Greg Kroah-Hartman 
> Date:   Wed Feb 13 11:03:58 2008 -0800

Seriously?  Something I wrote 4 years ago?  I really have no idea,
sorry.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-07 Thread Yinghai Lu 
On Fri, Sep 7, 2012 at 6:00 PM, Yinghai Lu  wrote:
> On Thu, Aug 23, 2012 at 12:45 AM, Feng Tang  wrote:
>> From 57a28ee5e7662ca28ba4c793aa037d64bd082dee Mon Sep 17 00:00:00 2001
>> From: Feng Tang 
>> Date: Wed, 22 Aug 2012 15:41:51 +0800
>> Subject: [PATCH 1/2] PCI: Use local parameter pci_device_id for 
>> pci_get_subsys/class()
>>
>> This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
>>
>> pci_get_subsys() may get called in late system reboot stage, using
>> a sleepable kmalloc() sounds fragile and will casue a kernel warning
>> with my recent commmit 55c844a "x86/reboot: Fix a warning message
>> triggered by stop_other_cpus()" which disable local interrupt in
>> late system shutdown/reboot phase. Using a local parameter instead
>> will fix it and make it eligible for calling forom atomic context.
>>
>> Do the same change for the pci_get_class() as suggeted by Bjorn Helgaas
>>
>> Bisected-by: Fengguang Wu 
>> Signed-off-by: Feng Tang 
>> Reviewed-by: Fengguang Wu 
>> ---
>>  drivers/pci/search.c |   35 +++
>>  1 files changed, 11 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
>> index 993d4a0..78a08b1 100644
>> --- a/drivers/pci/search.c
>> +++ b/drivers/pci/search.c
>> @@ -245,8 +245,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
>> unsigned int device,
>>unsigned int ss_vendor, unsigned int 
>> ss_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,18 +256,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;
>>
>> -   return pdev;
>> +   return pci_get_dev_by_id(, from);
>>  }
>>
>>  /**
>> @@ -307,19 +300,13 @@ pci_get_device(unsigned int vendor, unsigned int 
>> device, struct pci_dev *from)
>>   */
>>  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
>>  {
>> -   struct pci_dev *dev;
>> -   struct pci_device_id *id;
>> +   struct pci_device_id id;
>>
>> -   id = kzalloc(sizeof(*id), GFP_KERNEL);
>> -   if (!id)
>> -   return NULL;
>> -   id->vendor = id->device = id->subvendor = id->subdevice = PCI_ANY_ID;
>> -   id->class_mask = PCI_ANY_ID;
>> -   id->class = class;
>> +   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
>> +   id.class_mask = PCI_ANY_ID;
>> +   id.class = class;
>>
>> -   dev = pci_get_dev_by_id(id, from);
>> -   kfree(id);
>> -   return dev;
>> +   return pci_get_dev_by_id(, from);
>>  }
>>
>>  /**
>
> with this one in pci/next pci config in /sys are not created.
>
> 10:~ # lspci -tv
> pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
> lspci: Unable to read the standard configuration space header of
> device :00:03.0
> pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
> lspci: Unable to read the standard configuration space header of
> device :00:02.0
> pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
> lspci: Unable to read the standard configuration space header of
> device :00:01.3
> pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
> lspci: Unable to read the standard configuration space header of
> device :00:01.1
> pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
> lspci: Unable to read the standard configuration space header of
> device :00:01.0
> pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
> lspci: Unable to read the standard configuration space header of
> device :00:00.0
> -[:00]-
>
> bisected to this commit
>
> ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
> commit ccee7d23102f5e5765ec24779c5b77472af8f79e
> Author: Feng Tang 
> Date:   Thu Aug 23 15:45:03 2012 +0800
>
> PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
> kmalloc
>
> This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
>
> pci_get_subsys() may get called in late system reboot stage, using
> a sleepable kmalloc() sounds fragile and will cause a kernel warning
> with my recent commmit 55c844a "x86/reboot: Fix a warning message
> triggered by stop_other_cpus()" which disable local interrupt in
> late system

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-07 Thread Yinghai Lu 
On Thu, Aug 23, 2012 at 12:45 AM, Feng Tang  wrote:
> From 57a28ee5e7662ca28ba4c793aa037d64bd082dee Mon Sep 17 00:00:00 2001
> From: Feng Tang 
> Date: Wed, 22 Aug 2012 15:41:51 +0800
> Subject: [PATCH 1/2] PCI: Use local parameter pci_device_id for 
> pci_get_subsys/class()
>
> This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682
>
> pci_get_subsys() may get called in late system reboot stage, using
> a sleepable kmalloc() sounds fragile and will casue a kernel warning
> with my recent commmit 55c844a "x86/reboot: Fix a warning message
> triggered by stop_other_cpus()" which disable local interrupt in
> late system shutdown/reboot phase. Using a local parameter instead
> will fix it and make it eligible for calling forom atomic context.
>
> Do the same change for the pci_get_class() as suggeted by Bjorn Helgaas
>
> Bisected-by: Fengguang Wu 
> Signed-off-by: Feng Tang 
> Reviewed-by: Fengguang Wu 
> ---
>  drivers/pci/search.c |   35 +++
>  1 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 993d4a0..78a08b1 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -245,8 +245,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
> unsigned int device,
>unsigned int ss_vendor, unsigned int ss_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,18 +256,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;
>
> -   return pdev;
> +   return pci_get_dev_by_id(, from);
>  }
>
>  /**
> @@ -307,19 +300,13 @@ pci_get_device(unsigned int vendor, unsigned int 
> device, struct pci_dev *from)
>   */
>  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
>  {
> -   struct pci_dev *dev;
> -   struct pci_device_id *id;
> +   struct pci_device_id id;
>
> -   id = kzalloc(sizeof(*id), GFP_KERNEL);
> -   if (!id)
> -   return NULL;
> -   id->vendor = id->device = id->subvendor = id->subdevice = PCI_ANY_ID;
> -   id->class_mask = PCI_ANY_ID;
> -   id->class = class;
> +   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
> +   id.class_mask = PCI_ANY_ID;
> +   id.class = class;
>
> -   dev = pci_get_dev_by_id(id, from);
> -   kfree(id);
> -   return dev;
> +   return pci_get_dev_by_id(, from);
>  }
>
>  /**

with this one in pci/next pci config in /sys are not created.

10:~ # lspci -tv
pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
lspci: Unable to read the standard configuration space header of
device :00:03.0
pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
lspci: Unable to read the standard configuration space header of
device :00:02.0
pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
lspci: Unable to read the standard configuration space header of
device :00:01.3
pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
lspci: Unable to read the standard configuration space header of
device :00:01.1
pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
lspci: Unable to read the standard configuration space header of
device :00:01.0
pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
lspci: Unable to read the standard configuration space header of
device :00:00.0
-[:00]-

bisected to this commit

ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
commit ccee7d23102f5e5765ec24779c5b77472af8f79e
Author: Feng Tang 
Date:   Thu Aug 23 15:45:03 2012 +0800

PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid kmalloc

This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682

pci_get_subsys() may get called in late system reboot stage, using
a sleepable kmalloc() sounds fragile and will cause a kernel warning
with my recent commmit 55c844a "x86/reboot: Fix a warning message
triggered by stop_other_cpus()" which disable local interrupt in
late system shutdown/reboot phase. Using a local parameter instead
will fix it and make it eligible for calling forom atomic context.

Do the same change for the pci_get_class() as suggested by Bjorn Helgaas

[bhelgaas:

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-07 Thread Yinghai Lu 
On Thu, Aug 23, 2012 at 12:45 AM, Feng Tang feng.t...@intel.com wrote:
 From 57a28ee5e7662ca28ba4c793aa037d64bd082dee Mon Sep 17 00:00:00 2001
 From: Feng Tang feng.t...@intel.com
 Date: Wed, 22 Aug 2012 15:41:51 +0800
 Subject: [PATCH 1/2] PCI: Use local parameter pci_device_id for 
 pci_get_subsys/class()

 This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682

 pci_get_subsys() may get called in late system reboot stage, using
 a sleepable kmalloc() sounds fragile and will casue a kernel warning
 with my recent commmit 55c844a x86/reboot: Fix a warning message
 triggered by stop_other_cpus() which disable local interrupt in
 late system shutdown/reboot phase. Using a local parameter instead
 will fix it and make it eligible for calling forom atomic context.

 Do the same change for the pci_get_class() as suggeted by Bjorn Helgaas

 Bisected-by: Fengguang Wu fengguang...@intel.com
 Signed-off-by: Feng Tang feng.t...@intel.com
 Reviewed-by: Fengguang Wu fengguang...@intel.com
 ---
  drivers/pci/search.c |   35 +++
  1 files changed, 11 insertions(+), 24 deletions(-)

 diff --git a/drivers/pci/search.c b/drivers/pci/search.c
 index 993d4a0..78a08b1 100644
 --- a/drivers/pci/search.c
 +++ b/drivers/pci/search.c
 @@ -245,8 +245,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
 unsigned int device,
unsigned int ss_vendor, unsigned int ss_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,18 +256,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;

 -   return pdev;
 +   return pci_get_dev_by_id(id, from);
  }

  /**
 @@ -307,19 +300,13 @@ pci_get_device(unsigned int vendor, unsigned int 
 device, struct pci_dev *from)
   */
  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
  {
 -   struct pci_dev *dev;
 -   struct pci_device_id *id;
 +   struct pci_device_id id;

 -   id = kzalloc(sizeof(*id), GFP_KERNEL);
 -   if (!id)
 -   return NULL;
 -   id-vendor = id-device = id-subvendor = id-subdevice = PCI_ANY_ID;
 -   id-class_mask = PCI_ANY_ID;
 -   id-class = class;
 +   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
 +   id.class_mask = PCI_ANY_ID;
 +   id.class = class;

 -   dev = pci_get_dev_by_id(id, from);
 -   kfree(id);
 -   return dev;
 +   return pci_get_dev_by_id(id, from);
  }

  /**

with this one in pci/next pci config in /sys are not created.

10:~ # lspci -tv
pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
lspci: Unable to read the standard configuration space header of
device :00:03.0
pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
lspci: Unable to read the standard configuration space header of
device :00:02.0
pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
lspci: Unable to read the standard configuration space header of
device :00:01.3
pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
lspci: Unable to read the standard configuration space header of
device :00:01.1
pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
lspci: Unable to read the standard configuration space header of
device :00:01.0
pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
lspci: Unable to read the standard configuration space header of
device :00:00.0
-[:00]-

bisected to this commit

ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
commit ccee7d23102f5e5765ec24779c5b77472af8f79e
Author: Feng Tang feng.t...@intel.com
Date:   Thu Aug 23 15:45:03 2012 +0800

PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid kmalloc

This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682

pci_get_subsys() may get called in late system reboot stage, using
a sleepable kmalloc() sounds fragile and will cause a kernel warning
with my recent commmit 55c844a x86/reboot: Fix a warning message
triggered by stop_other_cpus() which disable local interrupt in
late system shutdown/reboot phase. Using a local parameter instead
will fix it and make it eligible for calling forom atomic context.

Do the same change for the pci_get_class() as suggested by Bjorn Helgaas

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-07 Thread Yinghai Lu 
On Fri, Sep 7, 2012 at 6:00 PM, Yinghai Lu ying...@kernel.org wrote:
 On Thu, Aug 23, 2012 at 12:45 AM, Feng Tang feng.t...@intel.com wrote:
 From 57a28ee5e7662ca28ba4c793aa037d64bd082dee Mon Sep 17 00:00:00 2001
 From: Feng Tang feng.t...@intel.com
 Date: Wed, 22 Aug 2012 15:41:51 +0800
 Subject: [PATCH 1/2] PCI: Use local parameter pci_device_id for 
 pci_get_subsys/class()

 This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682

 pci_get_subsys() may get called in late system reboot stage, using
 a sleepable kmalloc() sounds fragile and will casue a kernel warning
 with my recent commmit 55c844a x86/reboot: Fix a warning message
 triggered by stop_other_cpus() which disable local interrupt in
 late system shutdown/reboot phase. Using a local parameter instead
 will fix it and make it eligible for calling forom atomic context.

 Do the same change for the pci_get_class() as suggeted by Bjorn Helgaas

 Bisected-by: Fengguang Wu fengguang...@intel.com
 Signed-off-by: Feng Tang feng.t...@intel.com
 Reviewed-by: Fengguang Wu fengguang...@intel.com
 ---
  drivers/pci/search.c |   35 +++
  1 files changed, 11 insertions(+), 24 deletions(-)

 diff --git a/drivers/pci/search.c b/drivers/pci/search.c
 index 993d4a0..78a08b1 100644
 --- a/drivers/pci/search.c
 +++ b/drivers/pci/search.c
 @@ -245,8 +245,7 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, 
 unsigned int device,
unsigned int ss_vendor, unsigned int 
 ss_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,18 +256,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;

 -   return pdev;
 +   return pci_get_dev_by_id(id, from);
  }

  /**
 @@ -307,19 +300,13 @@ pci_get_device(unsigned int vendor, unsigned int 
 device, struct pci_dev *from)
   */
  struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from)
  {
 -   struct pci_dev *dev;
 -   struct pci_device_id *id;
 +   struct pci_device_id id;

 -   id = kzalloc(sizeof(*id), GFP_KERNEL);
 -   if (!id)
 -   return NULL;
 -   id-vendor = id-device = id-subvendor = id-subdevice = PCI_ANY_ID;
 -   id-class_mask = PCI_ANY_ID;
 -   id-class = class;
 +   id.vendor = id.device = id.subvendor = id.subdevice = PCI_ANY_ID;
 +   id.class_mask = PCI_ANY_ID;
 +   id.class = class;

 -   dev = pci_get_dev_by_id(id, from);
 -   kfree(id);
 -   return dev;
 +   return pci_get_dev_by_id(id, from);
  }

  /**

 with this one in pci/next pci config in /sys are not created.

 10:~ # lspci -tv
 pcilib: Cannot open /sys/bus/pci/devices/:00:03.0/config
 lspci: Unable to read the standard configuration space header of
 device :00:03.0
 pcilib: Cannot open /sys/bus/pci/devices/:00:02.0/config
 lspci: Unable to read the standard configuration space header of
 device :00:02.0
 pcilib: Cannot open /sys/bus/pci/devices/:00:01.3/config
 lspci: Unable to read the standard configuration space header of
 device :00:01.3
 pcilib: Cannot open /sys/bus/pci/devices/:00:01.1/config
 lspci: Unable to read the standard configuration space header of
 device :00:01.1
 pcilib: Cannot open /sys/bus/pci/devices/:00:01.0/config
 lspci: Unable to read the standard configuration space header of
 device :00:01.0
 pcilib: Cannot open /sys/bus/pci/devices/:00:00.0/config
 lspci: Unable to read the standard configuration space header of
 device :00:00.0
 -[:00]-

 bisected to this commit

 ccee7d23102f5e5765ec24779c5b77472af8f79e is the first bad commit
 commit ccee7d23102f5e5765ec24779c5b77472af8f79e
 Author: Feng Tang feng.t...@intel.com
 Date:   Thu Aug 23 15:45:03 2012 +0800

 PCI: Use pci_device_id on stack for pci_get_subsys/class() to avoid 
 kmalloc

 This fixes a kernel warning https://lkml.org/lkml/2012/7/31/682

 pci_get_subsys() may get called in late system reboot stage, using
 a sleepable kmalloc() sounds fragile and will cause a kernel warning
 with my recent commmit 55c844a x86/reboot: Fix a warning message
 triggered by stop_other_cpus() which disable local interrupt in
 late system shutdown/reboot phase. Using a local parameter instead
 will fix it and make it eligible for

Re: [PATCH 1/2] PCI: Use local parameter pci_device_id for pci_get_subsys/class()

2012-09-07 Thread Greg Kroah-Hartman 
On Fri, Sep 07, 2012 at 06:32:48PM -0700, Yinghai Lu wrote:
 Greg,
 
 Any reason for using kmalloc instead of local variable during your
 rewriting pci search code?
 
 commit 95247b57ed844511a212265b45cf9a919753aea1
 Author: Greg Kroah-Hartman gre...@suse.de
 Date:   Wed Feb 13 11:03:58 2008 -0800

Seriously?  Something I wrote 4 years ago?  I really have no idea,
sorry.

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/