| Message ID | 49E96731.1000501@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
* Yinghai Lu <yinghai@kernel.org> wrote: > Impact: fix bug i think this needs to be marked Cc: <stable@kernel.org> as well, for 2.6.29.x, maybe even 2.6.28.x ? ( Please note a small commit log detail: a few days go we started putting impact lines to the end of the commit as 'footers', in square brackets - right before the signoff lines. We do this to move them closer to other mechanic-looking tags and to not intrude the flow of the natural-language story line of the commit. Also note that 'fix bug' is not a good impact line even if it was a footer, because it does not really summarize the effects of a patch specifically enough. A better variant would be: [ Impact: fix corrupted names in /proc/iomem ] I've inserted this impact line into your commit below, to show the exact placement we started using. Note, this impact line would also be a perfect summary line, if the 'pci: ' tag is added before it: pci: fix corrupted names in /proc/iomem Jesse or Linus might opt to remove the impact line - it's a per subsystem discretion thing. ) Ingo > notice one system /proc/iomem some entries missed the name for pci_devices > > # cat /proc/iomem > 00000000-000973ff : System RAM > 00097400-0009ffff : reserved > 000a0000-000bffff : PCI Bus #00 > 000c0000-000cffff : pnp 00:0c > 000e0000-000fffff : reserved > 00100000-b7f9ffff : System RAM > 00200000-00c67b4b : Kernel code > 00c67b4c-01331edf : Kernel data > 015a5000-01fc9657 : Kernel bss > 20000000-23ffffff : GART > b7fae000-b7faffff : System RAM > b7fb0000-b7fbdfff : ACPI Tables > b7fbe000-b7feffff : ACPI Non-volatile Storage > b7ff0000-b7ffffff : reserved > b8000000-beffffff : PCI Bus #00 > bf000000-bfffffff : PCI Bus #80 > bfe80000-bfebffff : pnp 00:0e > bfef9000-bfef9fff : > bfef9000-bfef9fff : forcedeth > bfefa000-bfefa00f : > bfefa000-bfefa00f : forcedeth > bfefa400-bfefa4ff : > bfefa400-bfefa4ff : forcedeth > bfefa800-bfefa80f : > bfefa800-bfefa80f : forcedeth > bfefac00-bfefacff : > bfefac00-bfefacff : forcedeth > bfefb000-bfefbfff : > bfefb000-bfefbfff : forcedeth > bfefc000-bfefcfff : > bfefc000-bfefcfff : sata_nv > bfefd000-bfefdfff : > bfefd000-bfefdfff : sata_nv > bfefe000-bfefefff : > bfefe000-bfefefff : sata_nv > bfeff000-bfefffff : IOAPIC 1 > bfeff000-bfefffff : > ... > > it turns that we need to reget res->name because dev->dev.kobj name is changed > after device_add. > > [ Impact: fix corrupted names in /proc/iomem ] > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > --- > drivers/pci/bus.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > Index: linux-2.6/drivers/pci/bus.c > =================================================================== > --- linux-2.6.orig/drivers/pci/bus.c > +++ linux-2.6/drivers/pci/bus.c > @@ -70,6 +70,19 @@ pci_bus_alloc_resource(struct pci_bus *b > return ret; > } > > +static void pci_dev_update_res_name(struct pci_dev *dev) > +{ > + int idx; > + > + /* after device_add will get new name, reget it */ > + for (idx = 0; idx <= PCI_ROM_RESOURCE; idx++) { > + struct resource *res = &dev->resource[idx]; > + > + if (res->name) > + res->name = pci_name(dev); > + } > +} > + > /** > * pci_bus_add_device - add a single device > * @dev: device to add > @@ -84,6 +97,7 @@ int pci_bus_add_device(struct pci_dev *d > if (retval) > return retval; > > + pci_dev_update_res_name(dev); > dev->is_added = 1; > pci_proc_attach_device(dev); > pci_create_sysfs_dev_files(dev); -- 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
On Sat, 18 Apr 2009 09:51:30 +0200 Ingo Molnar <mingo@elte.hu> wrote: > > * Yinghai Lu <yinghai@kernel.org> wrote: > > > Impact: fix bug > > i think this needs to be marked Cc: <stable@kernel.org> as well, for > 2.6.29.x, maybe even 2.6.28.x ? > > ( Please note a small commit log detail: a few days go we started > putting impact lines to the end of the commit as 'footers', in > square brackets - right before the signoff lines. We do this to > move them closer to other mechanic-looking tags and to not intrude > the flow of the natural-language story line of the commit. > > Also note that 'fix bug' is not a good impact line even if it was > a footer, because it does not really summarize the effects of a > patch specifically enough. A better variant would be: > > [ Impact: fix corrupted names in /proc/iomem ] > > I've inserted this impact line into your commit below, to show the > exact placement we started using. Note, this impact line would > also be a perfect summary line, if the 'pci: ' tag is added before > it: > > pci: fix corrupted names in /proc/iomem > > Jesse or Linus might opt to remove the impact line - it's a per > subsystem discretion thing. ) Yeah I noticed the x86 patches seem to have those "Impact" lines these days, but I couldn't figure out what they meant. Sometimes they indicate the symptom being addressed, other times they act as a sort of summary subject. What's the intention? Is it really "user visible impact"? Or something else? Patch subjects generally suffer from similar ambiguity (sometimes describing what the patch is doing to the code, other times what issue the patch is addressing), so it would be nice if "Impact" was something separate and well defined.
On Fri, 17 Apr 2009, Yinghai Lu wrote: > > it turns that we need to reget res->name because dev->dev.kobj name is changed > after device_add. Can we not make the rule be that the name should just be set before? IOW, there is something else broken, I think. Rather than put this ugly band-aid, why now make sure that whoever had that broken name fixes it? Linus -- 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
Linus Torvalds wrote: > > On Fri, 17 Apr 2009, Yinghai Lu wrote: >> it turns that we need to reget res->name because dev->dev.kobj name is changed >> after device_add. > > Can we not make the rule be that the name should just be set before? > > IOW, there is something else broken, I think. Rather than put this ugly > band-aid, why now make sure that whoever had that broken name fixes it? > driver core guys are enforcing to use dev_name(device) instead of referring it. for pci code: via acpi_pci_root_driver.ops.add (aka acpi_pci_root_add) ==> pci_acpi_scan_root is used to scan pci bus/device, and at the same we read the resource for pci_dev at this point still need to use bus->devices to go over all pci_devices if needed. in the pci_read_bases, we have res->name = pci_name(pci_dev); pci_name is calling dev_name. later via acpi_pci_root_driver.ops.start (aka acpi_pci_root_start) ==> pci_bus_add_device to add all pci_dev in kobj tree. pci_bus_add_device will call device_add. actually in device_add /* first, register with generic layer. */ error = kobject_add(&dev->kobj, dev->kobj.parent, "%s", dev_name(dev)); if (error) goto Error; will get one new name for that kobj, old name is freed. Will try to make kobject_add more smart to reuse the old one. YH -- 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
On Sat, Apr 18, 2009 at 12:19:21PM -0700, Yinghai Lu wrote: > Linus Torvalds wrote: > > > > On Fri, 17 Apr 2009, Yinghai Lu wrote: > >> it turns that we need to reget res->name because dev->dev.kobj name is changed > >> after device_add. > > > > Can we not make the rule be that the name should just be set before? > > > > IOW, there is something else broken, I think. Rather than put this ugly > > band-aid, why now make sure that whoever had that broken name fixes it? > > > > driver core guys are enforcing to use dev_name(device) instead of referring it. By "enforcing" you now mean, "there is no other way" :) > for pci code: > > via acpi_pci_root_driver.ops.add (aka acpi_pci_root_add) ==> > pci_acpi_scan_root is used to scan pci bus/device, and at the same we > read the resource for pci_dev at this point still need to use > bus->devices to go over all pci_devices if needed. > in the pci_read_bases, we have res->name = pci_name(pci_dev); pci_name > is calling dev_name. > > later via acpi_pci_root_driver.ops.start (aka acpi_pci_root_start) ==> > pci_bus_add_device to add all pci_dev in kobj tree. > > pci_bus_add_device will call device_add. > > actually in device_add > > /* first, register with generic layer. */ > error = kobject_add(&dev->kobj, dev->kobj.parent, "%s", dev_name(dev)); > if (error) > goto Error; > > will get one new name for that kobj, old name is freed. > > Will try to make kobject_add more smart to reuse the old one. I don't understand the problem here, how are you going to change the kobject core? Is this just because you aren't getting a name for the resource? If so, why would the driver core care about this? confused, greg k-h -- 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
On Sat, Apr 18, 2009 at 21:23, Greg KH <gregkh@suse.de> wrote: > On Sat, Apr 18, 2009 at 12:19:21PM -0700, Yinghai Lu wrote: >> Linus Torvalds wrote: >> > Can we not make the rule be that the name should just be set before? I guess, that's what they do already, otherwise they could not use dev_name(). >> pci_bus_add_device will call device_add. >> >> actually in device_add >> >> Â Â Â Â /* first, register with generic layer. */ >> Â Â Â Â error = kobject_add(&dev->kobj, dev->kobj.parent, "%s", dev_name(dev)); >> Â Â Â Â if (error) >> Â Â Â Â Â Â Â Â goto Error; >> >> will get one new name for that kobj, old name is freed. >> >> Will try to make kobject_add more smart to reuse the old one. > > I don't understand the problem here, how are you going to change the > kobject core? Â Is this just because you aren't getting a name for the > resource? Â If so, why would the driver core care about this? Seems a bit odd what we do when registering a device. Usually one sets the name with dev_set_name() which will name the kobject. When one later registers the device, we reallocate the same name, so the pointer changes. I guess the problem here is that the pci code remembers the name pointer after it was set, but it will not be the same after the device is live. We should probably make device_add() pass a NULL as the name, and kobject_add() in that case use the name that is properly set already. Thanks, Kay -- 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
Index: linux-2.6/drivers/pci/bus.c =================================================================== --- linux-2.6.orig/drivers/pci/bus.c +++ linux-2.6/drivers/pci/bus.c @@ -70,6 +70,19 @@ pci_bus_alloc_resource(struct pci_bus *b return ret; } +static void pci_dev_update_res_name(struct pci_dev *dev) +{ + int idx; + + /* after device_add will get new name, reget it */ + for (idx = 0; idx <= PCI_ROM_RESOURCE; idx++) { + struct resource *res = &dev->resource[idx]; + + if (res->name) + res->name = pci_name(dev); + } +} + /** * pci_bus_add_device - add a single device * @dev: device to add @@ -84,6 +97,7 @@ int pci_bus_add_device(struct pci_dev *d if (retval) return retval; + pci_dev_update_res_name(dev); dev->is_added = 1; pci_proc_attach_device(dev); pci_create_sysfs_dev_files(dev);
Impact: fix bug notice one system /proc/iomem some entries missed the name for pci_devices # cat /proc/iomem 00000000-000973ff : System RAM 00097400-0009ffff : reserved 000a0000-000bffff : PCI Bus #00 000c0000-000cffff : pnp 00:0c 000e0000-000fffff : reserved 00100000-b7f9ffff : System RAM 00200000-00c67b4b : Kernel code 00c67b4c-01331edf : Kernel data 015a5000-01fc9657 : Kernel bss 20000000-23ffffff : GART b7fae000-b7faffff : System RAM b7fb0000-b7fbdfff : ACPI Tables b7fbe000-b7feffff : ACPI Non-volatile Storage b7ff0000-b7ffffff : reserved b8000000-beffffff : PCI Bus #00 bf000000-bfffffff : PCI Bus #80 bfe80000-bfebffff : pnp 00:0e bfef9000-bfef9fff : bfef9000-bfef9fff : forcedeth bfefa000-bfefa00f : bfefa000-bfefa00f : forcedeth bfefa400-bfefa4ff : bfefa400-bfefa4ff : forcedeth bfefa800-bfefa80f : bfefa800-bfefa80f : forcedeth bfefac00-bfefacff : bfefac00-bfefacff : forcedeth bfefb000-bfefbfff : bfefb000-bfefbfff : forcedeth bfefc000-bfefcfff : bfefc000-bfefcfff : sata_nv bfefd000-bfefdfff : bfefd000-bfefdfff : sata_nv bfefe000-bfefefff : bfefe000-bfefefff : sata_nv bfeff000-bfefffff : IOAPIC 1 bfeff000-bfefffff : ... it turns that we need to reget res->name because dev->dev.kobj name is changed after device_add. Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/bus.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) -- 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